oxc-project / oxc-project/backlog

Add separate scopes for function bodies

Open
#176 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-semantic
Dominant language
No language data
Stars
7
Forks
0
PR merge metrics
No merged PRs in 30d

Description

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?

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the linked SemanticBuilder code in crates/oxc_semantic/src/builder.rs at lines 1955-1963, then trace how the transformer resolves references against the existing ScopeTree. Compare the separate-function-body-scope and parameter-depth alternatives using the JSX and nested default-parameter examples, including the duplicate arguments case. Done means correct bindings, valid redeclaration handling, and measured scope-resolution performance.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, rust
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.