feat(lexer): report glob n for interpolated template dynamic imports - #205
feat(lexer): report glob n for interpolated template dynamic imports#205BridgeAR wants to merge 6 commits into
Conversation
|
Review of #205 Sound. I verified the three decoders agree on the output across edge cases (multi-substitution, nested templates, strings/comments in the substitution body, escaped ${, attribute-following, concatenation negatives). The "lone template" detection differs per port but is equivalent: JS uses acornPos === lastTokenPos + 1, asm uses acornPos === e, wasm slices [s,e) and rejects an early backtick — all correctly reject One real bug worth fixing before merge: a regex literal containing } inside a substitution is mis-skipped (the comment admits "regex is not disambiguated"), and it produces a wrong glob rather than bailing to undefined:
All three ports share this (so they stay consistent), but silently emitting a wrong specifier is worse than undefined. It's an obscure case; acceptable to document as a known limitation, but ideally the skip logic should detect the mismatch and drop to undefined. |
|
I think if we are going to land this, it would be worthwhile basing it to and landing alongside #211 to separate these extra features from the core featureset used in es-module-shims. |
This comment was marked as outdated.
This comment was marked as outdated.
01c1374 to
f9a1f5e
Compare
|
Looks like the |
guybedford
left a comment
There was a problem hiding this comment.
This adds glob n for interpolated template dynamic imports (import(./a/${x}.js) → ./a/*.js). Reviewing against two criteria: (1) the new codepaths must be gated behind the full build so the minimal build size is entirely unaffected (no growth above 10 bytes), and (2) validity of the glob semantics. Criterion 1 fails — the feature is not gated and ships in the minimal build — and criterion 2 has several correctness defects, including an eval-based code-execution path and a cross-build hang.
Criterion 1 — min-build gating: fails
The entire feature is unconditional (src/lexer.h, chompfile.toml, src/lexer.c, src/lexer.ts). Nothing is behind #ifndef LEXER_MIN / MINIMAL:
_rt,_te,_rtsare added to all three minimal export lists —lexer.min.wasm(chompfile.toml:157),lexer.min.emcc.js(207),lexer.min.layout.js(224).struct Importgainstemplate_spansunconditionally. Note the siblingattributesfield directly above it is wrapped in#ifndef LEXER_MIN— so this reads as an omission, not an intentional exception.- The
TemplateSpanstruct, thetemplateSpanImport/specifierTemplateDepth/template_span_write_headglobals, thert/te/rtsreaders (lexer.h:261–279), and the recording code inconsumeToken's}and`cases plustemplateString(lexer.c:149–163, 175–185, 989–990) are all unconditional. src/lexer.tsdecodeTemplateand its call site are not behindMINIMAL.
Effect: three new wasm exports (retaining their bodies + name strings), a 4-byte-per-Import struct growth, and the span-recording branches all land in lexer.min.wasm — well over the 10-byte budget. This needs #ifndef LEXER_MIN end-to-end (struct field, globals, readers, both recording sites) and the three _rt/_te/_rts entries removed from the minimal export lists. The minimal consumer (es-module-shims) reads specifiers via source.slice and never calls these.
Criterion 2 — semantics
src/lexer.ts:345 — arbitrary code execution in the wasm build. decodeTemplate cooks its skeleton with decode(), which is (0, eval)(str) (lexer.ts:303). When the parser records no spans but the decoder still runs, wasm.rt() returns false at every ${, so the raw substitution text survives into the skeleton, the lone-template check still passes, and the whole literal is eval'd with its expressions live. This is reachable: the C gate (lexer.c:181) requires the backtick to abut (, but the decoder gate is only source[s] === '\``, so import( ${globalThis.alert(1)}.js)(one space after() records no spans yet enters decodeTemplate→alert(1)runs duringparse()`. The pure-JS port doesn't eval, so this is wasm-specific. A lexer documented as pure analysis must never eval source.
src/lexer.h:265 — infinite loop / hang in the wasm and asm builds. rt() sets template_span_read_head to NULL on exhaustion and returns false; the next rt() sees NULL and reloads import_read_head->template_spans — the first span again — returning true, so te() returns a position behind the scan cursor and the decoder jumps backward and loops forever. Reachable whenever the decoder meets more top-level-looking ${ than recorded spans (e.g. the nested-import under-recording below). The pure-JS port is bounded by spanIndex < spans.length and returns undefined — so this is both a DoS on the shipped wasm/asm builds and a three-way divergence.
src/lexer.c:182 / lexer.js:291 — nested dynamic-import template silently degrades the outer glob. templateSpanImport is a single global with no save/restore; an inner import(...) inside a substitution overwrites it and its close sets it back to NULL permanently, so the outer specifier's remaining ${...} are never recorded. Verified: import(a${ import(b${y}) }c) returns n === undefined for the outer import (expected a*c). The state is per-import but not stacked alongside dynamicImportStack.
lexer.js:187 — cross-build divergence on whitespace around the argument. The JS port anchors on charCodeAt(e-1) === '\`` with e = pos(the)position), so any whitespace/comment before)drops the glob; the C build useslastTokenPos + 1and keeps it. Verified:import(./a/${x}.js )→undefinedin JS but the glob in wasm/asm. It's also inconsistent *within* a build —import("./a.js" )returns./a.jsandimport(./a/${x}.js , y) returns the glob; only the space-before-)template form fails. Symmetrically, whitespace after(diverges the other way. Prettier emits exactly the multiline ``long`` + newline +)` form for long specifiers, so this is common.
src/lexer.ts:345 — CR/CRLF normalization divergence. The TS decoder eval-cooks the skeleton, which normalizes \r\n/\r → \n in static parts; the JS and asm decoders copy raw source slices. Verified: import(a\r\nb${x}) → a\r\nb* in the JS build but a\nb* in the wasm build.
README.md:243 — doc contradicts the implementation and the new tests. The added paragraph claims a substitution containing a / that could open a regex literal "cannot be disambiguated from division without full token context, so the glob is dropped to undefined." The implementation does the opposite (the real tokenizer records the spans), and test/_unit.cjs asserts import(a${ /x}y/g }b) → a*b and import(a${ b/c }d) → a*d. Delete or correct the paragraph.
lexer.js:58 — literal * is indistinguishable from a wildcard *. Static asterisks are emitted verbatim next to substitution wildcards: import(a*${x}.js) → a**.js. A consumer expanding n as a glob (the stated purpose) can't tell which * came from a substitution. Worth documenting; ideally escape literal * in the static parts.
Lower-priority cleanup
- The JS port attaches the internal
spansarray to the returned publicImportSpecifierobjects (lexer.js:26), so the JS result shape diverges from wasm's and leaks an undocumented field — keep it in module state or delete before return. - The three decoder walkers are near-duplicated with subtly different check order (the TS one validates the closing backtick after a full O(n) walk instead of before). The C backtick guard's five conjuncts reduce to
pos == dynamicImportStack[...]->start. - The comment blocks are heavier than this file's convention (bare declarations, one-line getter labels) and restate the same rationale across README, four source files, and the type doc.
Bottom line: criterion 1 is not met (feature is fully in the min build); criterion 2 surfaces an eval code-execution path, a wasm/asm hang, a nested-import correctness bug, and multiple cross-build divergences. Needs end-to-end LEXER_MIN gating plus fixes to the eval fallback, the rt() reload, the global templateSpanImport stacking, and the whitespace/e anchoring before merge.
* perf(lexer): skip identifier and number runs On the 3,057 KiB sample corpus, Wasm drops from 15,356.31 to 13,010.46 us per sweep on Node 18.20.8 (-15.28%) and from 11,724.34 to 9,856.69 us on Node 24.18.0 (-15.93%). The asm.js build drops from 15,934.34 to 12,945.57 us on Node 24.18.0 (-18.76%). The fast path adds 379 raw / 105 gzip bytes to the full Wasm binary. Measurements used 1,500 warmup sweeps followed by 9 interleaved trials of 50 sweeps, dropping the best and worst trials.
lexer.js reimplemented the WebAssembly lexer in hand-written JS, kept in sync by hand, but was unreachable through the package `exports` (only `.` and `./js` are exposed - the wasm and asm.js builds) and `chomp test` never ran it, so it could drift silently. The asm.js build already covers the no-WebAssembly case, and the removed `if (!js)` test guards now run unconditionally, matching their existing behavior under the wasm and asm suites.
A dynamic import whose entire argument is a template literal with substitutions
returned n: undefined, so a consumer resolving the specifier (a bundler or glob
importer) had nothing to work with. It now reports the static skeleton as a
glob with each ${...} collapsed to a single "*": import(`./locales/${x}.js`)
yields "./locales/*.js".
Only a lone template literal qualifies. A template concatenated with anything
else (import(`a` + b)) has no static skeleton and still returns undefined.
Fixes: guybedford#137
The glob walker that builds a dynamic-import template skeleton skips over
strings, nested templates, and comments inside each ${...} substitution, but it
cannot tell a regex literal from division without the main parser's token
context. A regex carrying a "}" closed the substitution early and emitted a
wrong specifier instead of bailing: import(`a${ /x}y/g }b`) reported "a*y/g }b"
rather than undefined.
skipInterpolation now flags a bare "/" (one that does not open a // or /*
comment) and the three decoders drop n to undefined rather than guess. This
over-bails the rare division case (import(`a${ b/c }d`)), which is acceptable:
a missing glob is recoverable, a wrong one is not.
Fixes: guybedford#137
The interpolated-template glob walked each ${...} substitution with a
hand-rolled scanner in all three decoders. None of them could tell a regex
literal from division without token context, so a regex carrying a "}" closed
the substitution early: import(`a${ /x}y/g }b`) reported "a*y/g }b". The prior
fix bailed to undefined on any bare "/", which also dropped legitimate division
(import(`a${ b/c }d`)).
The parser already resolves regex vs division for the whole source and descends
into ${ ... } for nested-import detection, so it now records each top-level
substitution's end on the dynamic import (struct TemplateSpan). The decoders
splice a "*" per span and jump the body, dropping their skipInterpolation /
skipQuoted / skipComment scanners and the interpolationError bail. Both
ambiguous cases now resolve correctly: "a*b" and "a*d".
Fixes: guybedford#137
…ects ## Summary Interpolated template glob tracking leaked into minimal builds and diverged between decoders. Missing parser spans could evaluate a substitution, reload an exhausted span list into a loop, or lose the outer glob around a nested import. ## Why Keep span tracking behind LEXER_MIN, use parser-recorded spans as the substitution boundary, and copy static source without eval. The full Wasm and asm.js builds now agree while minimal output stays unchanged. Static parts remain raw source, so escapes are not cooked and a literal * stays literal. ## Test plan - chomp test Fixes: guybedford#137
6f36e93 to
bbf7c3d
Compare
Summary
A dynamic import whose entire argument is a template literal with substitutions returned
n: undefined, so a consumer resolving the specifier (a bundler or glob importer) had nothing to work with. It now reports the static skeleton as a glob with each${...}collapsed to a single*:import(`./locales/${locale}.js`)yields./locales/*.js. Only a lone template literal qualifies — a template concatenated with anything else (import(`a` + b)) has no static skeleton and still returnsundefined.The change lives entirely in the three decoders (WASM
src/lexer.ts, asm.jssrc/lexer.asm.js, pure-JSlexer.js);src/lexer.cis untouched, since the parser already produces the correct specifier range and still descends into${ ... }for nested-import detection. Source-only — the generatedlib/artifacts are left for the build.Test plan
Dynamic import interpolated template specifier globtest covers multi-substitution, leading/trailing/whole substitution, escapes, string/comment/nested-template interpolation bodies, attribute-following, and the concatenation negatives.WIP / draft: opening early for CI and review feedback.
Fixes: #137