From 5f38ef671958a2fb054cb999d68faa8ffdc79e54 Mon Sep 17 00:00:00 2001 From: Damian Stasik <920747+damianstasik@users.noreply.github.com> Date: Fri, 17 Jan 2025 16:22:34 +0100 Subject: [PATCH 1/5] Fix maintainer check condition (#32110) ## Summary I've noticed that the value stored under `is_core_team` gets stringified, so some PRs may be mislabelled as coming from the core team. I've checked this on my fork and saw stringified `null` returned by the `is_core_team`, and this PR explicitly checks for the correct value. Feel free to close this PR if you want to go with another approach. ## How did you test this change? Checked this change on my fork with and without listing myself in the maintainers file. --- .github/workflows/shared_label_core_team_prs.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/shared_label_core_team_prs.yml b/.github/workflows/shared_label_core_team_prs.yml index b96aea88054fa..227dcee4c20b8 100644 --- a/.github/workflows/shared_label_core_team_prs.yml +++ b/.github/workflows/shared_label_core_team_prs.yml @@ -13,7 +13,7 @@ jobs: uses: facebook/react/.github/workflows/shared_check_maintainer.yml@main label: - if: ${{ needs.check_maintainer.outputs.is_core_team }} + if: ${{ needs.check_maintainer.outputs.is_core_team == 'true' }} runs-on: ubuntu-latest needs: check_maintainer steps: From d46b04a27dc9fb4c7baaa379f5a6aec1449883a1 Mon Sep 17 00:00:00 2001 From: lauren Date: Fri, 17 Jan 2025 10:35:25 -0500 Subject: [PATCH 2/5] [ci] Fix maintainer output condition check (#32111) It appears GH actions treats outputs from workflow_calls to [always be strings](https://github.com/orgs/community/discussions/9343) so we need to do an explicit comparison. --- .github/workflows/compiler_discord_notify.yml | 2 +- .github/workflows/runtime_discord_notify.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/compiler_discord_notify.yml b/.github/workflows/compiler_discord_notify.yml index b9cc3e9b9fa83..c21c533c0ffcd 100644 --- a/.github/workflows/compiler_discord_notify.yml +++ b/.github/workflows/compiler_discord_notify.yml @@ -12,7 +12,7 @@ jobs: uses: facebook/react/.github/workflows/shared_check_maintainer.yml@main notify: - if: ${{ needs.check_maintainer.outputs.is_core_team }} + if: ${{ needs.check_maintainer.outputs.is_core_team == 'true' }} needs: check_maintainer runs-on: ubuntu-latest steps: diff --git a/.github/workflows/runtime_discord_notify.yml b/.github/workflows/runtime_discord_notify.yml index abf7970a1e634..9ef578eeed974 100644 --- a/.github/workflows/runtime_discord_notify.yml +++ b/.github/workflows/runtime_discord_notify.yml @@ -12,7 +12,7 @@ jobs: uses: facebook/react/.github/workflows/shared_check_maintainer.yml@main notify: - if: ${{ needs.check_maintainer.outputs.is_core_team }} + if: ${{ needs.check_maintainer.outputs.is_core_team == 'true' }} needs: check_maintainer runs-on: ubuntu-latest steps: From 313c8c55de39cd5f009ebb033eec1666b3daa59e Mon Sep 17 00:00:00 2001 From: Bramus Date: Fri, 17 Jan 2025 17:42:30 +0100 Subject: [PATCH 3/5] Fix moveBefore feature detection (#32087) `moveBefore` was moved to the `ParentNode` mixin as per https://github.com/whatwg/dom/pull/1307#discussion_r1881981120 _(and was committed in https://github.com/whatwg/dom/commit/3f3e94c5beda922962dacaeb606087f57bd7f7be)_ As a result, its existence can no longer be checked on `Node.prototype` but must be checked in `Element.prototype` --- packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index af93760ccfcc9..cea03b73b59d5 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -780,7 +780,7 @@ const supportsMoveBefore = // $FlowFixMe[prop-missing]: We're doing the feature detection here. enableMoveBefore && typeof window !== 'undefined' && - typeof window.Node.prototype.moveBefore === 'function'; + typeof window.Element.prototype.moveBefore === 'function'; export function appendChild( parentInstance: Instance, From 7f3826e8e44caa6984232ab8d752d8b9441e4c99 Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Fri, 17 Jan 2025 09:04:02 -0800 Subject: [PATCH 4/5] [compiler] validation against calling impure functions (#31960) For now we just reject all calls of impure functions, and the validation is off by default. Going forward we can make this more precise and only reject impure functions called during render. Note that I was intentionally imprecise in the return type of these functions in order to avoid changing output of existing code. We lie to the compiler and say that Date.now, performance.now, and Math.random return unknown mutable objects rather than primitives. Once the validation is complete and vetted we can switch this to be more precise. --- .../src/Entrypoint/Pipeline.ts | 5 ++ .../src/HIR/Environment.ts | 5 ++ .../src/HIR/Globals.ts | 50 ++++++++++++++++++ .../src/HIR/ObjectShape.ts | 4 ++ .../src/HIR/TypeSchema.ts | 4 ++ .../ValiateNoImpureFunctionsInRender.ts | 52 +++++++++++++++++++ ...valid-impure-functions-in-render.expect.md | 33 ++++++++++++ ...rror.invalid-impure-functions-in-render.js | 8 +++ 8 files changed, 161 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValiateNoImpureFunctionsInRender.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index ca6abc0748eb9..a354acf69274e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -99,6 +99,7 @@ import {propagateScopeDependenciesHIR} from '../HIR/PropagateScopeDependenciesHI import {outlineJSX} from '../Optimization/OutlineJsx'; import {optimizePropsMethodCalls} from '../Optimization/OptimizePropsMethodCalls'; import {transformFire} from '../Transform'; +import {validateNoImpureFunctionsInRender} from '../Validation/ValiateNoImpureFunctionsInRender'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -257,6 +258,10 @@ function runWithEnvironment( validateNoJSXInTryStatement(hir); } + if (env.config.validateNoImpureFunctionsInRender) { + validateNoImpureFunctionsInRender(hir); + } + inferReactivePlaces(hir); log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); 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 f1db567660ec9..9df34242ec089 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -353,6 +353,11 @@ const EnvironmentConfigSchema = z.object({ validateNoCapitalizedCalls: z.nullable(z.array(z.string())).default(null), validateBlocklistedImports: z.nullable(z.array(z.string())).default(null), + /** + * Validate against impure functions called during render + */ + validateNoImpureFunctionsInRender: z.boolean().default(false), + /* * When enabled, the compiler assumes that hooks follow the Rules of React: * - Hooks may memoize computation based on any of their parameters, thus diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts index ac5d44bab0875..3fe2e938ce57b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -141,6 +141,44 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [ ], ]), ], + [ + 'performance', + addObject(DEFAULT_SHAPES, 'performance', [ + // Static methods (TODO) + [ + 'now', + // Date.now() + addFunction(DEFAULT_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: {kind: 'Poly'}, // TODO: could be Primitive, but that would change existing compilation + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Mutable, // same here + impure: true, + canonicalName: 'performance.now', + }), + ], + ]), + ], + [ + 'Date', + addObject(DEFAULT_SHAPES, 'Date', [ + // Static methods (TODO) + [ + 'now', + // Date.now() + addFunction(DEFAULT_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: {kind: 'Poly'}, // TODO: could be Primitive, but that would change existing compilation + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Mutable, // same here + impure: true, + canonicalName: 'Date.now', + }), + ], + ]), + ], [ 'Math', addObject(DEFAULT_SHAPES, 'Math', [ @@ -209,6 +247,18 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [ returnValueKind: ValueKind.Primitive, }), ], + [ + 'random', + addFunction(DEFAULT_SHAPES, [], { + positionalParams: [], + restParam: Effect.Read, + returnType: {kind: 'Poly'}, // TODO: could be Primitive, but that would change existing compilation + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Mutable, // same here + impure: true, + canonicalName: 'Math.random', + }), + ], ]), ], ['Infinity', {kind: 'Primitive'}], diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index 4482d17890196..7c05c05531ae9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -172,6 +172,10 @@ export type FunctionSignature = { * - Else uses the effects specified by this signature. */ mutableOnlyIfOperandsAreMutable?: boolean; + + impure?: boolean; + + canonicalName?: string; }; /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts index 362328db72155..9aac2a264f60a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts @@ -40,6 +40,8 @@ export type FunctionTypeConfig = { returnValueKind: ValueKind; noAlias?: boolean | null | undefined; mutableOnlyIfOperandsAreMutable?: boolean | null | undefined; + impure?: boolean | null | undefined; + canonicalName?: string | null | undefined; }; export const FunctionTypeSchema: z.ZodType = z.object({ kind: z.literal('function'), @@ -50,6 +52,8 @@ export const FunctionTypeSchema: z.ZodType = z.object({ returnValueKind: ValueKindSchema, noAlias: z.boolean().nullable().optional(), mutableOnlyIfOperandsAreMutable: z.boolean().nullable().optional(), + impure: z.boolean().nullable().optional(), + canonicalName: z.string().nullable().optional(), }); export type HookTypeConfig = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValiateNoImpureFunctionsInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValiateNoImpureFunctionsInRender.ts new file mode 100644 index 0000000000000..287a0aa978b6f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValiateNoImpureFunctionsInRender.ts @@ -0,0 +1,52 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {CompilerError, ErrorSeverity} from '..'; +import {HIRFunction} from '../HIR'; +import {getFunctionCallSignature} from '../Inference/InferReferenceEffects'; + +/** + * Checks that known-impure functions are not called during render. Examples of invalid functions to + * call during render are `Math.random()` and `Date.now()`. Users may extend this set of + * impure functions via a module type provider and specifying functions with `impure: true`. + * + * TODO: add best-effort analysis of functions which are called during render. We have variations of + * this in several of our validation passes and should unify those analyses into a reusable helper + * and use it here. + */ +export function validateNoImpureFunctionsInRender(fn: HIRFunction): void { + const errors = new CompilerError(); + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const value = instr.value; + if (value.kind === 'MethodCall' || value.kind == 'CallExpression') { + const callee = + value.kind === 'MethodCall' ? value.property : value.callee; + const signature = getFunctionCallSignature( + fn.env, + callee.identifier.type, + ); + if (signature != null && signature.impure === true) { + errors.push({ + reason: + 'Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)', + description: + signature.canonicalName != null + ? `\`${signature.canonicalName}\` is an impure function whose results may change on every call` + : null, + severity: ErrorSeverity.InvalidReact, + loc: callee.loc, + suggestions: null, + }); + } + } + } + } + if (errors.hasErrors()) { + throw errors; + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md new file mode 100644 index 0000000000000..a67d467df8cfd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +// @validateNoImpureFunctionsInRender + +function Component() { + const date = Date.now(); + const now = performance.now(); + const rand = Math.random(); + return ; +} + +``` + + +## Error + +``` + 2 | + 3 | function Component() { +> 4 | const date = Date.now(); + | ^^^^^^^^ InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `Date.now` is an impure function whose results may change on every call (4:4) + +InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `performance.now` is an impure function whose results may change on every call (5:5) + +InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `Math.random` is an impure function whose results may change on every call (6:6) + 5 | const now = performance.now(); + 6 | const rand = Math.random(); + 7 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.js new file mode 100644 index 0000000000000..6faf98caff702 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.js @@ -0,0 +1,8 @@ +// @validateNoImpureFunctionsInRender + +function Component() { + const date = Date.now(); + const now = performance.now(); + const rand = Math.random(); + return ; +} From 77656c557a05951643ffecdf4efdf43b8bc73d84 Mon Sep 17 00:00:00 2001 From: lauren Date: Fri, 17 Jan 2025 12:42:07 -0500 Subject: [PATCH 5/5] [ci] Use correct actor when checking if maintainer (#32112) --- .github/workflows/compiler_discord_notify.yml | 2 ++ .github/workflows/runtime_discord_notify.yml | 2 ++ .github/workflows/shared_check_maintainer.yml | 6 +++++- .github/workflows/shared_label_core_team_prs.yml | 2 ++ 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/compiler_discord_notify.yml b/.github/workflows/compiler_discord_notify.yml index c21c533c0ffcd..b4694e12be94d 100644 --- a/.github/workflows/compiler_discord_notify.yml +++ b/.github/workflows/compiler_discord_notify.yml @@ -10,6 +10,8 @@ on: jobs: check_maintainer: uses: facebook/react/.github/workflows/shared_check_maintainer.yml@main + with: + actor: ${{ github.event.pull_request.user.login }} notify: if: ${{ needs.check_maintainer.outputs.is_core_team == 'true' }} diff --git a/.github/workflows/runtime_discord_notify.yml b/.github/workflows/runtime_discord_notify.yml index 9ef578eeed974..a29735ac4ed8e 100644 --- a/.github/workflows/runtime_discord_notify.yml +++ b/.github/workflows/runtime_discord_notify.yml @@ -10,6 +10,8 @@ on: jobs: check_maintainer: uses: facebook/react/.github/workflows/shared_check_maintainer.yml@main + with: + actor: ${{ github.event.pull_request.user.login }} notify: if: ${{ needs.check_maintainer.outputs.is_core_team == 'true' }} diff --git a/.github/workflows/shared_check_maintainer.yml b/.github/workflows/shared_check_maintainer.yml index 53f4c7a8af046..cd43ffe55979a 100644 --- a/.github/workflows/shared_check_maintainer.yml +++ b/.github/workflows/shared_check_maintainer.yml @@ -2,6 +2,10 @@ name: (Shared) Check maintainer on: workflow_call: + inputs: + actor: + required: true + type: string outputs: is_core_team: value: ${{ jobs.check_maintainer.outputs.is_core_team }} @@ -24,7 +28,7 @@ jobs: with: script: | const fs = require('fs'); - const actor = '${{ github.actor }}'; + const actor = '${{ inputs.actor }}'; const data = await fs.readFileSync('./MAINTAINERS', { encoding: 'utf8' }); const maintainers = new Set(data.split('\n')); if (maintainers.has(actor)) { diff --git a/.github/workflows/shared_label_core_team_prs.yml b/.github/workflows/shared_label_core_team_prs.yml index 227dcee4c20b8..dc432b54f72a6 100644 --- a/.github/workflows/shared_label_core_team_prs.yml +++ b/.github/workflows/shared_label_core_team_prs.yml @@ -11,6 +11,8 @@ env: jobs: check_maintainer: uses: facebook/react/.github/workflows/shared_check_maintainer.yml@main + with: + actor: ${{ github.event.pull_request.user.login }} label: if: ${{ needs.check_maintainer.outputs.is_core_team == 'true' }}