From 63ceab6f8dcb2aac96dfd986d8108f510914fc78 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 5 Nov 2024 18:49:40 -0500 Subject: [PATCH] [compiler] Lower JSXMemberExpression with LoadLocal `JSXMemberExpression` is currently the only instruction (that I know of) that directly references identifier lvalues without a corresponding `LoadLocal`. This has some side effects: - deadcode elimination and constant propagation now reach JSXMemberExpressions - we can delete `LoweredFunction.dependencies` without dangling references (previously, the only reference to JSXMemberExpression objects in HIR was in function dependencies) - JSXMemberExpression now is consistent with all other instructions (e.g. has a rvalue-producing LoadLocal) ' --- .../src/HIR/BuildHIR.ts | 8 +- .../invalid-jsx-lowercase-localvar.expect.md | 75 +++++++++++++++++++ .../invalid-jsx-lowercase-localvar.jsx | 29 +++++++ ...local-memberexpr-tag-conditional.expect.md | 3 +- .../jsx-local-memberexpr-tag.expect.md | 3 +- ...se-localvar-memberexpr-in-lambda.expect.md | 59 +++++++++++++++ ...owercase-localvar-memberexpr-in-lambda.jsx | 12 +++ ...sx-lowercase-localvar-memberexpr.expect.md | 45 +++++++++++ .../jsx-lowercase-localvar-memberexpr.jsx | 10 +++ .../jsx-lowercase-memberexpr.expect.md | 44 +++++++++++ .../compiler/jsx-lowercase-memberexpr.jsx | 9 +++ .../jsx-memberexpr-tag-in-lambda.expect.md | 3 +- .../packages/snap/src/SproutTodoFilter.ts | 3 + .../snap/src/sprout/shared-runtime.ts | 3 + 14 files changed, 299 insertions(+), 7 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.jsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.jsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.jsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.jsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index 9494436d1f463..ecc22365dd03c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -3186,7 +3186,13 @@ function lowerJsxMemberExpression( loc: object.node.loc ?? null, suggestions: null, }); - objectPlace = lowerIdentifier(builder, object); + + const kind = getLoadKind(builder, object); + objectPlace = lowerValueToTemporary(builder, { + kind: kind, + place: lowerIdentifier(builder, object), + loc: exprPath.node.loc ?? GeneratedSource, + }); } const property = exprPath.get('property').node.name; return lowerValueToTemporary(builder, { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.expect.md new file mode 100644 index 0000000000000..925346225c3e8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.expect.md @@ -0,0 +1,75 @@ + +## Input + +```javascript +import {Throw} from 'shared-runtime'; + +/** + * Note: this is disabled in the evaluator due to different devmode errors. + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * logs: ['Warning: <%s /> is using incorrect casing. Use PascalCase for React components, or lowercase for HTML elements.%s','invalidTag'] + * + * Forget: + * (kind: ok) + * logs: [ + * 'Warning: <%s /> is using incorrect casing. Use PascalCase for React components, or lowercase for HTML elements.%s','invalidTag', + * 'Warning: The tag <%s> is unrecognized in this browser. If you meant to render a React component, start its name with an uppercase letter.%s','invalidTag', + * ] + */ +function useFoo() { + const invalidTag = Throw; + /** + * Need to be careful to not parse `invalidTag` as a localVar (i.e. render + * Throw). Note that the jsx transform turns this into a string tag: + * `jsx("invalidTag"... + */ + return ; +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { Throw } from "shared-runtime"; + +/** + * Note: this is disabled in the evaluator due to different devmode errors. + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * logs: ['Warning: <%s /> is using incorrect casing. Use PascalCase for React components, or lowercase for HTML elements.%s','invalidTag'] + * + * Forget: + * (kind: ok) + * logs: [ + * 'Warning: <%s /> is using incorrect casing. Use PascalCase for React components, or lowercase for HTML elements.%s','invalidTag', + * 'Warning: The tag <%s> is unrecognized in this browser. If you meant to render a React component, start its name with an uppercase letter.%s','invalidTag', + * ] + */ +function useFoo() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = ; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.jsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.jsx new file mode 100644 index 0000000000000..1e62eb011765e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-jsx-lowercase-localvar.jsx @@ -0,0 +1,29 @@ +import {Throw} from 'shared-runtime'; + +/** + * Note: this is disabled in the evaluator due to different devmode errors. + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * logs: ['Warning: <%s /> is using incorrect casing. Use PascalCase for React components, or lowercase for HTML elements.%s','invalidTag'] + * + * Forget: + * (kind: ok) + * logs: [ + * 'Warning: <%s /> is using incorrect casing. Use PascalCase for React components, or lowercase for HTML elements.%s','invalidTag', + * 'Warning: The tag <%s> is unrecognized in this browser. If you meant to render a React component, start its name with an uppercase letter.%s','invalidTag', + * ] + */ +function useFoo() { + const invalidTag = Throw; + /** + * Need to be careful to not parse `invalidTag` as a localVar (i.e. render + * Throw). Note that the jsx transform turns this into a string tag: + * `jsx("invalidTag"... + */ + return ; +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag-conditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag-conditional.expect.md index 0cb821459cd03..f13d3a05981d4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag-conditional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag-conditional.expect.md @@ -27,11 +27,10 @@ import * as SharedRuntime from "shared-runtime"; function useFoo(t0) { const $ = _c(1); const { cond } = t0; - const MyLocal = SharedRuntime; if (cond) { let t1; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t1 = ; + t1 = ; $[0] = t1; } else { t1 = $[0]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag.expect.md index ab11ddedb8489..f24e7a754dbe7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-memberexpr-tag.expect.md @@ -22,10 +22,9 @@ import { c as _c } from "react/compiler-runtime"; import * as SharedRuntime from "shared-runtime"; function useFoo() { const $ = _c(1); - const MyLocal = SharedRuntime; let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = ; + t0 = ; $[0] = t0; } else { t0 = $[0]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.expect.md new file mode 100644 index 0000000000000..248234793931e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +import * as SharedRuntime from 'shared-runtime'; +import {invoke} from 'shared-runtime'; +function useComponentFactory({name}) { + const localVar = SharedRuntime; + const cb = () => hello world {name}; + return invoke(cb); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useComponentFactory, + params: [{name: 'sathya'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import * as SharedRuntime from "shared-runtime"; +import { invoke } from "shared-runtime"; +function useComponentFactory(t0) { + const $ = _c(4); + const { name } = t0; + let t1; + if ($[0] !== name) { + t1 = () => ( + hello world {name} + ); + $[0] = name; + $[1] = t1; + } else { + t1 = $[1]; + } + const cb = t1; + let t2; + if ($[2] !== cb) { + t2 = invoke(cb); + $[2] = cb; + $[3] = t2; + } else { + t2 = $[3]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useComponentFactory, + params: [{ name: "sathya" }], +}; + +``` + +### Eval output +(kind: ok)
{"children":["hello world ","sathya"]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.jsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.jsx new file mode 100644 index 0000000000000..534490d5d4ef5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr-in-lambda.jsx @@ -0,0 +1,12 @@ +import * as SharedRuntime from 'shared-runtime'; +import {invoke} from 'shared-runtime'; +function useComponentFactory({name}) { + const localVar = SharedRuntime; + const cb = () => hello world {name}; + return invoke(cb); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useComponentFactory, + params: [{name: 'sathya'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.expect.md new file mode 100644 index 0000000000000..5778bf599f88f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.expect.md @@ -0,0 +1,45 @@ + +## Input + +```javascript +import * as SharedRuntime from 'shared-runtime'; +function Component({name}) { + const localVar = SharedRuntime; + return hello world {name}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{name: 'sathya'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import * as SharedRuntime from "shared-runtime"; +function Component(t0) { + const $ = _c(2); + const { name } = t0; + let t1; + if ($[0] !== name) { + t1 = hello world {name}; + $[0] = name; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ name: "sathya" }], +}; + +``` + +### Eval output +(kind: ok)
{"children":["hello world ","sathya"]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.jsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.jsx new file mode 100644 index 0000000000000..d55037fca039e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-localvar-memberexpr.jsx @@ -0,0 +1,10 @@ +import * as SharedRuntime from 'shared-runtime'; +function Component({name}) { + const localVar = SharedRuntime; + return hello world {name}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{name: 'sathya'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.expect.md new file mode 100644 index 0000000000000..f5f7b3727e99c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +import * as SharedRuntime from 'shared-runtime'; +function Component({name}) { + return hello world {name}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{name: 'sathya'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import * as SharedRuntime from "shared-runtime"; +function Component(t0) { + const $ = _c(2); + const { name } = t0; + let t1; + if ($[0] !== name) { + t1 = hello world {name}; + $[0] = name; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ name: "sathya" }], +}; + +``` + +### Eval output +(kind: ok)
{"children":["hello world ","sathya"]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.jsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.jsx new file mode 100644 index 0000000000000..992cbecebe38e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-lowercase-memberexpr.jsx @@ -0,0 +1,9 @@ +import * as SharedRuntime from 'shared-runtime'; +function Component({name}) { + return hello world {name}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{name: 'sathya'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md index 363f82d12c6a6..22fa3b2e2a2e3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md @@ -25,10 +25,9 @@ import { c as _c } from "react/compiler-runtime"; import * as SharedRuntime from "shared-runtime"; function useFoo() { const $ = _c(1); - const MyLocal = SharedRuntime; let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const callback = () => ; + const callback = () => ; t0 = callback(); $[0] = t0; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index b868afc52b82d..4eb53a17ae065 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -475,6 +475,9 @@ const skipFilter = new Set([ 'rules-of-hooks/rules-of-hooks-93dc5d5e538a', 'rules-of-hooks/rules-of-hooks-69521d94fa03', + // false positives + 'invalid-jsx-lowercase-localvar', + // bugs 'fbt/bug-fbt-plural-multiple-function-calls', 'fbt/bug-fbt-plural-multiple-mixed-call-tag', diff --git a/compiler/packages/snap/src/sprout/shared-runtime.ts b/compiler/packages/snap/src/sprout/shared-runtime.ts index 0f3e09b12e127..e6e82d6b77e05 100644 --- a/compiler/packages/snap/src/sprout/shared-runtime.ts +++ b/compiler/packages/snap/src/sprout/shared-runtime.ts @@ -252,6 +252,9 @@ export function Stringify(props: any): React.ReactElement { toJSON(props, props?.shouldInvokeFns), ); } +export function Throw() { + throw new Error(); +} export function ValidateMemoization({ inputs,