fix(semantic): update symbol_declarations when a function is found after a var

[camc314]

Fixes #11215

I'm really not happy with this fix, but I can't see a better solution.

Since we only do 1x pass in semantic we can't hoist the function declaraions + declare them before the var declarations, hence here, we have to update the original declaration to point to the function declaration.

[camc314]

• fix(semantic): update symbol_declarations when a function is found after a var #11234 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #11234 will not alter performance

_{Comparing c/05-22-fix_semantic_update_symbol_declarations_when_a_function_is_found_after_a_var (1bff337) with main (07e6ae0)}

Summary

✅ 38 untouched benchmarks

[Boshen]

@Dunqing will take a look at how tsc works.

[Dunqing]

Wait... I just found out the current implementation is correct, there is no error in JavaScript. See Oxc Playground.

The current implementation has covered this case.

oxc/crates/oxc_semantic/src/binder.rs

Lines 182 to 189 in 1846b03

	let excludes = if builder.source_type.is_typescript() {
	SymbolFlags::FunctionExcludes
	} else {
	// `var x; function x() {}` is valid in non-strict mode, but `TypeScript`
	// doesn't care about non-strict mode, so we need to exclude this,
	// and further check in checker.
	SymbolFlags::FunctionExcludes - SymbolFlags::FunctionScopedVariable
	};

[camc314]

yes, the current implementation is correct however the symbol_declarations node id points to the var declaration (if it comes first) rather than the function declaration (which is should point to since the FN decl was hoisted first).

I wonder wether we should just move the debug assertion 🤔

[Dunqing]

yes, the current implementation is correct however the symbol_declarations node id points to the var declaration (if it comes first) rather than the function declaration (which is should point to since the FN decl was hoisted first).

I wonder wether we should just move the debug assertion 🤔

Thank you for explaining the problem more. I finally figure out what happened here. Currently, the implementation not exactly handle variable hoisting in the same way as the specification, and I don't think it is worth dealing with it just for this case.

In this case, I think we can use Scoping::symbol_redeclarations to solve this problem. If the current symbol has multiple declarations, you can find each declaration node ID from it.

oxc/crates/oxc_semantic/src/scoping.rs

Lines 18 to 23 in 1846b03

	#[derive(Debug)]
	pub struct Redeclaration {
	pub span: Span,
	pub declaration: NodeId,
	pub flags: SymbolFlags,
	}

[camc314]

ok, so we will leave semantic as is with this bug and fix in the lint rule.

are there any other cases other than function and var in the same scope that we need to consider?

e.g. here it has scope flags function, but doens't point to a function, do we need to consider that for any other types

[Dunqing]

are there any other cases other than function and var in the same scope that we need to consider?

e.g. here it has scope flags function, but doens't point to a function, do we need to consider that for any other types

As far as I know, no other cases we need to care about. I've sent a fix in #11280 and copied over a test from this PR. Thank you for the help!