-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
[compiler][fixtures] test repros: codegen, alignScope, phis #29878
Changes from all commits
2827cbc
bec3735
98231bc
c322f7e
e7772bf
eb7e076
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
import { mutate } from "shared-runtime"; | ||
|
||
/** | ||
* Fixture showing why `concat` needs to capture both the callee and rest args. | ||
* Here, observe that arr1's values are captured into arr2. | ||
* - Later mutations of arr2 may write to values within arr1. | ||
* - Observe that it's technically valid to separately memoize the array arr1 | ||
* itself. | ||
*/ | ||
function Foo({ inputNum }) { | ||
const arr1: Array<number | object> = [{ a: 1 }, {}]; | ||
const arr2 = arr1.concat([1, inputNum]); | ||
mutate(arr2[0]); | ||
return arr2; | ||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
fn: Foo, | ||
params: [{ inputNum: 2 }], | ||
sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }], | ||
}; | ||
|
||
``` | ||
|
||
## Code | ||
|
||
```javascript | ||
import { c as _c } from "react/compiler-runtime"; | ||
import { mutate } from "shared-runtime"; | ||
|
||
/** | ||
* Fixture showing why `concat` needs to capture both the callee and rest args. | ||
* Here, observe that arr1's values are captured into arr2. | ||
* - Later mutations of arr2 may write to values within arr1. | ||
* - Observe that it's technically valid to separately memoize the array arr1 | ||
* itself. | ||
*/ | ||
function Foo(t0) { | ||
const $ = _c(3); | ||
const { inputNum } = t0; | ||
let t1; | ||
if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
t1 = [{ a: 1 }, {}]; | ||
$[0] = t1; | ||
} else { | ||
t1 = $[0]; | ||
} | ||
const arr1 = t1; | ||
let arr2; | ||
if ($[1] !== inputNum) { | ||
arr2 = arr1.concat([1, inputNum]); | ||
mutate(arr2[0]); | ||
$[1] = inputNum; | ||
$[2] = arr2; | ||
} else { | ||
arr2 = $[2]; | ||
} | ||
return arr2; | ||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
fn: Foo, | ||
params: [{ inputNum: 2 }], | ||
sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }], | ||
}; | ||
|
||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { mutate } from "shared-runtime"; | ||
|
||
/** | ||
* Fixture showing why `concat` needs to capture both the callee and rest args. | ||
* Here, observe that arr1's values are captured into arr2. | ||
* - Later mutations of arr2 may write to values within arr1. | ||
* - Observe that it's technically valid to separately memoize the array arr1 | ||
* itself. | ||
*/ | ||
function Foo({ inputNum }) { | ||
const arr1: Array<number | object> = [{ a: 1 }, {}]; | ||
const arr2 = arr1.concat([1, inputNum]); | ||
mutate(arr2[0]); | ||
return arr2; | ||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
fn: Foo, | ||
params: [{ inputNum: 2 }], | ||
sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
import { makeArray, print } from "shared-runtime"; | ||
|
||
/** | ||
* Exposes bug involving iife inlining + codegen. | ||
* We currently inline iifes to labeled blocks (not value-blocks). | ||
* | ||
* Here, print(1) and the evaluation of makeArray(...) get the same scope | ||
* as the compiler infers that the makeArray call may mutate its arguments. | ||
* Since print(1) does not get its own scope (and is thus not a declaration | ||
* or dependency), it does not get promoted. | ||
* As a result, print(1) gets reordered across the labeled-block instructions | ||
* to be inlined at the makeArray callsite. | ||
* | ||
* Current evaluator results: | ||
* Found differences in evaluator results | ||
* Non-forget (expected): | ||
* (kind: ok) [null,2] | ||
* logs: [1,2] | ||
* Forget: | ||
* (kind: ok) [null,2] | ||
* logs: [2,1] | ||
*/ | ||
function useTest() { | ||
return makeArray<number | void>( | ||
print(1), | ||
(function foo() { | ||
print(2); | ||
return 2; | ||
})() | ||
); | ||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
fn: useTest, | ||
params: [], | ||
}; | ||
|
||
``` | ||
|
||
## Code | ||
|
||
```javascript | ||
import { c as _c } from "react/compiler-runtime"; | ||
import { makeArray, print } from "shared-runtime"; | ||
|
||
/** | ||
* Exposes bug involving iife inlining + codegen. | ||
* We currently inline iifes to labeled blocks (not value-blocks). | ||
* | ||
* Here, print(1) and the evaluation of makeArray(...) get the same scope | ||
* as the compiler infers that the makeArray call may mutate its arguments. | ||
* Since print(1) does not get its own scope (and is thus not a declaration | ||
* or dependency), it does not get promoted. | ||
* As a result, print(1) gets reordered across the labeled-block instructions | ||
* to be inlined at the makeArray callsite. | ||
* | ||
* Current evaluator results: | ||
* Found differences in evaluator results | ||
* Non-forget (expected): | ||
* (kind: ok) [null,2] | ||
* logs: [1,2] | ||
* Forget: | ||
* (kind: ok) [null,2] | ||
* logs: [2,1] | ||
*/ | ||
function useTest() { | ||
const $ = _c(1); | ||
let t0; | ||
if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
let t1; | ||
|
||
print(2); | ||
t1 = 2; | ||
t0 = makeArray(print(1), t1); | ||
$[0] = t0; | ||
} else { | ||
t0 = $[0]; | ||
} | ||
return t0; | ||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
fn: useTest, | ||
params: [], | ||
}; | ||
|
||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import { makeArray, print } from "shared-runtime"; | ||
|
||
/** | ||
* Exposes bug involving iife inlining + codegen. | ||
* We currently inline iifes to labeled blocks (not value-blocks). | ||
* | ||
* Here, print(1) and the evaluation of makeArray(...) get the same scope | ||
* as the compiler infers that the makeArray call may mutate its arguments. | ||
* Since print(1) does not get its own scope (and is thus not a declaration | ||
* or dependency), it does not get promoted. | ||
* As a result, print(1) gets reordered across the labeled-block instructions | ||
* to be inlined at the makeArray callsite. | ||
* | ||
* Current evaluator results: | ||
* Found differences in evaluator results | ||
* Non-forget (expected): | ||
* (kind: ok) [null,2] | ||
* logs: [1,2] | ||
* Forget: | ||
* (kind: ok) [null,2] | ||
* logs: [2,1] | ||
*/ | ||
function useTest() { | ||
return makeArray<number | void>( | ||
print(1), | ||
(function foo() { | ||
print(2); | ||
return 2; | ||
})() | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @josephsavona @mvitousek Curious as to your thoughts on this issue. My intuition (could definitely be far off the mark) is that we can benefit from assertions in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice find!!!! Per discussion: i think what we need to do here is add a compiler pass that forces temporaries to be used wherever necessary to retain instruction ordering. We're basically looking for any instruction whose lvalue is first referenced after some intervening lvalue-less instruction. This could go right after we prune unused lvalues. |
||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
fn: useTest, | ||
params: [], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
/** | ||
* Similar fixture to `error.todo-align-scopes-nested-block-structure`, but | ||
* a simpler case. | ||
*/ | ||
function useFoo(cond) { | ||
let s = null; | ||
if (cond) { | ||
s = {}; | ||
} else { | ||
return null; | ||
} | ||
mutate(s); | ||
return s; | ||
} | ||
|
||
``` | ||
|
||
|
||
## Error | ||
|
||
``` | ||
Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:13) | ||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/** | ||
* Similar fixture to `error.todo-align-scopes-nested-block-structure`, but | ||
* a simpler case. | ||
*/ | ||
function useFoo(cond) { | ||
let s = null; | ||
if (cond) { | ||
s = {}; | ||
} else { | ||
return null; | ||
} | ||
mutate(s); | ||
return s; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
/** | ||
* Fixture showing that it's not sufficient to only align direct scoped | ||
* accesses of a block-fallthrough pair. | ||
* Below is a simplified view of HIR blocks in this fixture. | ||
* Note that here, s is mutated in both bb1 and bb4. However, neither | ||
* bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves. | ||
* | ||
* This means that we need to recursively visit all scopes accessed between | ||
* a block and its fallthrough and extend the range of those scopes which overlap | ||
* with an active block/fallthrough pair, | ||
* | ||
* bb0 | ||
* ┌──────────────┐ | ||
* │let s = null │ | ||
* │test cond1 │ | ||
* │ <fallthr=bb3>│ | ||
* └┬─────────────┘ | ||
* │ bb1 | ||
* ├─►┌───────┐ | ||
* │ │s = {} ├────┐ | ||
* │ └───────┘ │ | ||
* │ bb2 │ | ||
* └─►┌───────┐ │ | ||
* │return;│ │ | ||
* └───────┘ │ | ||
* bb3 │ | ||
* ┌──────────────┐◄┘ | ||
* │test cond2 │ | ||
* │ <fallthr=bb5>│ | ||
* └┬─────────────┘ | ||
* │ bb4 | ||
* ├─►┌─────────┐ | ||
* │ │mutate(s)├─┐ | ||
* ▼ └─────────┘ │ | ||
* bb5 │ | ||
* ┌───────────┐ │ | ||
* │return s; │◄──┘ | ||
* └───────────┘ | ||
*/ | ||
function useFoo(cond1, cond2) { | ||
let s = null; | ||
if (cond1) { | ||
s = {}; | ||
} else { | ||
return null; | ||
} | ||
|
||
if (cond2) { | ||
mutate(s); | ||
} | ||
|
||
return s; | ||
} | ||
|
||
``` | ||
|
||
|
||
## Error | ||
|
||
``` | ||
Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:15) | ||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/** | ||
* Fixture showing that it's not sufficient to only align direct scoped | ||
* accesses of a block-fallthrough pair. | ||
* Below is a simplified view of HIR blocks in this fixture. | ||
* Note that here, s is mutated in both bb1 and bb4. However, neither | ||
* bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves. | ||
* | ||
* This means that we need to recursively visit all scopes accessed between | ||
* a block and its fallthrough and extend the range of those scopes which overlap | ||
* with an active block/fallthrough pair, | ||
* | ||
* bb0 | ||
* ┌──────────────┐ | ||
* │let s = null │ | ||
* │test cond1 │ | ||
* │ <fallthr=bb3>│ | ||
* └┬─────────────┘ | ||
* │ bb1 | ||
* ├─►┌───────┐ | ||
* │ │s = {} ├────┐ | ||
* │ └───────┘ │ | ||
* │ bb2 │ | ||
* └─►┌───────┐ │ | ||
* │return;│ │ | ||
* └───────┘ │ | ||
* bb3 │ | ||
* ┌──────────────┐◄┘ | ||
* │test cond2 │ | ||
* │ <fallthr=bb5>│ | ||
* └┬─────────────┘ | ||
* │ bb4 | ||
* ├─►┌─────────┐ | ||
* │ │mutate(s)├─┐ | ||
* ▼ └─────────┘ │ | ||
* bb5 │ | ||
* ┌───────────┐ │ | ||
* │return s; │◄──┘ | ||
* └───────────┘ | ||
*/ | ||
function useFoo(cond1, cond2) { | ||
let s = null; | ||
if (cond1) { | ||
s = {}; | ||
} else { | ||
return null; | ||
} | ||
|
||
if (cond2) { | ||
mutate(s); | ||
} | ||
|
||
return s; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! This should be trivial to fix, our definition for concat() just needs to be more pessimistic:
react/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts
Lines 229 to 238 in 2ba462b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, already in my local bugfix stack!