From 5956a09e0b87203166a73a9e01543675c93417c8 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 1 May 2025 17:43:55 +0900 Subject: [PATCH] [compiler] Validate against mutable functions being frozen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 5 + .../src/HIR/Environment.ts | 5 + ...ValidateNoFreezingKnownMutableFunctions.ts | 130 ++++++++++++++++++ ...-argument-mutates-local-variable.expect.md | 34 +++++ ...unction-argument-mutates-local-variable.js | 8 ++ ...id-pass-mutable-function-as-prop.expect.md | 30 ++++ ...r.invalid-pass-mutable-function-as-prop.js | 8 ++ ...eturn-mutable-function-from-hook.expect.md | 36 +++++ ...valid-return-mutable-function-from-hook.js | 10 ++ ...es-memoizes-with-captures-values.expect.md | 77 +++++++++++ ...utable-method-not-memod-together.expect.md | 58 -------- 11 files changed, 343 insertions(+), 58 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.object-captured-by-returned-mutable-method-not-memod-together.expect.md 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 8dfdc76978cc..831d1ca38054 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -103,6 +103,7 @@ import {transformFire} from '../Transform'; import {validateNoImpureFunctionsInRender} from '../Validation/ValidateNoImpureFunctionsInRender'; import {CompilerError} from '..'; import {validateStaticComponents} from '../Validation/ValidateStaticComponents'; +import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoFreezingKnownMutableFunctions'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -274,6 +275,10 @@ function runWithEnvironment( if (env.config.validateNoImpureFunctionsInRender) { validateNoImpureFunctionsInRender(hir).unwrap(); } + + if (env.config.validateNoFreezingKnownMutableFunctions) { + validateNoFreezingKnownMutableFunctions(hir).unwrap(); + } } inferReactivePlaces(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 276e4f7b404d..a487b5086c0f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -367,6 +367,11 @@ const EnvironmentConfigSchema = z.object({ */ validateNoImpureFunctionsInRender: z.boolean().default(false), + /** + * Validate against passing mutable functions to hooks + */ + validateNoFreezingKnownMutableFunctions: 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/Validation/ValidateNoFreezingKnownMutableFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts new file mode 100644 index 000000000000..81612a744172 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts @@ -0,0 +1,130 @@ +/** + * 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, Effect, ErrorSeverity} from '..'; +import { + FunctionEffect, + HIRFunction, + IdentifierId, + isMutableEffect, + isRefOrRefLikeMutableType, + Place, +} from '../HIR'; +import { + eachInstructionValueOperand, + eachTerminalOperand, +} from '../HIR/visitors'; +import {Result} from '../Utils/Result'; +import {Iterable_some} from '../Utils/utils'; + +/** + * Validates that functions with known mutations (ie due to types) cannot be passed + * where a frozen value is expected. Example: + * + * ``` + * function Component() { + * const cache = new Map(); + * const onClick = () => { + * cache.set(...); + * } + * useHook(onClick); // ERROR: cannot pass a mutable value + * return // ERROR: cannot pass a mutable value + * } + * ``` + * + * Because `onClick` function mutates `cache` when called, `onClick` is equivalent to a mutable + * variables. But unlike other mutables values like an array, the receiver of the function has + * no way to avoid mutation — for example, a function can receive an array and choose not to mutate + * it, but there's no way to know that a function is mutable and avoid calling it. + * + * This pass detects functions with *known* mutations (Store or Mutate, not ConditionallyMutate) + * that are passed where a frozen value is expected and rejects them. + */ +export function validateNoFreezingKnownMutableFunctions( + fn: HIRFunction, +): Result { + const errors = new CompilerError(); + const contextMutationEffects: Map< + IdentifierId, + Extract + > = new Map(); + + function visitOperand(operand: Place): void { + if (operand.effect === Effect.Freeze) { + const effect = contextMutationEffects.get(operand.identifier.id); + if (effect != null) { + errors.push({ + reason: `This argument is a function which modifies local variables when called, which can bypass memoization and cause the UI not to update`, + description: `Functions that are returned from hooks, passed as arguments to hooks, or passed as props to components may not mutate local variables`, + loc: operand.loc, + severity: ErrorSeverity.InvalidReact, + }); + errors.push({ + reason: `The function modifies a local variable here`, + loc: effect.loc, + severity: ErrorSeverity.InvalidReact, + }); + } + } + } + + for (const block of fn.body.blocks.values()) { + for (const instr of block.instructions) { + const {lvalue, value} = instr; + switch (value.kind) { + case 'LoadLocal': { + const effect = contextMutationEffects.get(value.place.identifier.id); + if (effect != null) { + contextMutationEffects.set(lvalue.identifier.id, effect); + } + break; + } + case 'StoreLocal': { + const effect = contextMutationEffects.get(value.value.identifier.id); + if (effect != null) { + contextMutationEffects.set(lvalue.identifier.id, effect); + contextMutationEffects.set( + value.lvalue.place.identifier.id, + effect, + ); + } + break; + } + case 'FunctionExpression': { + const knownMutation = (value.loweredFunc.func.effects ?? []).find( + effect => { + return ( + effect.kind === 'ContextMutation' && + (effect.effect === Effect.Store || + effect.effect === Effect.Mutate) && + Iterable_some(effect.places, place => { + return ( + isMutableEffect(place.effect, place.loc) && + !isRefOrRefLikeMutableType(place.identifier.type) + ); + }) + ); + }, + ); + if (knownMutation && knownMutation.kind === 'ContextMutation') { + contextMutationEffects.set(lvalue.identifier.id, knownMutation); + } + break; + } + default: { + for (const operand of eachInstructionValueOperand(value)) { + visitOperand(operand); + } + } + } + } + for (const operand of eachTerminalOperand(block.terminal)) { + visitOperand(operand); + } + } + return errors.asResult(); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.expect.md new file mode 100644 index 000000000000..86a9e14d80e8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.expect.md @@ -0,0 +1,34 @@ + +## Input + +```javascript +// @validateNoFreezingKnownMutableFunctions + +function useFoo() { + const cache = new Map(); + useHook(() => { + cache.set('key', 'value'); + }); +} + +``` + + +## Error + +``` + 3 | function useFoo() { + 4 | const cache = new Map(); +> 5 | useHook(() => { + | ^^^^^^^ +> 6 | cache.set('key', 'value'); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 7 | }); + | ^^^^ InvalidReact: This argument is a function which modifies local variables when called, which can bypass memoization and cause the UI not to update. Functions that are returned from hooks, passed as arguments to hooks, or passed as props to components may not mutate local variables (5:7) + +InvalidReact: The function modifies a local variable here (6:6) + 8 | } + 9 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.js new file mode 100644 index 000000000000..323d35700a72 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hook-function-argument-mutates-local-variable.js @@ -0,0 +1,8 @@ +// @validateNoFreezingKnownMutableFunctions + +function useFoo() { + const cache = new Map(); + useHook(() => { + cache.set('key', 'value'); + }); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.expect.md new file mode 100644 index 000000000000..0d4742f26c60 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.expect.md @@ -0,0 +1,30 @@ + +## Input + +```javascript +// @validateNoFreezingKnownMutableFunctions +function Component() { + const cache = new Map(); + const fn = () => { + cache.set('key', 'value'); + }; + return ; +} + +``` + + +## Error + +``` + 5 | cache.set('key', 'value'); + 6 | }; +> 7 | return ; + | ^^ InvalidReact: This argument is a function which modifies local variables when called, which can bypass memoization and cause the UI not to update. Functions that are returned from hooks, passed as arguments to hooks, or passed as props to components may not mutate local variables (7:7) + +InvalidReact: The function modifies a local variable here (5:5) + 8 | } + 9 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.js new file mode 100644 index 000000000000..11793dfac5db --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-pass-mutable-function-as-prop.js @@ -0,0 +1,8 @@ +// @validateNoFreezingKnownMutableFunctions +function Component() { + const cache = new Map(); + const fn = () => { + cache.set('key', 'value'); + }; + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.expect.md new file mode 100644 index 000000000000..63a09bedaa0d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.expect.md @@ -0,0 +1,36 @@ + +## Input + +```javascript +// @validateNoFreezingKnownMutableFunctions +import {useHook} from 'shared-runtime'; + +function useFoo() { + useHook(); // for inference to kick in + const cache = new Map(); + return () => { + cache.set('key', 'value'); + }; +} + +``` + + +## Error + +``` + 5 | useHook(); // for inference to kick in + 6 | const cache = new Map(); +> 7 | return () => { + | ^^^^^^^ +> 8 | cache.set('key', 'value'); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 9 | }; + | ^^^^ InvalidReact: This argument is a function which modifies local variables when called, which can bypass memoization and cause the UI not to update. Functions that are returned from hooks, passed as arguments to hooks, or passed as props to components may not mutate local variables (7:9) + +InvalidReact: The function modifies a local variable here (8:8) + 10 | } + 11 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.js new file mode 100644 index 000000000000..3df37783dc11 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-return-mutable-function-from-hook.js @@ -0,0 +1,10 @@ +// @validateNoFreezingKnownMutableFunctions +import {useHook} from 'shared-runtime'; + +function useFoo() { + useHook(); // for inference to kick in + const cache = new Map(); + return () => { + cache.set('key', 'value'); + }; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md new file mode 100644 index 000000000000..e771bf12bde5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +// @flow +/** + * This hook returns a function that when called with an input object, + * will return the result of mapping that input with the supplied map + * function. Results are cached, so if the same input is passed again, + * the same output object will be returned. + * + * Note that this technically violates the rules of React and is unsafe: + * hooks must return immutable objects and be pure, and a function which + * captures and mutates a value when called is inherently not pure. + * + * However, in this case it is technically safe _if_ the mapping function + * is pure *and* the resulting objects are never modified. This is because + * the function only caches: the result of `returnedFunction(someInput)` + * strictly depends on `returnedFunction` and `someInput`, and cannot + * otherwise change over time. + */ +hook useMemoMap( + map: TInput => TOutput +): TInput => TOutput { + return useMemo(() => { + // The original issue is that `cache` was not memoized together with the returned + // function. This was because neither appears to ever be mutated — the function + // is known to mutate `cache` but the function isn't called. + // + // The fix is to detect cases like this — functions that are mutable but not called - + // and ensure that their mutable captures are aliased together into the same scope. + const cache = new WeakMap(); + return input => { + let output = cache.get(input); + if (output == null) { + output = map(input); + cache.set(input, output); + } + return output; + }; + }, [map]); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; + +function useMemoMap(map) { + const $ = _c(2); + let t0; + let t1; + if ($[0] !== map) { + const cache = new WeakMap(); + t1 = (input) => { + let output = cache.get(input); + if (output == null) { + output = map(input); + cache.set(input, output); + } + return output; + }; + $[0] = map; + $[1] = t1; + } else { + t1 = $[1]; + } + t0 = t1; + return t0; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.object-captured-by-returned-mutable-method-not-memod-together.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.object-captured-by-returned-mutable-method-not-memod-together.expect.md deleted file mode 100644 index 11f26962a283..000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.object-captured-by-returned-mutable-method-not-memod-together.expect.md +++ /dev/null @@ -1,58 +0,0 @@ - -## Input - -```javascript -// @flow -/** - * Creates a map function that lazily caches the results for future invocations. - */ -hook useMemoMap( - map: TInput => TOutput -): TInput => TOutput { - return useMemo(() => { - const cache = new WeakMap(); - return input => { - let output = cache.get(input); - if (output == null) { - output = map(input); - cache.set(input, output); - } - return output; - }; - }, [map]); -} - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; - -function useMemoMap(map) { - const $ = _c(2); - let t0; - let t1; - if ($[0] !== map) { - const cache = new WeakMap(); - t1 = (input) => { - let output = cache.get(input); - if (output == null) { - output = map(input); - cache.set(input, output); - } - return output; - }; - $[0] = map; - $[1] = t1; - } else { - t1 = $[1]; - } - t0 = t1; - return t0; -} - -``` - -### Eval output -(kind: exception) Fixture not implemented \ No newline at end of file