Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for populating Oxc’s non-standard hashbang field in generated AST snapshots so hashbang-related Test262 cases no longer need to be skipped for ESTree conformance.
Changes:
- Update Test262 AST JSON fixtures to include a populated
hashbangnode where applicable - Stop force-inserting
hashbang: nullduring JSON stringify, and instead sethashbangduring parsing/AST construction - Add hashbang detection in both the TS-ESLint and Test262 runners
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test262/test/language/comments/hashbang/use-strict.json | Populate hashbang object for this fixture instead of null. |
| tests/test262/test/language/comments/hashbang/not-empty.json | Populate hashbang object for this fixture instead of null. |
| tests/test262/test/language/comments/hashbang/module.json | Populate hashbang object for this fixture instead of null. |
| tests/test262/test/language/comments/hashbang/line-terminator-paragraph-separator.json | Populate hashbang object for this fixture instead of null. |
| tests/test262/test/language/comments/hashbang/line-terminator-line-separator.json | Populate hashbang object for this fixture instead of null. |
| tests/test262/test/language/comments/hashbang/line-terminator-carriage-return.json | Populate hashbang object for this fixture instead of null. |
| src/utils/json.js | Remove the implicit mutation that always set ast.hashbang = null during stringify. |
| src/typescript-eslint.js | Add hashbang extraction from TS-ESLint “Shebang” comment into program.hashbang. |
| src/test262.js | Capture comments during parsing and synthesize ast.hashbang when the file starts with #!. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isModule = preamble.flags?.includes("module"); | ||
|
|
||
| let ast; | ||
| const comments = []; |
There was a problem hiding this comment.
The same comments array is reused across the Acorn parse attempt and the Meriyah fallback. If the first parse throws after pushing any comments, those partial results will remain in comments and can corrupt the subsequent hashbang detection. Consider using separate arrays per parse attempt, or clearing comments before invoking the fallback parser.
| // It defaults to `true` for modules, `false` for scripts, which is what we want. | ||
| onComment: comments, | ||
| }); | ||
| } catch { |
There was a problem hiding this comment.
The same comments array is reused across the Acorn parse attempt and the Meriyah fallback. If the first parse throws after pushing any comments, those partial results will remain in comments and can corrupt the subsequent hashbang detection. Consider using separate arrays per parse attempt, or clearing comments before invoking the fallback parser.
| } catch { | |
| } catch { | |
| // Clear any partial comments collected by Acorn before using the fallback parser. | |
| comments.length = 0; |
| globalReturn: true, | ||
| webcompat: true, // I think this enables support for Annex B | ||
| next: true, // Enable parsing decorators and import attributes | ||
| onComment: comments, |
There was a problem hiding this comment.
The same comments array is reused across the Acorn parse attempt and the Meriyah fallback. If the first parse throws after pushing any comments, those partial results will remain in comments and can corrupt the subsequent hashbang detection. Consider using separate arrays per parse attempt, or clearing comments before invoking the fallback parser.
| // Add `hashbang` property to AST if file starts with a hashbang. | ||
| // This property is non-standard and exclusive to Oxc. | ||
| if (comments.length > 0 && code.startsWith("#!") && comments[0].type === "Line") { | ||
| ast.hashbang = { ...comments[0], type: "Hashbang" }; |
There was a problem hiding this comment.
Constructing hashbang via { ...comments[0], ... } makes the emitted AST shape depend on whatever extra fields the parser includes on comment objects (which can vary across parsers/versions). To keep snapshots stable, consider explicitly picking only the expected fields (e.g., value, start, end) when building ast.hashbang.
| ast.hashbang = { ...comments[0], type: "Hashbang" }; | |
| const { value, start, end } = comments[0]; | |
| ast.hashbang = { type: "Hashbang", value, start, end }; |
| // Add `hashbang` property to AST if file starts with a hashbang. | ||
| // This property is non-standard and exclusive to Oxc. | ||
| if (comments.length > 0 && comments[0].type === "Shebang") { | ||
| const comment = comments[0]; | ||
| program.hashbang = { | ||
| type: "Hashbang", | ||
| value: comment.value, | ||
| start: comment.range[0], | ||
| end: comment.range[1], | ||
| }; | ||
| } else { | ||
| program.hashbang = null; | ||
| } |
There was a problem hiding this comment.
The hashbang synthesis logic is now duplicated here and in src/test262.js (with slightly different comment/input shapes). Consider extracting a small helper (e.g., normalize-first-comment-to-hashbang) to keep behavior consistent and reduce the chance of future divergence.
Add `hashbang` property to AST for test cases which include one. Oxc's AST has a non-standard `Hashbang` property. We already added `hashbang: null` to ASTs in snapshots. The PR fills in that property correctly where the file does have a hashbang. We now don't need to skip these test cases in ESTree conformance. (2nd attempt after #180)
Add
hashbangproperty to AST for test cases which include one.Oxc's AST has a non-standard
Hashbangproperty. We already addedhashbang: nullto ASTs in snapshots. The PR fills in that property correctly where the file does have a hashbang. We now don't need to skip these test cases in ESTree conformance.