From ad32811b649985322528483596a2cbb88451b080 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 5 Jun 2023 14:08:26 -0400 Subject: [PATCH] [RFC] Improve validation against ref access in render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The goal of this PR is to improve ValidateNoRefAccessInRender to find function expressions which a) access refs and b) may be called during render. Currently we always allow ref access in any function expression, but that's obviously optimistic. For the approach, the observation is that we already have a system that tells us whether a function may get called — mutable range inference. So long as we consider a function "mutable", we'll infer a range for it, but the problem is that we don't currently view functions which depend on refs to be mutable. So here I'm doing ~~sort of~~ a hack to force function deps on refs to be treated as Effect.Capture. This is enough for the function to be considered mutable, for a mutable range to be assigned, and for us to detect that during ref validation. I don't love the hack, i'm open to other ideas! --- .../src/Entrypoint/Pipeline.ts | 7 +++--- .../src/HIR/ValidateNoRefAccesInRender.ts | 7 ++++++ .../src/Inference/AnalyseFunctions.ts | 9 +++++++ ...invalid-access-ref-during-render.expect.md | 2 +- ...ror.invalid-pass-ref-to-function.expect.md | 2 +- ...d-set-and-read-ref-during-render.expect.md | 4 ++-- ...f-added-to-dep-without-type-info.expect.md | 2 +- .../fixtures/compiler/ref-in-effect.expect.md | 6 +++-- .../fixtures/compiler/ref-in-effect.js | 3 ++- ...n-callback-invoked-during-render.expect.md | 24 +++++++++++++++++++ ...d-ref-in-callback-invoked-during-render.js | 9 +++++++ 11 files changed, 64 insertions(+), 11 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.js diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/forget/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index 7f6cf0b7ea..4a75131ce0 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/forget/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -95,9 +95,6 @@ export function* run( inferTypes(hir); yield log({ kind: "hir", name: "InferTypes", value: hir }); - if (env.validateRefAccessDuringRender) { - validateNoRefAccessInRender(hir); - } if (env.validateHooksUsage) { validateHooksUsage(hir); const conditionalHooksResult = validateUnconditionalHooks(hir).unwrap(); @@ -128,6 +125,10 @@ export function* run( inferMutableRanges(hir); yield log({ kind: "hir", name: "InferMutableRanges", value: hir }); + if (env.validateRefAccessDuringRender) { + validateNoRefAccessInRender(hir); + } + leaveSSA(hir); yield log({ kind: "hir", name: "LeaveSSA", value: hir }); diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateNoRefAccesInRender.ts b/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateNoRefAccesInRender.ts index 536528c085..180febfded 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateNoRefAccesInRender.ts +++ b/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateNoRefAccesInRender.ts @@ -53,6 +53,13 @@ export function validateNoRefAccessInRender(fn: HIRFunction): void { // For now we assume *all* function expressions are safe, eventually we can // be more precise and disallow ref access in functions that may be called // during render + const mutableRange = instr.lvalue.identifier.mutableRange; + if (mutableRange.end > mutableRange.start + 1) { + for (const operand of eachInstructionValueOperand(instr.value)) { + validateNonRefValue(error, operand); + validateNonRefObject(error, operand); + } + } break; } case "CallExpression": diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts b/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts index 0baa188655..32653b71ad 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts +++ b/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts @@ -11,6 +11,8 @@ import { FunctionExpression, HIRFunction, Identifier, + isRefValueType, + isUseRefType, mergeConsecutiveBlocks, Place, ReactiveScopeDependency, @@ -123,6 +125,13 @@ function infer( if (name !== null && mutations.has(name)) { dep.effect = Effect.Capture; + } else if (isUseRefType(dep.identifier) || isRefValueType(dep.identifier)) { + // TODO: this is a hack to ensure we treat functions which reference refs + // as having a capture and therefore being considered mutable. this ensures + // the function gets a mutable range which accounts for anywhere that it + // could be called, and allows us to help ensure it isn't called during + // render + dep.effect = Effect.Capture; } } diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md index 5e2149a01c..436fc08db3 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md @@ -15,7 +15,7 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at $23:TObject (5:5) +[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at freeze $23:TObject (5:5) ``` \ No newline at end of file diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md index f90ca6a5e6..54cb17b425 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md @@ -14,7 +14,7 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at $22:TObject (3:3) +[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at mutate $22[6:8]:TObject (3:3) ``` \ No newline at end of file diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md index 48d58063c8..0f97f4e8ea 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md @@ -14,9 +14,9 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at $22:TObject (3:3) +[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at store $22[7:9]:TObject (3:3) -[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at $25:TObject (4:4) +[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at freeze $25:TObject (4:4) ``` \ No newline at end of file diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md index 390e18137b..3a4da0a087 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md @@ -21,7 +21,7 @@ function Foo({ a }) { ## Error ``` -[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at $30:TObject (4:4) +[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at capture $30:TObject (4:4) ``` \ No newline at end of file diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.expect.md index 1eb8fb418b..6af6facbed 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.expect.md @@ -5,7 +5,8 @@ function Component(props) { const ref = useRef(null); const onChange = (e) => { - ref.current = e.target.value; + const newValue = e.target.value ?? ref.current; + ref.current = newValue; }; useEffect(() => { console.log(ref.current); @@ -25,7 +26,8 @@ function Component(props) { let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = (e) => { - ref.current = e.target.value; + const newValue = e.target.value ?? ref.current; + ref.current = newValue; }; $[0] = t0; } else { diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.js index b26e06c529..00b960cabe 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.js +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ref-in-effect.js @@ -1,7 +1,8 @@ function Component(props) { const ref = useRef(null); const onChange = (e) => { - ref.current = e.target.value; + const newValue = e.target.value ?? ref.current; + ref.current = newValue; }; useEffect(() => { console.log(ref.current); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md new file mode 100644 index 0000000000..d2989287b7 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md @@ -0,0 +1,24 @@ + +## Input + +```javascript +// @debug +function Component(props) { + const ref = useRef(null); + const renderItem = (item) => { + const current = ref.current; + return ; + }; + return {props.items.map((item) => renderItem(item))}; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at capture $43[6:16]:TObject (5:5) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.js new file mode 100644 index 0000000000..f400b30286 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.js @@ -0,0 +1,9 @@ +// @debug +function Component(props) { + const ref = useRef(null); + const renderItem = (item) => { + const current = ref.current; + return ; + }; + return {props.items.map((item) => renderItem(item))}; +}