From 2ae3592d29caf9aca2cff36e6089c1dfb92dfa4e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sun, 4 Jun 2023 11:08:54 -0700 Subject: [PATCH] Fix/review bug repros, mark fixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I reviewed the test cases we have marked as bugs ("_bug.*") and realized that several of them are already fixed — woohoo! Then one wasn't fixed _yet_: our type inference loses track of refs if you stash them inside an object/array. But that's why I added the ValidateNoRefAccessInRender pass, which i've updated to detect and reject these invalid cases. There are now only a few bugs left (more fixes coming). --- .../src/HIR/ValidateNoRefAccesInRender.ts | 10 +++ ...f-added-to-dep-without-type-info.expect.md | 61 ------------------- ...apturing-reference-changes-type.expect.md} | 0 ...js => capturing-reference-changes-type.js} | 0 ...invalid-access-ref-during-render.expect.md | 4 -- ...d-set-and-read-ref-during-render.expect.md | 2 + ...f-added-to-dep-without-type-info.expect.md | 27 ++++++++ ...use-ref-added-to-dep-without-type-info.js} | 4 +- ...md => lambda-reassign-primitive.expect.md} | 0 ...mitive.js => lambda-reassign-primitive.js} | 0 ...bda-reassign-shadowed-primitive.expect.md} | 0 ... => lambda-reassign-shadowed-primitive.js} | 0 12 files changed, 42 insertions(+), 66 deletions(-) delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.capturing-reference-changes-type.expect.md => capturing-reference-changes-type.expect.md} (100%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.capturing-reference-changes-type.js => capturing-reference-changes-type.js} (100%) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.use-ref-added-to-dep-without-type-info.js => error.invalid-use-ref-added-to-dep-without-type-info.js} (61%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.lambda-reassign-primitive.expect.md => lambda-reassign-primitive.expect.md} (100%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.lambda-reassign-primitive.js => lambda-reassign-primitive.js} (100%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.lambda-reassign-shadowed-primitive.expect.md => lambda-reassign-shadowed-primitive.expect.md} (100%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.lambda-reassign-shadowed-primitive.js => lambda-reassign-shadowed-primitive.js} (100%) diff --git a/compiler/forget/src/HIR/ValidateNoRefAccesInRender.ts b/compiler/forget/src/HIR/ValidateNoRefAccesInRender.ts index 21d84e04c5..536528c085 100644 --- a/compiler/forget/src/HIR/ValidateNoRefAccesInRender.ts +++ b/compiler/forget/src/HIR/ValidateNoRefAccesInRender.ts @@ -40,6 +40,15 @@ export function validateNoRefAccessInRender(fn: HIRFunction): void { for (const [, block] of fn.body.blocks) { for (const instr of block.instructions) { switch (instr.value.kind) { + case "PropertyLoad": + case "LoadLocal": + case "StoreLocal": + case "Destructure": { + // These instructions are necessary for storing the results of a useRef into + // a variable and referencing them in functions. We can propagate type info + // for these instructions so they ensure we have a complete analysis. + break; + } case "FunctionExpression": { // 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 @@ -57,6 +66,7 @@ export function validateNoRefAccessInRender(fn: HIRFunction): void { default: { for (const operand of eachInstructionValueOperand(instr.value)) { validateNonRefValue(error, operand); + validateNonRefObject(error, operand); } } } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.expect.md deleted file mode 100644 index 9fef9a4744..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.expect.md +++ /dev/null @@ -1,61 +0,0 @@ - -## Input - -```javascript -function Foo({ a }) { - const ref = useRef(); - // type information is lost here as we don't track types of fields - const val = { ref }; - // without type info, we don't know that val.ref.current is a ref value so we - // end up depending on val.ref.current - const x = { a, val: val.ref.current }; - - return ; -} - -``` - -## Code - -```javascript -import { unstable_useMemoCache as useMemoCache } from "react"; -function Foo(t23) { - const $ = useMemoCache(7); - const { a } = t23; - const ref = useRef(); - const c_0 = $[0] !== ref; - let t0; - if (c_0) { - t0 = { ref }; - $[0] = ref; - $[1] = t0; - } else { - t0 = $[1]; - } - const val = t0; - const c_2 = $[2] !== a; - const c_3 = $[3] !== val.ref.current; - let t1; - if (c_2 || c_3) { - t1 = { a, val: val.ref.current }; - $[2] = a; - $[3] = val.ref.current; - $[4] = t1; - } else { - t1 = $[4]; - } - const x = t1; - const c_5 = $[5] !== x; - let t2; - if (c_5) { - t2 = ; - $[5] = x; - $[6] = t2; - } else { - t2 = $[6]; - } - return t2; -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.capturing-reference-changes-type.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/capturing-reference-changes-type.expect.md similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.capturing-reference-changes-type.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/capturing-reference-changes-type.expect.md diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.capturing-reference-changes-type.js b/compiler/forget/src/__tests__/fixtures/compiler/capturing-reference-changes-type.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.capturing-reference-changes-type.js rename to compiler/forget/src/__tests__/fixtures/compiler/capturing-reference-changes-type.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md index 8a816009a2..5e2149a01c 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md @@ -15,10 +15,6 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at $20:TObject (4:4) - -[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at value$21:TObject (5:5) - [ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at $23:TObject (5:5) ``` diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md index 8cfd73d10c..48d58063c8 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md @@ -14,6 +14,8 @@ 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 (the `current` property) may not be accessed during render. Cannot access ref value at $25:TObject (4:4) ``` diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md new file mode 100644 index 0000000000..390e18137b --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md @@ -0,0 +1,27 @@ + +## Input + +```javascript +function Foo({ a }) { + const ref = useRef(); + // type information is lost here as we don't track types of fields + const val = { ref }; + // without type info, we don't know that val.ref.current is a ref value so we + // *would* end up depending on val.ref.current + // however, this is an instance of accessing a ref during render and is disallowed + // under React's rules, so we reject this input + const x = { a, val: val.ref.current }; + + return ; +} + +``` + + +## 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) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.js similarity index 61% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.js rename to compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.js index 019761d4db..bbf5facc4a 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.use-ref-added-to-dep-without-type-info.js +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.js @@ -3,7 +3,9 @@ function Foo({ a }) { // type information is lost here as we don't track types of fields const val = { ref }; // without type info, we don't know that val.ref.current is a ref value so we - // end up depending on val.ref.current + // *would* end up depending on val.ref.current + // however, this is an instance of accessing a ref during render and is disallowed + // under React's rules, so we reject this input const x = { a, val: val.ref.current }; return ; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-primitive.expect.md similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-primitive.expect.md diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.js b/compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-primitive.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.js rename to compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-primitive.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-shadowed-primitive.expect.md similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-shadowed-primitive.expect.md diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.js b/compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-shadowed-primitive.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.js rename to compiler/forget/src/__tests__/fixtures/compiler/lambda-reassign-shadowed-primitive.js