diff --git a/.github/workflows/compiler_playground.yml b/.github/workflows/compiler_playground.yml index 34349f584ef26..a19e87e25e78e 100644 --- a/.github/workflows/compiler_playground.yml +++ b/.github/workflows/compiler_playground.yml @@ -57,8 +57,6 @@ jobs: key: playwright-browsers-v6-${{ runner.arch }}-${{ runner.os }}-${{ steps.playwright_version.outputs.playwright_version }} - run: npx playwright install --with-deps chromium if: steps.cache_playwright_browsers.outputs.cache-hit != 'true' - - run: npx playwright install-deps - if: steps.cache_playwright_browsers.outputs.cache-hit == 'true' - run: CI=true yarn test - run: ls -R test-results if: '!cancelled()' diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index f0dbc069f082c..d9fb47da3b204 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -316,7 +316,7 @@ jobs: if: steps.node_modules.outputs.cache-hit != 'true' - run: ./scripts/react-compiler/build-compiler.sh && ./scripts/react-compiler/link-compiler.sh - run: yarn workspace eslint-plugin-react-hooks test - + # ----- BUILD ----- build_and_lint: name: yarn build and lint @@ -811,9 +811,18 @@ jobs: pattern: _build_* path: build merge-multiple: true - - run: | - npx playwright install - sudo npx playwright install-deps + - name: Check Playwright version + id: playwright_version + run: echo "playwright_version=$(npm ls @playwright/test | grep @playwright | sed 's/.*@//' | head -1)" >> "$GITHUB_OUTPUT" + - name: Cache Playwright Browsers for version ${{ steps.playwright_version.outputs.playwright_version }} + id: cache_playwright_browsers + uses: actions/cache@v4 + with: + path: ~/.cache/ms-playwright + key: playwright-browsers-v6-${{ runner.arch }}-${{ runner.os }}-${{ steps.playwright_version.outputs.playwright_version }} + - name: Playwright install deps + if: steps.cache_playwright_browsers.outputs.cache-hit != 'true' + run: npx playwright install --with-deps chromium - run: ./scripts/ci/run_devtools_e2e_tests.js env: RELEASE_CHANNEL: experimental diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index 964217c399317..f3007034c3b7d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts @@ -541,6 +541,9 @@ export enum ErrorCategory { // Checking for valid usage of manual memoization UseMemo = 'UseMemo', + // Checking for higher order functions acting as factories for components/hooks + Factories = 'Factories', + // Checks that manual memoization is preserved PreserveManualMemo = 'PreserveManualMemo', @@ -703,6 +706,16 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule { recommended: true, }; } + case ErrorCategory.Factories: { + return { + category, + name: 'component-hook-factories', + description: + 'Validates against higher order functions defining nested components or hooks. ' + + 'Components and hooks should be defined at the module level', + recommended: true, + }; + } case ErrorCategory.FBT: { return { category, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index 92151501622bf..5a9ef9495fa1d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -494,7 +494,20 @@ function findFunctionsToCompile( ): Array { const queue: Array = []; const traverseFunction = (fn: BabelFn, pass: CompilerPass): void => { + // In 'all' mode, compile only top level functions + if ( + pass.opts.compilationMode === 'all' && + fn.scope.getProgramParent() !== fn.scope.parent + ) { + return; + } + const fnType = getReactFunctionType(fn, pass); + + if (pass.opts.environment.validateNoDynamicallyCreatedComponentsOrHooks) { + validateNoDynamicallyCreatedComponentsOrHooks(fn, pass, programContext); + } + if (fnType === null || programContext.alreadyCompiled.has(fn.node)) { return; } @@ -839,6 +852,73 @@ function shouldSkipCompilation( return false; } +/** + * Validates that Components/Hooks are always defined at module level. This prevents scope reference + * errors that occur when the compiler attempts to optimize the nested component/hook while its + * parent function remains uncompiled. + */ +function validateNoDynamicallyCreatedComponentsOrHooks( + fn: BabelFn, + pass: CompilerPass, + programContext: ProgramContext, +): void { + const parentNameExpr = getFunctionName(fn); + const parentName = + parentNameExpr !== null && parentNameExpr.isIdentifier() + ? parentNameExpr.node.name + : ''; + + const validateNestedFunction = ( + nestedFn: NodePath< + t.FunctionDeclaration | t.FunctionExpression | t.ArrowFunctionExpression + >, + ): void => { + if ( + nestedFn.node === fn.node || + programContext.alreadyCompiled.has(nestedFn.node) + ) { + return; + } + + if (nestedFn.scope.getProgramParent() !== nestedFn.scope.parent) { + const nestedFnType = getReactFunctionType(nestedFn as BabelFn, pass); + const nestedFnNameExpr = getFunctionName(nestedFn as BabelFn); + const nestedName = + nestedFnNameExpr !== null && nestedFnNameExpr.isIdentifier() + ? nestedFnNameExpr.node.name + : ''; + if (nestedFnType === 'Component' || nestedFnType === 'Hook') { + CompilerError.throwDiagnostic({ + category: ErrorCategory.Factories, + severity: ErrorSeverity.InvalidReact, + reason: `Components and hooks cannot be created dynamically`, + description: `The function \`${nestedName}\` appears to be a React ${nestedFnType.toLowerCase()}, but it's defined inside \`${parentName}\`. Components and Hooks should always be declared at module scope`, + details: [ + { + kind: 'error', + message: 'this function dynamically created a component/hook', + loc: parentNameExpr?.node.loc ?? fn.node.loc ?? null, + }, + { + kind: 'error', + message: 'the component is created here', + loc: nestedFnNameExpr?.node.loc ?? nestedFn.node.loc ?? null, + }, + ], + }); + } + } + + nestedFn.skip(); + }; + + fn.traverse({ + FunctionDeclaration: validateNestedFunction, + FunctionExpression: validateNestedFunction, + ArrowFunctionExpression: validateNestedFunction, + }); +} + function getReactFunctionType( fn: BabelFn, pass: CompilerPass, @@ -877,11 +957,6 @@ function getReactFunctionType( return componentSyntaxType; } case 'all': { - // Compile only top level functions - if (fn.scope.getProgramParent() !== fn.scope.parent) { - return null; - } - return getComponentOrHookLike(fn, hookPattern) ?? 'Other'; } default: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index fd68830f929dd..d85c6b19c6738 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -650,6 +650,13 @@ export const EnvironmentConfigSchema = z.object({ * useMemo(() => { ... }, [...]); */ validateNoVoidUseMemo: z.boolean().default(false), + + /** + * Validates that Components/Hooks are always defined at module level. This prevents scope + * reference errors that occur when the compiler attempts to optimize the nested component/hook + * while its parent function remains uncompiled. + */ + validateNoDynamicallyCreatedComponentsOrHooks: z.boolean().default(false), }); export type EnvironmentConfig = z.infer; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 7d2b0b234c7f4..c673ac53d9b10 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -880,7 +880,8 @@ export function printType(type: Type): string { if (type.kind === 'Object' && type.shapeId != null) { return `:T${type.kind}<${type.shapeId}>`; } else if (type.kind === 'Function' && type.shapeId != null) { - return `:T${type.kind}<${type.shapeId}>`; + const returnType = printType(type.return); + return `:T${type.kind}<${type.shapeId}>()${returnType !== '' ? `: ${returnType}` : ''}`; } else { return `:T${type.kind}`; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index 36f32213eab79..8dd79409ce151 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -27,6 +27,7 @@ import { InstructionKind, InstructionValue, isArrayType, + isJsxType, isMapType, isPrimitiveType, isRefOrRefValue, @@ -1871,6 +1872,23 @@ function computeSignatureForInstruction( }); } } + for (const prop of value.props) { + if ( + prop.kind === 'JsxAttribute' && + prop.place.identifier.type.kind === 'Function' && + (isJsxType(prop.place.identifier.type.return) || + (prop.place.identifier.type.return.kind === 'Phi' && + prop.place.identifier.type.return.operands.some(operand => + isJsxType(operand), + ))) + ) { + // Any props which return jsx are assumed to be called during render + effects.push({ + kind: 'Render', + place: prop.place, + }); + } + } } break; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 488d988b9715d..d3a297e2e51c2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -777,6 +777,15 @@ class Unifier { return {kind: 'Phi', operands: type.operands.map(o => this.get(o))}; } + if (type.kind === 'Function') { + return { + kind: 'Function', + isConstructor: type.isConstructor, + shapeId: type.shapeId, + return: this.get(type.return), + }; + } + return type; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.expect.md new file mode 100644 index 0000000000000..3e3dcab818682 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +function Component() { + const renderItem = item => { + // Multiple returns so that the return type is a Phi (union) + if (item == null) { + return null; + } + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a variable defined outside a component or hook is not allowed. Consider using an effect. + +error.invalid-mutate-global-in-render-helper-phi-return-prop.ts:12:4 + 10 | // called during render, and thus it's unsafe to mutate globals or call + 11 | // other impure code. +> 12 | global.property = true; + | ^^^^^^ value cannot be modified + 13 | return ; + 14 | }; + 15 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.js new file mode 100644 index 0000000000000..b6ca0a04aeea8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-phi-return-prop.js @@ -0,0 +1,16 @@ +function Component() { + const renderItem = item => { + // Multiple returns so that the return type is a Phi (union) + if (item == null) { + return null; + } + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md new file mode 100644 index 0000000000000..9e995d5124d4e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md @@ -0,0 +1,40 @@ + +## Input + +```javascript +function Component() { + const renderItem = item => { + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a variable defined outside a component or hook is not allowed. Consider using an effect. + +error.invalid-mutate-global-in-render-helper-prop.ts:8:4 + 6 | // called during render, and thus it's unsafe to mutate globals or call + 7 | // other impure code. +> 8 | global.property = true; + | ^^^^^^ value cannot be modified + 9 | return ; + 10 | }; + 11 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js new file mode 100644 index 0000000000000..9355c482fb61a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js @@ -0,0 +1,12 @@ +function Component() { + const renderItem = item => { + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-component-in-normal-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-component-in-normal-function.expect.md new file mode 100644 index 0000000000000..14d86ffe825a5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-component-in-normal-function.expect.md @@ -0,0 +1,54 @@ + +## Input + +```javascript +// @validateNoDynamicallyCreatedComponentsOrHooks +export function getInput(a) { + const Wrapper = () => { + const handleChange = () => { + a.onChange(); + }; + + return ; + }; + + return Wrapper; +} + +export const FIXTURE_ENTRYPOINT = { + fn: getInput, + isComponent: false, + params: [{onChange() {}}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: Components and hooks cannot be created dynamically + +The function `Wrapper` appears to be a React component, but it's defined inside `getInput`. Components and Hooks should always be declared at module scope + +error.nested-component-in-normal-function.ts:2:16 + 1 | // @validateNoDynamicallyCreatedComponentsOrHooks +> 2 | export function getInput(a) { + | ^^^^^^^^ this function dynamically created a component/hook + 3 | const Wrapper = () => { + 4 | const handleChange = () => { + 5 | a.onChange(); + +error.nested-component-in-normal-function.ts:3:8 + 1 | // @validateNoDynamicallyCreatedComponentsOrHooks + 2 | export function getInput(a) { +> 3 | const Wrapper = () => { + | ^^^^^^^ the component is created here + 4 | const handleChange = () => { + 5 | a.onChange(); + 6 | }; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-component-in-normal-function.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-component-in-normal-function.js new file mode 100644 index 0000000000000..36be05e3ee8bc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-component-in-normal-function.js @@ -0,0 +1,18 @@ +// @validateNoDynamicallyCreatedComponentsOrHooks +export function getInput(a) { + const Wrapper = () => { + const handleChange = () => { + a.onChange(); + }; + + return ; + }; + + return Wrapper; +} + +export const FIXTURE_ENTRYPOINT = { + fn: getInput, + isComponent: false, + params: [{onChange() {}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-hook-in-normal-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-hook-in-normal-function.expect.md new file mode 100644 index 0000000000000..0ab2b977561ee --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-hook-in-normal-function.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +// @validateNoDynamicallyCreatedComponentsOrHooks +import {useState} from 'react'; + +function createCustomHook(config) { + function useConfiguredState() { + const [state, setState] = useState(0); + + const increment = () => { + setState(state + config.step); + }; + + return [state, increment]; + } + + return useConfiguredState; +} + +export const FIXTURE_ENTRYPOINT = { + fn: createCustomHook, + isComponent: false, + params: [{step: 1}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: Components and hooks cannot be created dynamically + +The function `useConfiguredState` appears to be a React hook, but it's defined inside `createCustomHook`. Components and Hooks should always be declared at module scope + +error.nested-hook-in-normal-function.ts:4:9 + 2 | import {useState} from 'react'; + 3 | +> 4 | function createCustomHook(config) { + | ^^^^^^^^^^^^^^^^ this function dynamically created a component/hook + 5 | function useConfiguredState() { + 6 | const [state, setState] = useState(0); + 7 | + +error.nested-hook-in-normal-function.ts:5:11 + 3 | + 4 | function createCustomHook(config) { +> 5 | function useConfiguredState() { + | ^^^^^^^^^^^^^^^^^^ the component is created here + 6 | const [state, setState] = useState(0); + 7 | + 8 | const increment = () => { +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-hook-in-normal-function.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-hook-in-normal-function.js new file mode 100644 index 0000000000000..306e78f7b1c69 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.nested-hook-in-normal-function.js @@ -0,0 +1,22 @@ +// @validateNoDynamicallyCreatedComponentsOrHooks +import {useState} from 'react'; + +function createCustomHook(config) { + function useConfiguredState() { + const [state, setState] = useState(0); + + const increment = () => { + setState(state + config.step); + }; + + return [state, increment]; + } + + return useConfiguredState; +} + +export const FIXTURE_ENTRYPOINT = { + fn: createCustomHook, + isComponent: false, + params: [{step: 1}], +}; diff --git a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTab.css b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTab.css index dc82b25da2178..921ce0cff2a7b 100644 --- a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTab.css +++ b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTab.css @@ -15,15 +15,15 @@ } .TreeWrapper { + border-top: 1px solid var(--color-border); flex: 1 1 var(--horizontal-resize-tree-percentage); display: flex; flex-direction: row; - overflow: auto; - border-top: 1px solid var(--color-border); + height: 100%; } .InspectedElementWrapper { - flex: 1 1 calc(100% - var(--horizontal-resize-tree-percentage)); + flex: 1 1 35%; overflow-x: hidden; overflow-y: auto; } @@ -52,8 +52,6 @@ display: none; } - - @container devtools (width < 600px) { .SuspenseTab { flex-direction: column; @@ -61,7 +59,8 @@ .TreeWrapper { border-top: 1px solid var(--color-border); - flex: 1 0 var(--vertical-resize-tree-percentage); + flex: 1 1 var(--vertical-resize-tree-percentage); + overflow: hidden; } .InspectedElementWrapper { @@ -79,6 +78,7 @@ .TreeView footer { display: flex; justify-content: end; + border-top: 1px solid var(--color-border); } .ToggleInspectedElement[data-orientation="horizontal"] { @@ -89,21 +89,23 @@ .TreeList { flex: 0 0 var(--horizontal-resize-tree-list-percentage); border-right: 1px solid var(--color-border); - padding: 0.25rem + padding: 0.25rem; + overflow: auto; } .TreeView { flex: 1 1 35%; display: flex; flex-direction: column; + height: 100%; + overflow: auto; } - - .Rects { border-top: 1px solid var(--color-border); padding: 0.25rem; flex-grow: 1; + overflow: auto; } .TimelineWrapper {