refactor(ast): pass final ScopeFlags into visit_function

[overlookmotel]

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]

• refactor(semantic): remove defunct code setting ScopeFlags twice #4286
• refactor(ast): pass final ScopeFlags into visit_function #4283 👈
• refactor(semantic): move function/class-specific code into specific visitors #4278
• main

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]

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.

[overlookmotel]

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]

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.