From 5bcc119d2660d62f797f8a91267f0bea7567f734 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Wed, 26 Jun 2024 16:59:21 -0700 Subject: [PATCH 1/2] [compiler] Repro for nested function local reassignment issue Summary: Additional repro demonstrating a case that still exists after #30106 [ghstack-poisoned] --- ...eassign-local-variable-in-effect.expect.md | 101 ++++++++++++++++++ ...ction-reassign-local-variable-in-effect.js | 37 +++++++ .../packages/snap/src/SproutTodoFilter.ts | 2 + 3 files changed, 140 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md new file mode 100644 index 0000000000000..391c190158603 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md @@ -0,0 +1,101 @@ + +## Input + +```javascript +import { useEffect } from "react"; +function Component() { + let local; + const mk_reassignlocal = () => { + // Create the reassignment function inside another function, then return it + const reassignLocal = (newValue) => { + local = newValue; + }; + return reassignLocal; + } + const reassignLocal = mk_reassignlocal(); + const onMount = (newValue) => { + reassignLocal("hello"); + if (local === newValue) { + // Without React Compiler, `reassignLocal` is freshly created + // on each render, capturing a binding to the latest `local`, + // such that invoking reassignLocal will reassign the same + // binding that we are observing in the if condition, and + // we reach this branch + console.log("`local` was updated!"); + } else { + // With React Compiler enabled, `reassignLocal` is only created + // once, capturing a binding to `local` in that render pass. + // Therefore, calling `reassignLocal` will reassign the wrong + // version of `local`, and not update the binding we are checking + // in the if condition. + // + // To protect against this, we disallow reassigning locals from + // functions that escape + throw new Error("`local` not updated!"); + } + }; + useEffect(() => { + onMount(); + }, [onMount]); + return "ok"; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useEffect } from "react"; +function Component() { + const $ = _c(4); + let local; + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const mk_reassignlocal = () => { + const reassignLocal = (newValue) => { + local = newValue; + }; + return reassignLocal; + }; + + t0 = mk_reassignlocal(); + $[0] = t0; + } else { + t0 = $[0]; + } + const reassignLocal_0 = t0; + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = (newValue_0) => { + reassignLocal_0("hello"); + if (local === newValue_0) { + console.log("`local` was updated!"); + } else { + throw new Error("`local` not updated!"); + } + }; + $[1] = t1; + } else { + t1 = $[1]; + } + const onMount = t1; + let t2; + let t3; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = () => { + onMount(); + }; + t3 = [onMount]; + $[2] = t2; + $[3] = t3; + } else { + t2 = $[2]; + t3 = $[3]; + } + useEffect(t2, t3); + return "ok"; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js new file mode 100644 index 0000000000000..bc3f87c1547a0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js @@ -0,0 +1,37 @@ +import { useEffect } from "react"; +function Component() { + let local; + const mk_reassignlocal = () => { + // Create the reassignment function inside another function, then return it + const reassignLocal = (newValue) => { + local = newValue; + }; + return reassignLocal; + } + const reassignLocal = mk_reassignlocal(); + const onMount = (newValue) => { + reassignLocal("hello"); + if (local === newValue) { + // Without React Compiler, `reassignLocal` is freshly created + // on each render, capturing a binding to the latest `local`, + // such that invoking reassignLocal will reassign the same + // binding that we are observing in the if condition, and + // we reach this branch + console.log("`local` was updated!"); + } else { + // With React Compiler enabled, `reassignLocal` is only created + // once, capturing a binding to `local` in that render pass. + // Therefore, calling `reassignLocal` will reassign the wrong + // version of `local`, and not update the binding we are checking + // in the if condition. + // + // To protect against this, we disallow reassigning locals from + // functions that escape + throw new Error("`local` not updated!"); + } + }; + useEffect(() => { + onMount(); + }, [onMount]); + return "ok"; +} diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 8d28ba239a368..1bc208a53ed07 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -506,6 +506,8 @@ const skipFilter = new Set([ // needs to be executed as a module "meta-property", + + "todo.invalid-nested-function-reassign-local-variable-in-effect", ]); export default skipFilter; From 0eb2cf70a6ebb020ebe2135a1472618d97fad3cb Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Wed, 26 Jun 2024 17:07:38 -0700 Subject: [PATCH 2/2] Update on "[compiler] Repro for nested function local reassignment issue" Summary: Additional repro demonstrating a case that still exists after #30106 [ghstack-poisoned] --- ...-nested-function-reassign-local-variable-in-effect.expect.md | 2 +- ...invalid-nested-function-reassign-local-variable-in-effect.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md index 391c190158603..b863dfdcdbccb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md @@ -11,7 +11,7 @@ function Component() { local = newValue; }; return reassignLocal; - } + }; const reassignLocal = mk_reassignlocal(); const onMount = (newValue) => { reassignLocal("hello"); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js index bc3f87c1547a0..e754870acce67 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js @@ -7,7 +7,7 @@ function Component() { local = newValue; }; return reassignLocal; - } + }; const reassignLocal = mk_reassignlocal(); const onMount = (newValue) => { reassignLocal("hello");