How it is now
Currently functions only have a single scope which contains bindings for:
- Function name (for function expressions)
- Parameters
- Declarations in body of function
In SemanticBuilder we manage to correctly resolve IdentifierReferences within function params with an extra call to resolve_references_for_current_scope after walking the function params:
https://github.com/oxc-project/oxc/blob/b4407c4e9ae4fd35637c74452eb83997f9494875/crates/oxc_semantic/src/builder.rs#L1955-L1963
i.e. We resolve references in params before any bindings from body of function are added to the scope.
The problem
In the transformer, we also need to resolve references, but we don't have the advantage that we do in SemanticBuilder of constructing ScopeTree in order. It already exists, and we have to make sense of it.
Here's an example which doesn't work at present:
import React from 'react';
function foo(elem = <Component />) {
let React;
}
In classic mode, JSX transform converts this to:
import React from 'react';
function foo(elem = React.createElement(Component, null)) {
let React;
}
We need to resolve the binding for the inserted reference React. Currently, it resolves incorrectly to let React inside the function body, as resolution goes up the scope tree and finds a binding for React in the function's scope.
Additional problem
In sloppy mode code, there's also an edge case where you can have 2 bindings for arguments in a function - oxc-project/oxc#4232.
Obvious solution
The obvious solution is to add an additional scope to all function bodies.
Advantage: This is a simple solution which has no "surprises".
Disadvantages:
- More scopes = likely perf hit when resolving references for which the binding is outside current function.
- Makes it more complicated to detect illegal redeclarations (
function foo(x) { let x; }).
Alternative solution
- Add
SymbolFlags::Parameter (or maybe existing ScopeFlags::FunctionScopedVariable will do).
- Track in
Traverse if you're inside a function's parameters or not.
- When resolving a reference, if inside a function's parameters, for the first scope encountered with
ScopeFlags::Function flag, do not match bindings unless they have a SymbolFlags::Parameter flag.
Actually, it's a bit more complex than that, in order to handle cases like this:
const x = 1;
function foo(
y = function bar(z = x) {
const x = 3;
}
) {
const x = 2;
}
The x in z = x needs to ignore both the binding in bar and the binding in foo and resolve to top level const x = 1. So rather than an "in function params" yes/no flag, would need a "function parameter depth" counter (in this case depth is 2).
Advantage: Maintain lower number of scopes = probably better perf.
Disadvantage: The complexity of reference resolution could make it error-prone.
Which?
The more complicated solution should work (though it wouldn't solve the 2 x arguments bindings edge case). But it's somewhat complex.
Personally I would side with trying the simple solution first and measure how much it impacts performance. Perhaps it's negligible. If so, great, but if not then we can look at the more complicated solution.
Any thoughts?
How it is now
Currently functions only have a single scope which contains bindings for:
In
SemanticBuilderwe manage to correctly resolveIdentifierReferences within function params with an extra call toresolve_references_for_current_scopeafter walking the function params:https://github.com/oxc-project/oxc/blob/b4407c4e9ae4fd35637c74452eb83997f9494875/crates/oxc_semantic/src/builder.rs#L1955-L1963
i.e. We resolve references in params before any bindings from body of function are added to the scope.
The problem
In the transformer, we also need to resolve references, but we don't have the advantage that we do in
SemanticBuilderof constructingScopeTreein order. It already exists, and we have to make sense of it.Here's an example which doesn't work at present:
In classic mode, JSX transform converts this to:
We need to resolve the binding for the inserted reference
React. Currently, it resolves incorrectly tolet Reactinside the function body, as resolution goes up the scope tree and finds a binding forReactin the function's scope.Additional problem
In sloppy mode code, there's also an edge case where you can have 2 bindings for
argumentsin a function - oxc-project/oxc#4232.Obvious solution
The obvious solution is to add an additional scope to all function bodies.
Advantage: This is a simple solution which has no "surprises".
Disadvantages:
function foo(x) { let x; }).Alternative solution
SymbolFlags::Parameter(or maybe existingScopeFlags::FunctionScopedVariablewill do).Traverseif you're inside a function's parameters or not.ScopeFlags::Functionflag, do not match bindings unless they have aSymbolFlags::Parameterflag.Actually, it's a bit more complex than that, in order to handle cases like this:
The
xinz = xneeds to ignore both the binding inbarand the binding infooand resolve to top levelconst x = 1. So rather than an "in function params" yes/no flag, would need a "function parameter depth" counter (in this case depth is 2).Advantage: Maintain lower number of scopes = probably better perf.
Disadvantage: The complexity of reference resolution could make it error-prone.
Which?
The more complicated solution should work (though it wouldn't solve the 2 x
argumentsbindings edge case). But it's somewhat complex.Personally I would side with trying the simple solution first and measure how much it impacts performance. Perhaps it's negligible. If so, great, but if not then we can look at the more complicated solution.
Any thoughts?