Conversation
let?.x in script mode
overlookmotel
left a comment
There was a problem hiding this comment.
@copilot See below comment and action it.
let?.x in script modelet?.x with optional chaining
CodSpeed Performance ReportMerging #16840 will not alter performanceComparing Summary
Footnotes
|
let?.x with optional chaininglet?.x
99eb172 to
c7c05ba
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the parser to correctly handle optional chaining syntax with the let keyword (e.g., let?.x). Previously, the parser would fail to parse such expressions. The fix recognizes that in script mode (non-strict), let can be used as an identifier, and when followed by optional chaining (?.), it should be parsed as a member expression rather than a variable declaration.
Key Changes:
- Updated parser logic to recognize
Kind::QuestionDotafterletkeyword - Added test cases for both valid (script mode) and invalid (module mode) usage
- Fixed the
dot-notationESLint conformance test that was previously failing
Reviewed changes
Copilot reviewed 2 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_parser/src/js/declaration.rs |
Added Kind::QuestionDot to the pattern match that handles let as an identifier with property access |
tasks/coverage/misc/pass/let-optional-chaining.cjs |
Test file demonstrating valid usage of let?.x in script mode (CommonJS) |
tasks/coverage/misc/fail/let-optional-chaining.js |
Test file demonstrating invalid usage of let?.x in module mode (should produce errors) |
tasks/coverage/snapshots/parser_misc.snap |
Updated snapshot showing 4 new error cases for the fail test and incremented test counts |
tasks/coverage/snapshots/semantic_misc.snap |
Updated snapshot with incremented test counts |
tasks/coverage/snapshots/transformer_misc.snap |
Updated snapshot with incremented test counts |
tasks/coverage/snapshots/formatter_misc.snap |
Updated snapshot with incremented test counts |
tasks/coverage/snapshots/codegen_misc.snap |
Updated snapshot showing one expected failure (pre-existing bug per PR description) |
apps/oxlint/conformance/snapshot.md |
Updated conformance metrics showing dot-notation rule now fully passing (69/69 tests) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
The parser tweak is minimal and likely correct, but it changes the declaration-vs-expression routing based on a broader lookahead set and deserves extra regression coverage in statement-sensitive positions. The new module-mode failure fixture produces repetitive diagnostics, making snapshots noisier than necessary. codegen_misc.snap indicates the new fixture lowers codegen positive pass rate, which risks institutionalizing a red/unstable coverage metric even if the underlying idempotency issue is “pre-existing.”
Additional notes (2)
- Maintainability |
crates/oxc_parser/src/js/declaration.rs:24-31
The change to includeKind::QuestionDotfixes the immediate parse forlet?.x, but this branch now routes intoparse_expr()based solely on lookahead. That can widen the set of token sequences that stop being treated aslet-declaration candidates and instead become expression statements.
Even if the current tests pass, consider adding a focused regression case where let is followed by ?. inside contexts that are sensitive to statement vs declaration parsing (e.g. for (let?.x;;) {} / if (cond) let?.x / label: let?.x) to ensure this early decision doesn’t create new ambiguities.
- Maintainability |
tasks/coverage/misc/fail/let-optional-chaining.js:1-5
The module-mode failure fixture is currently expecting a reserved-word error at eachlet?.…line. That’s good for coverage, but it’s somewhat redundant and makes the snapshot noisy (multiple near-identical diagnostics).
Consider reducing to a single representative let?.x; in the fail fixture unless you specifically need to assert multiple optional-chaining forms fail at parse time in module mode.
Summary of changes
What changed
Parser behavior
- Updated
crates/oxc_parser/src/js/declaration.rsto treatlet?.…as an expression-statement continuation by includingKind::QuestionDotin the lookahead set (Dot | QuestionDot | LParen).
Test coverage
- Added script-mode pass fixture:
tasks/coverage/misc/pass/let-optional-chaining.cjs. - Added module-mode fail fixture:
tasks/coverage/misc/fail/let-optional-chaining.js.
Snapshots / conformance
- Updated various snapshots to reflect the additional fixture and its expected pass/fail outcomes:
tasks/coverage/snapshots/{parser_misc,formatter_misc,codegen_misc,semantic_misc,transformer_misc}.snap
- Updated
apps/oxlint/conformance/snapshot.mdtotals and moveddot-notationinto the fully passing set.
Fixes #16839.
Make parser parse
let?.xsuccessfully.There's a failure in codegen idempotency tests, but that's an unrelated and pre-existing bug, so am leaving that to another PR.