Skip to content

Add separate scopes for function bodies #176

@overlookmotel

Description

@overlookmotel

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?

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-semanticArea - Semantic Analysis

    Type

    No type

    Priority

    None yet

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions