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 5 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.
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
01c1374 to
f9a1f5e
Compare
|
Looks like the |
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
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.
…ects
Review of the interpolated-template glob surfaced that it shipped in the
minimal build and that its decoder-side reconstruction diverged per port. Both
are fixed by keeping the whole feature full-build-only and letting the parser be
the single source of the substitution spans the decoders splice.
1. Min-build gating: the TemplateSpan struct, the Import fields, the rt/te/rts
readers, and both recording sites are now behind LEXER_MIN, and _rt/_te/_rts
are dropped from the three minimal export lists. es-module-shims reads
specifiers via source.slice and never globs, so lexer.min.wasm is unchanged.
2. No eval on the glob path: the Wasm/asm decoder cooked its skeleton with
(0, eval), so import(` ${sideEffect()}.js`) (a space defeats the C gate but
not the decoder) executed the substitution during parse(). All three ports
now copy the static parts raw, so nothing is evaluated and the builds agree
byte-for-byte (this also removes the CR/CRLF normalization divergence).
3. rt() stays exhausted: it reset its read head to NULL on the last span and
the next call reloaded the first span, so a decoder meeting more top-level
${ than recorded spans jumped backward and looped. It now latches exhausted.
4. Nested imports no longer corrupt the outer glob: recording scratch is
per-import (keyed off dynamicImportStack) in the parser, and lexer.js gained
the dynamic-import stack it lacked, which also restores the outer import's
end/se that a nested dynamic import previously dropped.
5. Consistent lone-template detection: the C backtick gate keys on the recorded
specifier start and finalization keeps the spans only when the specifier's
closing backtick is the last token, so whitespace before/after the argument
(e.g. Prettier's multiline form) globs identically across builds instead of
three different answers.
Static parts stay raw source: escapes are not cooked and a literal "*" is
emitted verbatim, both documented. A nested dynamic import whose own specifier
is an interpolated template still yields undefined in the pure-JS port (the
Wasm/asm builds report the glob); documented as a known limitation.
Fixes: guybedford#137
The pure-JS port pushed the enclosing dynamic import onto a stack when
entering a nested import(...), but only popped it on the constant-specifier
fast path. Any nested import closing through the main loop's ")" instead
overwrote the outer import with null, losing its end/statement_end and
dropping its template glob. import(`a${ import(x) }c`) reported the outer
specifier as undefined instead of 'a*c', and any interpolated-template
nested import corrupted the enclosing entry the same way; this also removes
the previously documented limitation where a nested interpolated-template
import lost the outer glob only in this port.
Restoring from the stack in the ")" handler fixes both. As a side effect,
the three glob-recording WeakMap lookups now gate on the current import's
argument starting with a backtick first, skipping the lookup entirely for
the common non-template dynamic import.
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