Skip to content

print: accept File nodes and weave comments into the output - #73

Merged
NullVoxPopuli merged 3 commits into
NullVoxPopuli:mainfrom
NullVoxPopuli-ai-agent:print-file-comments
Jul 12, 2026
Merged

print: accept File nodes and weave comments into the output#73
NullVoxPopuli merged 3 commits into
NullVoxPopuli:mainfrom
NullVoxPopuli-ai-agent:print-file-comments

Conversation

@NullVoxPopuli-ai-agent

@NullVoxPopuli-ai-agent NullVoxPopuli-ai-agent commented Jul 12, 2026

Copy link
Copy Markdown

Follow-up from ember.nvp#58 (comment): print() threw unsupported node type 'File' on the node toTree returns, and since comments only live on File.comments, reprinting a file dropped every comment — which blocked using print() for ember.nvp's TS→JS import-specifier rewrite.

What this does

  • print(file) now prints file.program with file.comments woven back in: each comment is emitted immediately before the nearest printed node that follows it in the original source (which matches source order for parsed trees), and comments after the last statement are appended at the end of the file. Works for comments nested inside functions/classes (they pick up block indentation naturally) and in gjs/gts files alongside <template> regions.
  • Placement is approximate by design: a same-line trailing comment (let a = 1; // hi) becomes a leading comment of the next node. The intended pairing is print() + a formatter (prettier), per the ember.nvp use case, and the README documents this.
  • Implementation: the switch stays inside print() exactly as before; weaving is a top guard that only engages while a comment-carrying print(File) is in flight. On a comment hit it flushes the pending comments and re-enters (the cursor has advanced, so the recursion falls straight through to the switch). Weaving state is module-level (print is synchronous), saved/restored in printFile, so standalone print(node) calls behave exactly as before.

Performance

  • Complexity: comment flushing is a single monotone cursor over the sorted comments array — O(nodes + comments) total for a whole-file print; each comment is visited exactly once regardless of node count. No per-node allocation is added.
  • Standalone print(node) (the existing use case): pays one fileComments !== null check and one extra case "File" arm; measured on a synthetic 4,000-line / 1,200-comment file (median of 50 runs, interleaved with main as control): main 0.50ms vs this branch 0.50ms (−1%, within noise). An earlier revision of this PR split print into a wrapper + printNode, which cost an extra call frame per node (~7–8%); that structure is gone.
  • print(File) with weaving (new capability): 0.69ms vs 0.50ms for the same file's bare program — the delta is the 1,200 comment emissions and concatenations themselves.
  • CI Benchmark Comparison: green, but note it only exercises parse() (gts/gjs/hbs fixtures) — it does not cover print(), so the local numbers above are the relevant ones for this change. Parse deltas are all within ±5% in mixed directions (noise; this diff doesn't touch the parse path).
  • No comment copying via spread: filter() provides the fresh array, sorted in place; the caller's file.comments is never mutated, and files without comments skip weaving entirely.

Testing

  • New tests/print-file.test.js (9 tests, inline snapshots of the full printed output per review): program printing, empty file, leading/block/doc comments, nested comments with indentation, trailing comments, gjs with templates, no state leakage between calls, File without a comments property.
  • pnpm test: 229 passing; pnpm lint clean.

Once this is released, ember.nvp's rewrite can switch to mutate-AST + print() + prettier as requested in ember.nvp#58.

🤖 Generated with Claude Code

print() previously threw on the File node that toTree returns, and
comments -- which only live on File.comments -- were never printed, so
reprinting a whole file silently dropped every comment.

print(file) now prints file.program with the comments flushed back in:
each comment is emitted immediately before the nearest printed node
that follows it in the original source (matching source order), and
comments after the last statement are appended at the end. Placement
is approximate by design -- a same-line trailing comment becomes a
leading comment of the following node -- so callers that care about
exact layout should run a formatter over the result.

Weaving state is module-level (print is synchronous), saved/restored
in printFile, and cleared afterwards so unrelated print calls are
unaffected.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread tests/print-file.test.js Outdated
const tree = toTree(source, { filePath: "m.js" });
const output = print(tree);

expect(output).toContain(`/* setup */`);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use snapshot tests for this sort of assertion. as is, the assertions are hard to read, and could be incorrect (do this for all new tests in this file)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — all nine tests now snapshot the full printed output via toMatchInlineSnapshot, so the expected text is visible verbatim in each test.

Per review: the toContain/indexOf assertions are replaced with
toMatchInlineSnapshot of the full printed output, so each test shows
exactly what print(File) produces.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Jul 12, 2026
Comment thread src/print.js Outdated
const previousComments = fileComments;
const previousCursor = commentCursor;

fileComments = [...(file.comments ?? [])]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't ever use [...arr] it's a huge performance footgun

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone in ee10c72filter() already yields a fresh array, so the spread was a pure waste; sorting happens in place on the filtered copy and the caller's file.comments is never mutated. Files with no comments skip weaving entirely.

That commit also includes a perf pass beyond this line (numbers in the updated PR body): the print/printNode split I'd introduced cost an extra call frame per node (~7–8% on standalone print(program) of a 4k-line file), so the switch is back inside print() as on main, with weaving as a top guard that short-circuits on one null test — standalone printing now measures at parity with main (−1%, noise).

Perf pass over the comment weaving:

- The print/printNode split added an extra call frame to every node,
  measuring ~7-8% overhead on standalone print(program) of a 4k-line
  file. The switch is back inside print() (as on main); weaving is now
  a top guard that short-circuits on a single null test when no
  comment-carrying print(File) is in flight, and on a comment hit it
  flushes and re-enters (the cursor has advanced, so the recursion
  falls through). Standalone printing now measures at parity with main
  (-1%, within noise).
- Comment flushing remains a monotone cursor over the sorted comments
  array: O(nodes + comments) total, each comment visited exactly once.
- Per review: the [...arr] spread in printFile is gone -- filter()
  already yields a fresh array, so sorting in place is safe and the
  caller's file.comments is never mutated. Files without comments skip
  weaving entirely (fileComments stays null).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@NullVoxPopuli-ai-agent

NullVoxPopuli-ai-agent commented Jul 12, 2026

Copy link
Copy Markdown
Author

Follow-up on the benchmark question ("why is the medium tier slower on this PR?") — I pulled every Benchmark Comparison run whose logs are still available (June runs have expired) and tabulated the signed control-vs-experiment delta per tier (positive = experiment faster):

run gts s gts m gts l gjs s gjs m gjs l hbs s hbs m hbs l
#73 run1 (4a58320) +8 +1 −1 −2 −4 −2 −9 −2 −3
#73 run2 (b9e75df) 0 0 +2 +4 −4 −1 −2 −1 −5
#73 run3 (ee10c72) +6 +6 −4 0 −3 +2 −1 −4 −3
PR #71 (bench fmt) +1 +3 0 −2 −3 +3 −5 +1 −1
PR #70 (backtick fix) +4 +2 +1 −2 −4 +2 −10 −2 −3
bump-deps +17 +15 −6 +16 −3 −3 −4 +3 −1
PR (sort-comments) +5 +5 +1 +2 −2 −1 −6 +3 0
PR (ast-ref-doc, docs-only) +4 +3 0 +9 +6 0 −4 +6 0
PR (scope-tracker) +14 +5 +1 +3 −11 0 −5 −2 −2
main push +3 +8 −1 +12 +4 0 0 −1 −1

Medium-tier |Δ| across runs: #73 (3 runs × 3 tiers): max 6%, mean 2.8% — versus other runs: max 15%, mean 4.4%. Notably the docs-only PR (README generation, zero source changes) shows +6% swings on two medium tiers, and this PR's spread is run-to-run random rather than commit-correlated (gts-medium across #73's three runs: +1 / 0 / +6; two of those runs predate the perf restructuring, one follows it — same band).

Mechanism check, in case the table wasn't enough:

  • The bench only calls parse(). parse.js/transforms.js/tokens.js never import print.js, and print.js shares no helpers with the parse path — this diff cannot execute during the benchmark. Loading src/index.js evaluates print.js, but that only defines functions plus two module-level bindings (null/0) — nothing retained, no GC pressure.
  • Local interleaved comparison (6 alternating rounds × 30 iters, --expose-gc, medium fixtures, main worktree vs this branch): gts-medium −4.5%, gjs-medium −0.0%, hbs-medium +2.4% — parity, with round medians overlapping.

So the medium numbers on this PR are inside the harness's normal cross-run variance (and slightly tighter than the repo average). The earlier absolute-time question — why medium takes ~70ms at all — is fixture composition: medium is ~100× the bytes of small (42KB vs 0.4KB), cost is linear in input, and ~60–80% of parse time is @glimmer/syntax parsing of the <template> regions (oxc itself is <1ms even on large).

@NullVoxPopuli
NullVoxPopuli merged commit f575c4f into NullVoxPopuli:main Jul 12, 2026
4 checks passed
@NullVoxPopuli
NullVoxPopuli deleted the print-file-comments branch July 12, 2026 23:48
@github-actions github-actions Bot mentioned this pull request Jul 12, 2026
NullVoxPopuli-ai-agent pushed a commit to NullVoxPopuli-ai-agent/ember.nvp that referenced this pull request Jul 13, 2026
ember-estree 0.6.11 ships print(File) with comment weaving
(NullVoxPopuli/ember-estree#73), so the rewrite is now the requested
pipeline: mutate the Literal specifier nodes on the AST, print() the
File, and run prettier over the result. The replaceAll step is gone.
Files with no specifiers to rewrite are returned untouched.

Prettier runs with the emitted app's config (printWidth 100, template
tag plugin -- passed as a module since prettier resolves plugin names
against the generated project's cwd, not this package). While here,
babel-remove-types now also formats with the app's config instead of
its singleQuote default, which previously made every converted file
fail a generated JS project's own lint:prettier.

Known cosmetic gap: print() collapses blank lines between statements,
so a JS project with the eslint layer reports blank-line diagnostics
on tests/test-helper.js (simple-import-sort group separation,
newline-after-import, padding-line-between-statements). Blank-line
preservation in ember-estree's printer is the follow-up.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants