-
Notifications
You must be signed in to change notification settings - Fork 47.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[compiler][hir-rewrite] Check mutability of base identifier when hois…
…ting (#31032) Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * #31066 * __->__ #31032 Prior to this PR, we check whether the property load source (e.g. the evaluation of `<base>` in `<base>.property`) is mutable + scoped to determine whether the property load itself is eligible for hoisting. This changes to check the base identifier of the load. - This is needed for the next PR #31066. We want to evaluate whether the base identifier is mutable within the context of the *outermost function*. This is because all LoadLocals and PropertyLoads within a nested function declaration have mutable-ranges within the context of the function, but the base identifier is a context variable. - A side effect is that we no longer infer loads from props / other function arguments as mutable in edge cases (e.g. props escaping out of try-blocks or being assigned to context variables)
- Loading branch information
Showing
8 changed files
with
250 additions
and
71 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
65 changes: 65 additions & 0 deletions
65
...r/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.expect.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| import {identity} from 'shared-runtime'; | ||
|
|
||
| /** | ||
| * Not safe to hoist read of maybeNullObject.value.inner outside of the | ||
| * try-catch block, as that might throw | ||
| */ | ||
| function useFoo(maybeNullObject: {value: {inner: number}} | null) { | ||
| const y = []; | ||
| try { | ||
| y.push(identity(maybeNullObject.value.inner)); | ||
| } catch { | ||
| y.push('null'); | ||
| } | ||
|
|
||
| return y; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: useFoo, | ||
| params: [null], | ||
| sequentialRenders: [null, {value: 2}, {value: 3}, null], | ||
| }; | ||
|
|
||
| ``` | ||
|
|
||
| ## Code | ||
|
|
||
| ```javascript | ||
| import { c as _c } from "react/compiler-runtime"; | ||
| import { identity } from "shared-runtime"; | ||
|
|
||
| /** | ||
| * Not safe to hoist read of maybeNullObject.value.inner outside of the | ||
| * try-catch block, as that might throw | ||
| */ | ||
| function useFoo(maybeNullObject) { | ||
| const $ = _c(2); | ||
| let y; | ||
| if ($[0] !== maybeNullObject.value.inner) { | ||
| y = []; | ||
| try { | ||
| y.push(identity(maybeNullObject.value.inner)); | ||
| } catch { | ||
| y.push("null"); | ||
| } | ||
| $[0] = maybeNullObject.value.inner; | ||
| $[1] = y; | ||
| } else { | ||
| y = $[1]; | ||
| } | ||
| return y; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: useFoo, | ||
| params: [null], | ||
| sequentialRenders: [null, { value: 2 }, { value: 3 }, null], | ||
| }; | ||
|
|
||
| ``` | ||
22 changes: 22 additions & 0 deletions
22
...gin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import {identity} from 'shared-runtime'; | ||
|
|
||
| /** | ||
| * Not safe to hoist read of maybeNullObject.value.inner outside of the | ||
| * try-catch block, as that might throw | ||
| */ | ||
| function useFoo(maybeNullObject: {value: {inner: number}} | null) { | ||
| const y = []; | ||
| try { | ||
| y.push(identity(maybeNullObject.value.inner)); | ||
| } catch { | ||
| y.push('null'); | ||
| } | ||
|
|
||
| return y; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: useFoo, | ||
| params: [null], | ||
| sequentialRenders: [null, {value: 2}, {value: 3}, null], | ||
| }; |
79 changes: 79 additions & 0 deletions
79
...ompiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.expect.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| // @enablePropagateDepsInHIR | ||
| import {identity} from 'shared-runtime'; | ||
|
|
||
| /** | ||
| * Not safe to hoist read of maybeNullObject.value.inner outside of the | ||
| * try-catch block, as that might throw | ||
| */ | ||
| function useFoo(maybeNullObject: {value: {inner: number}} | null) { | ||
| const y = []; | ||
| try { | ||
| y.push(identity(maybeNullObject.value.inner)); | ||
| } catch { | ||
| y.push('null'); | ||
| } | ||
|
|
||
| return y; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: useFoo, | ||
| params: [null], | ||
| sequentialRenders: [null, {value: 2}, {value: 3}, null], | ||
| }; | ||
|
|
||
| ``` | ||
|
|
||
| ## Code | ||
|
|
||
| ```javascript | ||
| import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR | ||
| import { identity } from "shared-runtime"; | ||
|
|
||
| /** | ||
| * Not safe to hoist read of maybeNullObject.value.inner outside of the | ||
| * try-catch block, as that might throw | ||
| */ | ||
| function useFoo(maybeNullObject) { | ||
| const $ = _c(4); | ||
| let y; | ||
| if ($[0] !== maybeNullObject) { | ||
| y = []; | ||
| try { | ||
| let t0; | ||
| if ($[2] !== maybeNullObject.value.inner) { | ||
| t0 = identity(maybeNullObject.value.inner); | ||
| $[2] = maybeNullObject.value.inner; | ||
| $[3] = t0; | ||
| } else { | ||
| t0 = $[3]; | ||
| } | ||
| y.push(t0); | ||
| } catch { | ||
| y.push("null"); | ||
| } | ||
| $[0] = maybeNullObject; | ||
| $[1] = y; | ||
| } else { | ||
| y = $[1]; | ||
| } | ||
| return y; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: useFoo, | ||
| params: [null], | ||
| sequentialRenders: [null, { value: 2 }, { value: 3 }, null], | ||
| }; | ||
|
|
||
| ``` | ||
| ### Eval output | ||
| (kind: ok) ["null"] | ||
| [null] | ||
| [null] | ||
| ["null"] |
23 changes: 23 additions & 0 deletions
23
...ests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // @enablePropagateDepsInHIR | ||
| import {identity} from 'shared-runtime'; | ||
|
|
||
| /** | ||
| * Not safe to hoist read of maybeNullObject.value.inner outside of the | ||
| * try-catch block, as that might throw | ||
| */ | ||
| function useFoo(maybeNullObject: {value: {inner: number}} | null) { | ||
| const y = []; | ||
| try { | ||
| y.push(identity(maybeNullObject.value.inner)); | ||
| } catch { | ||
| y.push('null'); | ||
| } | ||
|
|
||
| return y; | ||
| } | ||
|
|
||
| export const FIXTURE_ENTRYPOINT = { | ||
| fn: useFoo, | ||
| params: [null], | ||
| sequentialRenders: [null, {value: 2}, {value: 3}, null], | ||
| }; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters