Repository navigation
feat(lexer): add opt-in extended export detection - #128
Conversation
guybedford
left a comment
There was a problem hiding this comment.
Thanks for putting this together. I checked out the branch, verified the committed Wasm rebuilds byte-for-byte from src/lexer.c, and ran the suite across all three targets. Some observations from probing the implementation, no conclusions drawn here, just surfacing the details for discussion.
The main design question I'd like to see written down in the PR is who the intended consumer of mode: 'extended' is. The README keeps the frozen-defaults rationale (avoiding version fragmentation), but an opt-in mode only avoids that if Node itself never adopts it, and if Node never adopts it, tools that aim to match Node's behaviour wouldn't have a reason to opt in. Clarifying which of these is intended would help frame the rest of the review.
Footprint for reference: Wasm 22.4K -> 24.2K (+8%), lexer.js gzipped 13.2K -> 14.2K (+7.6%). For the default path this is unused code.
On [/] tracking: since brackets are tracked only in extended mode, a regex misdetected as division containing a bracket class (e.g. /[\]]/) becomes a hard parse error only in extended mode. Low probability given the existing heuristics, but it's a mode divergence beyond detection patterns.
One pre-existing issue surfaced while testing (not introduced here): exports.\u0061b = 1 is filtered by the JS lexer but returned raw as \u0061b by Wasm. Same split carries into extended mode.
| Object spreads of `require('literal')` contribute reexports, but named properties with require-valued expressions do not. | ||
|
|
||
| The result contains syntactic candidates, which can include non-enumerable properties, throwing getters, and unreachable assignments. | ||
| Extended mode does not apply the default getter side-effect veto. |
There was a problem hiding this comment.
Dropping the unsafe-getter veto is a semantic change rather than broader syntax: the veto exists because non-matching getters can have side effects when names are snapshotted. Object.defineProperty(exports, 'x', { get: () => load() }) is now reported. The README documents it, but it should be called out in the PR description as a deliberate tradeoff.
e4f8a82 to
1df2d34
Compare
guybedford
left a comment
There was a problem hiding this comment.
Thanks for the updates. Re-verified on the new head: the committed Wasm rebuilds byte-identical from src/lexer.c with the pinned toolchain, the .wat matches, all three test targets pass, and default-mode output is unchanged against main across the sample files. JS and Wasm agree on every extended-mode case I probed (nested objects, spreads, regex and template values, accessors, computed and keyword keys, division after ], comments).
The flag-based entry point, default naming, depth guard, and the real webpack fixture all address the earlier threads, and the unterminated-braces check now lives on main for both modes.
Two remaining notes, neither blocking:
Bracket tracking in extended mode turns a misdetected regex into a hard error where default mode is silent. Concrete case is a labelled block followed by a regex containing a bracket class:
a: {}
/[\]]/.test(x); exports.z = 1;
// default: { exports: ['z'] }
// extended: Error: Unexpected closing bracket.Every other } form I tried (else, do, try, class, switch, arrow bodies) is handled correctly in both modes, so this is rare. Worth a line in the README's extended section.
Parenthesized comma expressions take the first operand rather than the last in both the module.exports and defineProperties paths, e.g. module.exports = ({ a: 1 }, { b: 2 }) yields a. Consistent now, and the README does not claim comma expressions, so just noting it.
Footprint for reference: Wasm 22.6K -> 24.3K, lexer.js gzipped 13.4K -> 14.3K.
1df2d34 to
5d64f0d
Compare
|
I believe this should be good to go. I tried to make the heuristic work in a way with the opt-in mode as such that it handles most cases correct, while not slowing down parsing drastically. |
The default matcher rejects parenthesized property descriptors generated by
webpack. Its unsafe-getter veto then suppresses declarations with the same
names.
This adds `parse(source, filename, { mode: 'extended' })` for broader
syntactic export detection. Extended mode accepts arbitrary object values
and descriptor bodies, skips simple leading comma operands, and recognizes
regex after labelled and switch case blocks. Its candidates can include
getters with side effects. Default detection remains frozen.
C and JavaScript retain separate scanners to preserve native Wasm scanning
and the standalone JavaScript build.
Refs: #88
Refs: #101
c2a54e6 to
59503f7
Compare
guybedford
left a comment
There was a problem hiding this comment.
Re-verified on the rebased head: squashed onto main after #135 and #136, resolving src/lexer.c against the linked star-export bindings. The Wasm is rebuilt with the current build flags and reproduces byte-identical from src/lexer.c, the .wat is regenerated, all three test targets pass, and default-mode output is unchanged against main across the sample files. JS and Wasm agree on every extended-mode case I probed, including the new labelled-block, switch-case, and comma-prefix handling.
The remaining extended-mode hard errors I could find are all regexes with a bracket class after a block whose preceding token the heuristic cannot classify (case a.b: {}, case f(): {}, a label inside a class static block, a label after ASI). Those fall under the README's documented limitation.
Landing.
The default matcher rejects parenthesized property descriptors generated by webpack. Its unsafe-getter veto then suppresses declarations with the same names.
This adds
parse(source, filename, { mode: 'extended' })for broader syntactic export detection. Extended mode accepts arbitrary object values and descriptor bodies. Its candidates can include getters with side effects. Default detection remains frozen.C and JavaScript retain separate scanners to preserve native Wasm scanning and the standalone JavaScript build.
Refs: #88
Refs: #101