refactor(ast): pass final `ScopeFlags` into `visit_function` by overlookmotel · Pull Request #4283 · oxc-project/oxc

overlookmotel · 2024-07-15T16:35:56Z

We have a strange workaround for visit_function where we pass in ScopeFlags, to support creating the scope inside Function, but setting different flags for MethodDefinitions.

Previously visit_function took Option<ScopeFlags> and then did flags.unwrap_or(ScopeFlags::empty()) | ScopeFlags::Function to it. Personally, I found this confusing. When I was looking at MethodDefinition, I was wondering "It's a function, why doesn't it set Function flag too?"

This changes makes it more explicit and clear what ScopeFlags everything has.

overlookmotel · 2024-07-15T16:36:14Z

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

Join @overlookmotel and the rest of your teammates on Graphite

codspeed-hq · 2024-07-15T16:41:44Z

CodSpeed Performance Report

Merging #4283 will not alter performance

_{Comparing 07-15-refactor_ast_pass_final_scopeflags_into_visit_function_ (2c7bb9f) with main (bf3d8d3)}

Summary

✅ 32 untouched benchmarks

rzvxa

What do you think about this?

quote! {
    if #strict_if {
        #flags | ScopeFlags::StrictMode
    } else {
        #flags
    }
}

Instead of this?

oxc/tasks/ast_codegen/src/generators/visit.rs

Lines 495 to 501 in 91ac3f2

    
           quote! {{ 
        
               let mut flags = #flags; 
        
               if #strict_if { 
        
                   flags |= ScopeFlags::StrictMode; 
        
               } 
        
               flags 
        
           }}

We no longer have to do unwrap stuff here so it generates a more concise code.

crates/oxc_ast/src/ast/js.rs

overlookmotel · 2024-07-15T21:03:43Z

What do you think about this?

quote! {
    if #strict_if {
        #flags | ScopeFlags::StrictMode
    } else {
        #flags
    }
}

We're into micro-optimization territory here but... What we ideally want is for compiler to do this in straight line code with a cmov instruction (instead of a branch). Godbolt shows that both versions produce this when #flags is simple, but I wonder if #flags was more complicated (e.g. a function call), including #flags twice would encourage compiler towards a branch.

Quite possibly it makes no difference at all either way, but if #strict_if { #flags |= ScopeFlags::StrictMode; } is closer to what we want the compiler to produce, so on balance I think it's a safer bet.

graphite-app · 2024-07-16T07:21:37Z

Merge activity

Jul 16, 3:21 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jul 16, 3:21 AM EDT: Dunqing added this pull request to the Graphite merge queue.
Jul 16, 3:25 AM EDT: Dunqing merged this pull request with the Graphite merge queue.

We have a strange workaround for `visit_function` where we pass in `ScopeFlags`, to support creating the scope inside `Function`, but setting different flags for `MethodDefinition`s. Previously `visit_function` took `Option<ScopeFlags>` and then did `flags.unwrap_or(ScopeFlags::empty()) | ScopeFlags::Function` to it. Personally, I found this confusing. When I was looking at `MethodDefinition`, I was wondering "It's a function, why doesn't it set Function flag too?" This changes makes it more explicit and clear what `ScopeFlags` everything has.

## [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]>

## [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]>

…ope (#7932) close: #7900 After #4283 changed, we don't need to inherit `ScopeFlags` from the `constructor`, `set`, `get` anymore, I think this is a logic of forgetting to remove

overlookmotel mentioned this pull request Jul 15, 2024

refactor(semantic): move function/class-specific code into specific visitors #4278

Merged

github-actions bot added A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic A-minifier Area - Minifier A-ast Area - AST A-isolated-declarations Isolated Declarations labels Jul 15, 2024

overlookmotel marked this pull request as ready for review July 15, 2024 16:39

rzvxa suggested changes Jul 15, 2024

View reviewed changes

rzvxa reviewed Jul 15, 2024

View reviewed changes

crates/oxc_ast/src/ast/js.rs Show resolved Hide resolved

Dunqing force-pushed the 07-15-refactor_semantic_move_function_class-specific_code_into_specific_visitors branch from df6fbba to 16698bc Compare July 15, 2024 18:36

Dunqing changed the base branch from 07-15-refactor_semantic_move_function_class-specific_code_into_specific_visitors to main July 15, 2024 18:40

Dunqing force-pushed the 07-15-refactor_ast_pass_final_scopeflags_into_visit_function_ branch from 91ac3f2 to a53f300 Compare July 15, 2024 18:41

overlookmotel mentioned this pull request Jul 15, 2024

refactor(semantic): remove defunct code setting ScopeFlags twice #4286

Merged

rzvxa approved these changes Jul 15, 2024

View reviewed changes

Dunqing added the 0-merge Merge with Graphite Merge Queue label Jul 16, 2024

Dunqing force-pushed the 07-15-refactor_ast_pass_final_scopeflags_into_visit_function_ branch from a53f300 to 2c7bb9f Compare July 16, 2024 07:22

graphite-app bot merged commit 2c7bb9f into main Jul 16, 2024

graphite-app bot deleted the 07-15-refactor_ast_pass_final_scopeflags_into_visit_function_ branch July 16, 2024 07:25

This was referenced Jul 17, 2024

Release crates v0.21.0 #4325

Closed

Release oxlint v0.6.1 #4326

Merged

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

Dunqing mentioned this pull request Dec 16, 2024

fix(semantic): remove inherting ScopeFlags::Modifiers from parent scope #7932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

refactor(ast): pass final `ScopeFlags` into `visit_function`#4283

refactor(ast): pass final `ScopeFlags` into `visit_function`#4283
graphite-app[bot] merged 1 commit intomainfrom
07-15-refactor_ast_pass_final_scopeflags_into_visit_function_

overlookmotel commented Jul 15, 2024 •

edited

Loading

Uh oh!

overlookmotel commented Jul 15, 2024 •

edited

Loading

Uh oh!

codspeed-hq bot commented Jul 15, 2024 •

edited

Loading

Uh oh!

rzvxa left a comment

Uh oh!

Uh oh!

overlookmotel commented Jul 15, 2024 •

edited

Loading

Uh oh!

graphite-app bot commented Jul 16, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	quote! {{
	let mut flags = #flags;
	if #strict_if {
	flags \|= ScopeFlags::StrictMode;
	}
	flags
	}}

Uh oh!

Comments

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

CodSpeed Performance Report

Merging #4283 will not alter performance

Summary

Uh oh!

rzvxa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Merge activity

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

overlookmotel commented Jul 15, 2024 •

edited

Loading

overlookmotel commented Jul 15, 2024 •

edited

Loading

codspeed-hq bot commented Jul 15, 2024 •

edited

Loading

overlookmotel commented Jul 15, 2024 •

edited

Loading

graphite-app bot commented Jul 16, 2024 •

edited

Loading