From 8ef22a2a90206eed7c67b5b78be39b4e542b3fff Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Wed, 22 Mar 2023 15:58:07 -0400 Subject: [PATCH] [hir] todo tests for lambda capture --- (I'm not sure if these are already known issues. I found them while playing around with lambda captures. They are also reproducible on main / stable) I have some limited understanding of lambda captures after reading Sathya's posts -- please correct if/where this is incorrect ``` function Component() { // instr1 // instr2 const func3 = function(...) { // func3instr1 } } ``` We currently determine effects of captured references in `AnalyzeFunctions`, before InferReferenceEffects. - i.e. for some function 1. dependencies of all functions (func3.deps) 2. prefix traversal of all instructions (e.g. instr1, instr2, func3.deps, func3instr1, ...) - is this just an implementation decision? i.e. what is stopping us from postfix traversal in InferReferenceEffects (e.g. instr1, instr2, func3instr1, func3.deps) As such, for each captured reference, `AnalyzeFunctions` needs to assign a reference effect. We currently check `MutableRange`, which seems to miss a few cases - We do not model assignments to primitives correctly, since primitives do not have a mutable range. - We're not able to model captured (but not mutated) values correctly. Would it be possible to consolidate `AnalyzeFunctions` into InferReferenceEffects, using some post-order traversal (iterating over a function's instructions to collect its dependencies + associated capture effects)? I definitely don't understand lambdas completely, so please tell me what I'm missing --- ...ug.lambda-capture-returned-alias.expect.md | 77 +++++++++++++++++++ .../_bug.lambda-capture-returned-alias.js | 18 +++++ ...ug.lambda-mutate-shadowed-object.expect.md | 42 ++++++++++ .../_bug.lambda-mutate-shadowed-object.js | 11 +++ .../_bug.lambda-reassign-primitive.expect.md | 39 ++++++++++ .../_bug.lambda-reassign-primitive.js | 14 ++++ ...mbda-reassign-shadowed-primitive.expect.md | 41 ++++++++++ ..._bug.lambda-reassign-shadowed-primitive.js | 11 +++ 8 files changed, 253 insertions(+) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.expect.md new file mode 100644 index 0000000000..14300c14e9 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +// Here, element should not be memoized independently of aliasedElement, since +// it is captured by fn. +// AnalyzeFunctions currently does not find captured objects. +// - mutated context refs are declared as `Capture` effect in `FunctionExpression.deps` +// - all other context refs are left as Unknown. InferReferenceEffects currently demotes +// them to reads +function CaptureNotMutate(props) { + const idx = foo(props.x); + const element = bar(props.el); + + const fn = function () { + const arr = { element }; + return arr[idx]; + }; + const aliasedElement = fn(); + mutate(aliasedElement); + return aliasedElement; +} + +``` + +## Code + +```javascript +// Here, element should not be memoized independently of aliasedElement, since +// it is captured by fn. +// AnalyzeFunctions currently does not find captured objects. +// - mutated context refs are declared as `Capture` effect in `FunctionExpression.deps` +// - all other context refs are left as Unknown. InferReferenceEffects currently demotes +// them to reads +function CaptureNotMutate(props) { + const $ = React.unstable_useMemoCache(7); + const c_0 = $[0] !== props.x; + let t0; + if (c_0) { + t0 = foo(props.x); + $[0] = props.x; + $[1] = t0; + } else { + t0 = $[1]; + } + const idx = t0; + const c_2 = $[2] !== props.el; + let t1; + if (c_2) { + t1 = bar(props.el); + $[2] = props.el; + $[3] = t1; + } else { + t1 = $[3]; + } + const element = t1; + const c_4 = $[4] !== element; + const c_5 = $[5] !== idx; + let aliasedElement; + if (c_4 || c_5) { + const fn = function () { + const arr = { element }; + return arr[idx]; + }; + aliasedElement = fn(); + mutate(aliasedElement); + $[4] = element; + $[5] = idx; + $[6] = aliasedElement; + } else { + aliasedElement = $[6]; + } + return aliasedElement; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.js b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.js new file mode 100644 index 0000000000..0bc0187f9a --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-capture-returned-alias.js @@ -0,0 +1,18 @@ +// Here, element should not be memoized independently of aliasedElement, since +// it is captured by fn. +// AnalyzeFunctions currently does not find captured objects. +// - mutated context refs are declared as `Capture` effect in `FunctionExpression.deps` +// - all other context refs are left as Unknown. InferReferenceEffects currently demotes +// them to reads +function CaptureNotMutate(props) { + const idx = foo(props.x); + const element = bar(props.el); + + const fn = function () { + const arr = { element }; + return arr[idx]; + }; + const aliasedElement = fn(); + mutate(aliasedElement); + return aliasedElement; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.expect.md new file mode 100644 index 0000000000..042e24d871 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +function Component() { + const x = {}; + { + const x = []; + const fn = function () { + mutate(x); + }; + fn(); + } + return x; // should return {} +} + +``` + +## Code + +```javascript +function Component() { + const $ = React.unstable_useMemoCache(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = {}; + $[0] = t0; + } else { + t0 = $[0]; + } + const x = t0; + + const x_0 = []; + const fn = function () { + mutate(x); + }; + fn(); + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.js b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.js new file mode 100644 index 0000000000..4971f05bd7 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-mutate-shadowed-object.js @@ -0,0 +1,11 @@ +function Component() { + const x = {}; + { + const x = []; + const fn = function () { + mutate(x); + }; + fn(); + } + return x; // should return {} +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.expect.md new file mode 100644 index 0000000000..484d364e08 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +// writing to primitives is not a 'mutate' or 'store' to context references, +// under current analysis in AnalyzeFunctions. +// $23:TFunction = Function @deps[ +// $21:TPrimitive, $22:TPrimitive]: + +function Component() { + let x = 40; + + const fn = function () { + x = x + 1; + }; + fn(); + return x; +} + +``` + +## Code + +```javascript +// writing to primitives is not a 'mutate' or 'store' to context references, +// under current analysis in AnalyzeFunctions. +// $23:TFunction = Function @deps[ +// $21:TPrimitive, $22:TPrimitive]: + +function Component() { + const fn = function () { + x = x + 1; + }; + fn(); + return 40; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.js b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.js new file mode 100644 index 0000000000..824b269bd5 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-primitive.js @@ -0,0 +1,14 @@ +// writing to primitives is not a 'mutate' or 'store' to context references, +// under current analysis in AnalyzeFunctions. +// $23:TFunction = Function @deps[ +// $21:TPrimitive, $22:TPrimitive]: + +function Component() { + let x = 40; + + const fn = function () { + x = x + 1; + }; + fn(); + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.expect.md new file mode 100644 index 0000000000..8a917699fd --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +function Component() { + const x = {}; + { + let x = 56; + const fn = function () { + x = 42; + }; + fn(); + } + return x; // should return {} +} + +``` + +## Code + +```javascript +function Component() { + const $ = React.unstable_useMemoCache(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = {}; + $[0] = t0; + } else { + t0 = $[0]; + } + const x = t0; + + const fn = function () { + x = 42; + }; + fn(); + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.js b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.js new file mode 100644 index 0000000000..66edf6b9f3 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.lambda-reassign-shadowed-primitive.js @@ -0,0 +1,11 @@ +function Component() { + const x = {}; + { + let x = 56; + const fn = function () { + x = 42; + }; + fn(); + } + return x; // should return {} +}