fix(semantic): do not rely on spans for node comparison in Function::bind#18296
Conversation
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
6c841fa to
463fab0
Compare
2f58b37 to
d0f8dc7
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug in the semantic binder where node identity comparison was using spans instead of memory addresses. The issue arises when transformed or generated AST nodes have zeroed spans (0, 0), making span-based comparison unreliable for determining node uniqueness.
Changes:
- Replaced span-based node comparison with address-based comparison in
Function::bind - Removed unused
GetSpanimport
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d0f8dc7 to
7853545
Compare
463fab0 to
a4d92d3
Compare
7853545 to
22b4d39
Compare
a4d92d3 to
01e8de9
Compare
01e8de9 to
95c9636
Compare
Merge activity
|
|
All are nice improvements! And it's not easy to find them if you don't dig into it! |
95c9636 to
2b813a5
Compare
2b813a5 to
cd3530d
Compare
…:bind` (#18296) Binder for `Function` was using spans to check for node equality. This is fine when the AST has come directly from the parser as spans are guaranteed to be unique, but it is not reliable when the AST has gone through transformation. If the code has been generated, it might have zeroed spans (`SPAN`), in which case span equality does not guarantee uniqueness. e.g. if this code is generated: ```js const obj = { [function f() {}]: function g() {}, }; ``` `f` and `g` both have the same span (0, 0). Switch to using `Address` for comparison, which doesn't have this problem.
cd3530d to
324dd7a
Compare
…:bind` (#18296) Binder for `Function` was using spans to check for node equality. This is fine when the AST has come directly from the parser as spans are guaranteed to be unique, but it is not reliable when the AST has gone through transformation. If the code has been generated, it might have zeroed spans (`SPAN`), in which case span equality does not guarantee uniqueness. e.g. if this code is generated: ```js const obj = { [function f() {}]: function g() {}, }; ``` `f` and `g` both have the same span (0, 0). Switch to using `Address` for comparison, which doesn't have this problem.
324dd7a to
9a15c6a
Compare
### 💥 BREAKING CHANGES - 22dec6a semantic: [**BREAKING**] Remove `Scoping::scope_build_child_ids` and all related APIs (#18362) (Dunqing) - 30a4899 oxc: [**BREAKING**] Remove `CompilerInterface::semantic_child_scope_ids` (#18361) (Dunqing) - 777fc40 ast: [**BREAKING**] Add `Ident` type (#18354) (Boshen) - af0ca46 span: [**BREAKING**] Use `ModuleKind::CommonJS` for `SourceType::cjs()` (#18276) (sapphi-red) ### 🚀 Features - 0a02026 semantic: Add TS1499 code to diagnostic (#18557) (camc314) - 8b4618f parser: Add TS1500 code to diagnostic (#18547) (camc314) - 866b6b3 parser: Add TS1048 code to diagnostic (#18546) (camc314) - 1117c44 parser: Add TS1054 code to diagnostic (#18541) (camc314) - e4fcdde semantic: Add TS1053 code to diagnostic (#18539) (camc314) - bcbf396 semantic: Add TS1052 code to diagnostic (#18538) (camc314) - 8155edf semantic: Add TS1049 code to diagnostic (#18535) (camc314) - 51d3b3f parser: Add TS1502 code to diagnostic (#18534) (camc314) - 00854e8 semantic: Add TS2337 error code to super call diagnostic (#18531) (camc314) - 993fd2b parser: Parse unambiguous await with better error messages (#18480) (Boshen) - 8db0e78 linter/plugins: Handle BOMs (#18376) (overlookmotel) - 6ac09e2 linter/plugins: Support source text not being at start of buffer (#18375) (overlookmotel) - 2ef5647 ast: Add escape_raw parameter to template_element builders (#18121) (Boshen) ### 🐛 Bug Fixes - 74d0998 semantic: Update error msg for multiple `default` cases in switch stmt (#18526) (camc314) - c205b0d ast: Remove `ThisExpression` from `TSModuleReference` (#18489) (Boshen) - aed3669 parser: Parse HTML-like comments in unambiguous mode (#18442) (Boshen) - c4132fb parser: Validate accessor parameters in interface method signatures (#18391) (Boshen) - b0cd74d semantic: Allow `var` and `function` with same name in static blocks (#18358) (Boshen) - 6037995 semantic: Allow `new.target` in class field initializers (#18349) (Boshen) - 9a15c6a semantic: Do not rely on spans for node comparison in `Function::bind` (#18296) (overlookmotel) ### ⚡ Performance - 6b600c4 semantic: Skip parent lookup for function declarations in `Function::bind` (#18293) (overlookmotel) - c27ad2d semantic: Move check for function declaration out of `is_function_part_of_if_statement` (#18292) (overlookmotel) - 63eb89e semantic: Skip checking redeclarations for function expressions (#18291) (overlookmotel) - 7c12743 semantic: Skip checking unresolved exports in CommonJS files (#18250) (overlookmotel) - 2349031 allocator: Increase initial chunk size from 512B to 16KB (#18234) (Boshen) ### 📚 Documentation - 8ccd853 npm: Update package homepage URLs and add keywords (#18509) (Boshen) - 9b3165f napi/parser: Clarify when to use `parseAsync` vs `parseSync` (#18486) (Boshen) - 1b59f63 napi/parser: Correct typo in README (#18251) (overlookmotel) - 00ff75f mangler: Fix `top_level` option in example (#18233) (overlookmotel) - 2ddc073 semantic: Fix typo in comment (#18238) (overlookmotel) Co-authored-by: Boshen <[email protected]>

Binder for
Functionwas using spans to check for node equality.This is fine when the AST has come directly from the parser as spans are guaranteed to be unique, but it is not reliable when the AST has gone through transformation. If the code has been generated, it might have zeroed spans (
SPAN), in which case span equality does not guarantee uniqueness.e.g. if this code is generated:
fandgboth have the same span (0, 0).Switch to using
Addressfor comparison, which doesn't have this problem.