Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .github/workflows/compiler_playground.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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()'
Expand Down
17 changes: 13 additions & 4 deletions .github/workflows/runtime_build_and_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',

Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,20 @@ function findFunctionsToCompile(
): Array<CompileSource> {
const queue: Array<CompileSource> = [];
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;
}
Expand Down Expand Up @@ -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
: '<anonymous>';

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
: '<anonymous>';
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,
Expand Down Expand Up @@ -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: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof EnvironmentConfigSchema>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
InstructionKind,
InstructionValue,
isArrayType,
isJsxType,
isMapType,
isPrimitiveType,
isRefOrRefValue,
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 <Item item={item} value={rand} />;
};
return <ItemList renderItem={renderItem} />;
}

```


## 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 <Item item={item} value={rand} />;
14 | };
15 | return <ItemList renderItem={renderItem} />;
```


Original file line number Diff line number Diff line change
@@ -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 <Item item={item} value={rand} />;
};
return <ItemList renderItem={renderItem} />;
}
Original file line number Diff line number Diff line change
@@ -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 <Item item={item} value={rand} />;
};
return <ItemList renderItem={renderItem} />;
}

```


## 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 <Item item={item} value={rand} />;
10 | };
11 | return <ItemList renderItem={renderItem} />;
```


Original file line number Diff line number Diff line change
@@ -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 <Item item={item} value={rand} />;
};
return <ItemList renderItem={renderItem} />;
}
Loading
Loading