refactor(semantic): correct scope in CatchClause #4192

Dunqing · 2024-07-11T05:52:45Z

CatchClause has two scopes. The first one is CatchClause, which will add a CatchParameter to it. The second one is Block, which will add binding that declares in the current block scope.

The spec has a syntax error about CatchParameter

It is a Syntax Error if any element of the BoundNames of CatchParameter also occurs in the LexicallyDeclaredNames of Block.

graphite-app · 2024-07-11T05:52:50Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Dunqing · 2024-07-11T05:53:02Z

feat(semantic): align class scope with typescript #4195 : 2 dependent PRs (#4141 , #4225 )
refactor(semantic): set current_scope_id to scope_id in enter_scope #4193
refactor(semantic): correct scope in CatchClause #4192 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite

codspeed-hq · 2024-07-11T05:58:04Z

CodSpeed Performance Report

Merging #4192 will not alter performance

_{Comparing 07-11-refactor_semantic_correct_scope_in_catchclause (7f1addd) with main (aa22073)}

Summary

✅ 31 untouched benchmarks

crates/oxc_semantic/src/builder.rs

graphite-app · 2024-07-11T08:42:05Z

Merge activity

Jul 11, 4:42 AM EDT: Boshen added this pull request to the Graphite merge queue.
Jul 11, 4:52 AM EDT: Boshen merged this pull request with the Graphite merge queue.

close: #4186 CatchClause has two scopes. The first one is `CatchClause`, which will add a `CatchParameter` to it. The second one is `Block`, which will add binding that declares in the current block scope. The spec has a syntax error about `CatchParameter` - It is a Syntax Error if any element of the BoundNames of CatchParameter also occurs in the LexicallyDeclaredNames of Block.

overlookmotel · 2024-07-11T09:21:56Z

crates/oxc_ast/src/ast/js.rs

 }

 #[visited_node]
-#[scope(if(self.param.is_some()))]


@Dunqing What was reason for removing if(self.param.is_some())? If no catch param, we don't need a scope, right?

I think it's arguable that it would be simpler if we didn't have any conditional scopes, but if we do that, probably we should do it consistently.

Yes, just to be consistent with the spec

OK, so is plan to make all scopes unconditional?

No plan. Maybe when you start improving semantics. We could do some offset spec changes for performance.

Personally, I don't think we need to adhere to the spec. The spec defines what observable behavior must be, but implementations are free to achieve that through any means they choose. V8, for example, diverges from the example algorithms in the spec very heavily for purpose of better performance.

I agree. I have found that there is a performance impact of boa reporting errors according to the specification

https://github.com/boa-dev/boa/blob/5b1621abd2b702b99bd6b4491973095f9b27c362/core/parser/src/parser/statement/try_stm/catch.rs#L92-L97

Thanks for coming back. OK. I would suggest reverting this to #[scope(if(self.param.is_some()))] for now then. If we keep scopes as conditional, it's quite easy to make them all unconditional later. But it's harder to go in other direction - you have to go through all the scopes and reason about which could be conditional.

Personally I think the most important thing either way is to be consistent, as it's one less oddity to reason about.

But that's just my opinion. You can disagree, and I won't argue further.

Ok. I will send a PR later

## [0.6.1] - 2024-07-17 ### Features - 83c2c62 codegen: Add option for choosing quotes; remove slow `choose_quot` method (#4219) (Boshen) - 1f8968a linter: Add eslint-plugin-promise rules: avoid-new, no-new-statics, params-names (#4293) (Jelle van der Waa) - a4dc56c linter: Add fixer for unicorn/no_useless_promise_resolve_reject (#4244) (Burlin) - 6fb808f linter: Add typescript-eslint/no-confusing-non-null-assertion (#4224) (Jaden Rodriguez) - 126b66c linter: Support eslint-plugin-vitest/valid-describe-callback (#4185) (cinchen) - 05b9a73 linter: Support eslint-plugin-vitest/valid-expect (#4183) (cinchen) - 3e56b2b linter: Support eslint-plugin-vitest/no-test-prefixes (#4182) (cinchen) - 3016f03 linter: Let fixer functions return a `None` fix (#4210) (DonIsaac) - bbe6137 linter: Implement unicorn/no-useless-undefined (#4079) (Burlin) - 20cdb1f semantic: Align class scope with typescript (#4195) (Dunqing) ### Bug Fixes - 9df60da linter: Correct find first non whitespace logic in @typescript-eslint/consistent-type-imports (#4198) (mysteryven) - 67240dc linter: Not ignore adjacent spans when fixing (#4217) (mysteryven) - dd07a54 linter: Global variables should always check the builtin variables (#4209) (Jelle van der Waa) - 351ecf2 semantic: Incorrect resolve references for `TSTypeQuery` (#4310) (Dunqing) - 1108f2a semantic: Resolve references to the incorrect symbol (#4280) (Dunqing) ### Performance - 0fdc88b linter: Optimize no-dupe-keys (#4292) (lucab) ### Refactor - 2c7bb9f ast: Pass final `ScopeFlags` into `visit_function` (#4283) (overlookmotel) - aa22073 codegen: Improve print API (#4196) (Boshen) - b5a8f3c linter: Use get_first_parameter_name from unicorn utils (#4255) (Jelle van der Waa) - 7089a3d linter: Split up fixer code into separate files (#4222) (DonIsaac) - ace4f1f semantic: Update the order of `visit_function` and `Visit` fields in the builder to be consistent (#4248) (Dunqing) - 7f1addd semantic: Correct scope in CatchClause (#4192) (Dunqing) Co-authored-by: Boshen <[email protected]>

…hClause` scope (#4337) related: #4192 #4323 I will figure it out #4323 later

## [0.21.0] - 2024-07-18 - d7ab0b8 semantic: [**BREAKING**] Simplify node creation (#4226) (lucab) ### Features - af4dc01 ast: Align ts ast scope with typescript (#4253) (Dunqing) - 83c2c62 codegen: Add option for choosing quotes; remove slow `choose_quot` method (#4219) (Boshen) - 5d17675 mangler: Add debug mode (#4314) (Boshen) - e3e663b mangler: Initialize crate and integrate into minifier (#4197) (Boshen) - c818472 minifier: Dce conditional expression `&&` or `||` (#4190) (Boshen) - 8a190eb oxc: Export `oxc_mangler` (Boshen) - 20cdb1f semantic: Align class scope with typescript (#4195) (Dunqing) - 92ee774 semantic: Add `ScopeFlags::CatchClause` for use in CatchClause (#4205) (Dunqing) - 205c259 sourcemap: Support SourceMapBuilder#token_chunks (#4220) (underfin) - 7eb960d transformer: Decode xml character entity `&#xhhhh` and `&#nnnn;` (#4235) (Boshen) ### Bug Fixes - bf3d8d3 codegen: Print annotation comment inside parens for new and call expressions (#4290) (Boshen) - 084ab76 codegen: Use `ryu-js` for f64 to string (Boshen) - e167ef7 codegen: Print parenthesis properly (#4245) (Boshen) - c65198f codegen: Choose the right quote for jsx attribute string (#4236) (Boshen) - be82c28 codegen: Print `JSXAttributeValue::StringLiteral` directly (#4231) (Boshen) - 3df9e69 mangler: No shorthand `BindingProperty`; handle var hoisting and export variables (#4319) (Boshen) - f144082 minifier: RemoveDeadCode should visit nested expression (#4268) (underfin) - 66b455a oxc_codegen: Avoid print same pure comments multiple time (#4230) (IWANABETHATGUY) - 9a87e41 parser: Avoid crashing on invalid const modifier (#4267) (lucab) - 641a78b parser: Fix tests for number parsing (#4254) (overlookmotel) - 9badac0 semantic: Avoid var hosting insert the var variable to the `CatchClause` scope (#4337) (Dunqing) - 95e15b6 semantic: Incorrect resolve references for `ExportSpecifier` (#4320) (Dunqing) - c362bf7 semantic: Incorrect resolve references for `TSInterfaceHeritage` (#4311) (Dunqing) - 351ecf2 semantic: Incorrect resolve references for `TSTypeQuery` (#4310) (Dunqing) - 1108f2a semantic: Resolve references to the incorrect symbol (#4280) (Dunqing) - 22d56bd semantic: Do not resolve references after `FormalParameters` in TS type (#4241) (overlookmotel)- 1c117eb Avoid print extra semicolon after accessor property (#4199) (IWANABETHATGUY) ### Performance - a8dc4f3 parser: Speed up parsing numbers with `_` separators (#4259) (overlookmotel) - b94540d parser: Speed up parsing octal literals (#4258) (overlookmotel) - a7b328c parser: Faster parsing decimal numbers (#4257) (overlookmotel) - f9d3f2e semantic: Inline ast record functions (#4272) (overlookmotel) - 8fad7db semantic: Reduce `AstNodeId` to `u32` (#4264) (overlookmotel) - 23743db semantic: Do not record ast nodes for cfg if cfg disabled (#4263) (overlookmotel) - da69076 semantic: Reduce overhead of cfg recording ast nodes (#4262) (overlookmotel) - cb15303 semantic: Reduce memory copies (#4216) (overlookmotel) - ef4c1f4 semantic: Reduce lookups (#4214) (overlookmotel) - f23e54f semantic: Recycle unresolved references hash maps (#4213) (overlookmotel) - 2602ce2 semantic: Reuse existing map of unresolved refs (#4206) (lucab) ### Refactor - 2c7bb9f ast: Pass final `ScopeFlags` into `visit_function` (#4283) (overlookmotel) - 3e099fe ast: Move `enter_scope` after `visit_binding_identifier` (#4246) (Dunqing) - aab7aaa ast/visit: Fire node events as the outermost one. (#4203) (rzvxa) - d1c4be0 codegen: Clean up annotation_comment (Boshen) - 06197b8 codegen: Separate tests (Boshen) - aa22073 codegen: Improve print API (#4196) (Boshen) - c5731a5 semantic: Remove defunct code setting ScopeFlags twice (#4286) (overlookmotel) - 16698bc semantic: Move function/class-specific code into specific visitors (#4278) (overlookmotel) - ee16668 semantic: Rename function param (#4277) (overlookmotel) - 25f0771 semantic: Alter syntax of `control_flow!` macro (#4275) (overlookmotel) - 639fd48 semantic: Comment why extra CFG enabled check (#4274) (overlookmotel) - c418bf5 semantic: Directly record `current_node_id` when adding a scope (#4265) (Dunqing) - ace4f1f semantic: Update the order of `visit_function` and `Visit` fields in the builder to be consistent (#4248) (Dunqing) - 8bfeabf semantic: Simplify adding `SymbolFlags::Export` (#4249) (Dunqing) - dc2b3c4 semantic: Add strict mode in scope flags for class definitions (#4156) (Dunqing) - 81ed588 semantic: Convert scope fields to IndexVecs (#4208) (lucab) - bbe5ded semantic: Set `current_scope_id` to `scope_id` in `enter_scope` (#4193) (Dunqing) - 7f1addd semantic: Correct scope in CatchClause (#4192) (Dunqing) - fc0b17d syntax: Turn the `AstNodeId::dummy` into a constant field. (#4308) (rzvxa) - a197e01 transformer/typescript: Remove unnecessary code (#4321) (Dunqing) - 1458d81 visit: Add `#[inline]` to empty functions (#4330) (overlookmotel) Co-authored-by: Boshen <[email protected]>

github-actions bot added A-semantic Area - Semantic A-ast Area - AST labels Jul 11, 2024

github-actions bot added the A-linter Area - Linter label Jul 11, 2024

Boshen reviewed Jul 11, 2024

View reviewed changes

crates/oxc_semantic/src/builder.rs Outdated Show resolved Hide resolved

Dunqing mentioned this pull request Jul 11, 2024

refactor(semantic): set current_scope_id to scope_id in enter_scope #4193

Merged

Dunqing force-pushed the 07-11-refactor_semantic_correct_scope_in_catchclause branch 3 times, most recently from 3ed4990 to ca61d3d Compare July 11, 2024 08:00

Boshen added the 0-merge Merge with Graphite Merge Queue label Jul 11, 2024 — with Graphite App

Boshen force-pushed the 07-11-refactor_semantic_correct_scope_in_catchclause branch from ca61d3d to 7f1addd Compare July 11, 2024 08:45

graphite-app bot merged commit 7f1addd into main Jul 11, 2024

graphite-app bot deleted the 07-11-refactor_semantic_correct_scope_in_catchclause branch July 11, 2024 08:52

overlookmotel reviewed Jul 11, 2024

View reviewed changes

This was referenced Jul 12, 2024

refactor: use scope way #4225

Closed

Record AstNodeId for all scopes in Semantic #4444

Closed

This was referenced Jul 17, 2024

Release crates v0.21.0 #4325

Closed

Release oxlint v0.6.1 #4326

Merged

Dunqing mentioned this pull request Jul 18, 2024

fix(semantic): avoid var hosting insert the var variable to the CatchClause scope #4337

Merged

Dunqing added a commit that referenced this pull request Jul 18, 2024

fix(semantic): avoid var hosting insert the var variable to the `Catc…

9badac0

…hClause` scope (#4337) related: #4192 #4323 I will figure it out #4323 later

github-actions bot mentioned this pull request Jul 18, 2024

Release crates v0.21.0 #4338

Merged

github-actions bot mentioned this pull request Jul 18, 2024

Release crates v0.21.0 #4344

Merged

overlookmotel mentioned this pull request Jul 18, 2024

Make CatchClause scope optional #4345

Closed

Uh oh!

refactor(semantic): correct scope in CatchClause #4192

refactor(semantic): correct scope in CatchClause #4192

Uh oh!

Conversation

Dunqing commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot commented Jul 11, 2024

Your org has enabled the Graphite merge queue for merging into main

Uh oh!

Dunqing commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #4192 will not alter performance

Summary

Uh oh!

Uh oh!

graphite-app bot commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dunqing commented Jul 11, 2024 •

edited

Loading

Dunqing commented Jul 11, 2024 •

edited

Loading

codspeed-hq bot commented Jul 11, 2024 •

edited

Loading

graphite-app bot commented Jul 11, 2024 •

edited

Loading