Skip to content

[compiler][bugfix] Fix hoisting of let declarations #32724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 24, 2025
Merged

[compiler][bugfix] Fix hoisting of let declarations #32724

merged 1 commit into from
Mar 24, 2025

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Mar 24, 2025

(Found when compiling Meta React code)

Let variable declarations and reassignments are currently rewritten to StoreLocal <varName> instructions, which each translates to a new const varName declaration in codegen.

// Example input
function useHook() {
  const getX = () => x;
  let x = CONSTANT1;
  if (cond) {
    x += CONSTANT2;
  }
  return <Stringify getX={getX} />
}

// Compiled output, prior to this PR
import { c as _c } from "react/compiler-runtime";
function useHook() {
  const $ = _c(1);
  let t0;
  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
    const getX = () => x;
    let x = CONSTANT1;
    if (cond) {
      let x = x + CONSTANT2;
      x;
    }

    t0 = <Stringify getX={getX} />;
    $[0] = t0;
  } else {
    t0 = $[0];
  }
  return t0;
}

This also manifests as a babel internal error when replacing the original function declaration with the compiler output. The below compilation output fails with Duplicate declaration "x" (This is an error on an internal node. Probably an internal error.).

// example input
let x = CONSTANT1;
if (cond) {
  x += CONSTANT2;
  x = CONSTANT3;
}

// current output
let x = CONSTANT1;
if (playheadDragState) {
  let x = x + CONSTANT2
  x;
  let x = CONSTANT3;
}

Comment on lines +39 to +40
let bar;
bar = _temp;
Copy link
Contributor Author

@mofeiZ mofeiZ Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could rewrite these to be a StoreLocal LValue: {kind: "Let"} which would result in cleaner output. Note that we currently produce a DeclareContext followed by a StoreContext for all non-hoisted context variables.

I implemented this with a DeclareContext followed by StoreContext to better preserve instruction semantics (a StoreLocal followed by StoreContext to the same variable feels a bit nonsensical). Since this is currently the last pass before codegen, this isn't a problem at the moment.

(Found when compiling Meta React code)

Let variable declarations and reassignments are currently rewritten to `StoreLocal <varName>` instructions, which each translates to a new `const varName` declaration in codegen.

```js
// Example input
function useHook() {
  const getX = () => x;
  let x = CONSTANT1;
  if (cond) {
    x += CONSTANT2;
  }
  return <Stringify getX={getX} />
}

// Compiled output, prior to this PR
import { c as _c } from "react/compiler-runtime";
function useHook() {
  const $ = _c(1);
  let t0;
  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
    const getX = () => x;
    let x = CONSTANT1;
    if (cond) {
      let x = x + CONSTANT2;
      x;
    }

    t0 = <Stringify getX={getX} />;
    $[0] = t0;
  } else {
    t0 = $[0];
  }
  return t0;
}
```

This also manifests as a babel internal error when replacing the original function declaration with the compiler output. The below compilation output fails with `Duplicate declaration "x" (This is an error on an internal node. Probably an internal error.)`.
```js
// example input
let x = CONSTANT1;
if (cond) {
  x += CONSTANT2;
  x = CONSTANT3;
}

// current output
let x = CONSTANT1;
if (playheadDragState) {
  let x = x + CONSTANT2
  x;
  let x = CONSTANT3;
}
```
@mofeiZ
Copy link
Contributor Author

mofeiZ commented Mar 24, 2025

  • Added comment + test case to check that compilation preserves TDZ

@mofeiZ mofeiZ merged commit 254dc4d into main Mar 24, 2025
23 of 31 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 24, 2025
(Found when compiling Meta React code)

Let variable declarations and reassignments are currently rewritten to
`StoreLocal <varName>` instructions, which each translates to a new
`const varName` declaration in codegen.

```js
// Example input
function useHook() {
  const getX = () => x;
  let x = CONSTANT1;
  if (cond) {
    x += CONSTANT2;
  }
  return <Stringify getX={getX} />
}

// Compiled output, prior to this PR
import { c as _c } from "react/compiler-runtime";
function useHook() {
  const $ = _c(1);
  let t0;
  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
    const getX = () => x;
    let x = CONSTANT1;
    if (cond) {
      let x = x + CONSTANT2;
      x;
    }

    t0 = <Stringify getX={getX} />;
    $[0] = t0;
  } else {
    t0 = $[0];
  }
  return t0;
}
```

This also manifests as a babel internal error when replacing the
original function declaration with the compiler output. The below
compilation output fails with `Duplicate declaration "x" (This is an
error on an internal node. Probably an internal error.)`.
```js
// example input
let x = CONSTANT1;
if (cond) {
  x += CONSTANT2;
  x = CONSTANT3;
}

// current output
let x = CONSTANT1;
if (playheadDragState) {
  let x = x + CONSTANT2
  x;
  let x = CONSTANT3;
}
```

DiffTrain build for [254dc4d](254dc4d)
@poteto poteto deleted the pr32724 branch March 26, 2025 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants