From 18e4165a942b2645321a65b2d9637cefe212b647 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sat, 3 May 2025 09:58:15 +0900 Subject: [PATCH 01/13] [compiler] Repro for false positive ValidateNoFreezingKnownMutableFunctions Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. [ghstack-poisoned] --- ...-use-effect-function-mutates-ref.expect.md | 58 +++++++++++++++++++ ...invalid-use-effect-function-mutates-ref.js | 27 +++++++++ 2 files changed, 85 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md new file mode 100644 index 0000000000000..6b244323a304d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md @@ -0,0 +1,58 @@ + +## Input + +```javascript +// @validateNoFreezingKnownMutableFunctions + +import {useCallback, useEffect, useRef} from 'react'; +import {useHook} from 'shared-runtime'; + +function Component() { + const params = useHook(); + const update = useCallback( + partialParams => { + const nextParams = { + ...params, + ...partialParams, + }; + nextParams.param = 'value'; + console.log(nextParams); + }, + [params] + ); + const ref = useRef(null); + useEffect(() => { + if (ref.current === null) { + update(); + } + }, [update]); + + return 'ok'; +} + +``` + + +## Error + +``` + 18 | ); + 19 | const ref = useRef(null); +> 20 | useEffect(() => { + | ^^^^^^^ +> 21 | if (ref.current === null) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 22 | update(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 23 | } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 24 | }, [update]); + | ^^^^ 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 (20:24) + +InvalidReact: The function modifies a local variable here (14:14) + 25 | + 26 | return 'ok'; + 27 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.js new file mode 100644 index 0000000000000..2e8b7827d09d9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.js @@ -0,0 +1,27 @@ +// @validateNoFreezingKnownMutableFunctions + +import {useCallback, useEffect, useRef} from 'react'; +import {useHook} from 'shared-runtime'; + +function Component() { + const params = useHook(); + const update = useCallback( + partialParams => { + const nextParams = { + ...params, + ...partialParams, + }; + nextParams.param = 'value'; + console.log(nextParams); + }, + [params] + ); + const ref = useRef(null); + useEffect(() => { + if (ref.current === null) { + update(); + } + }, [update]); + + return 'ok'; +} From 0ee6891337d8e48a0fd45eb946246b982ee90e2e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sat, 3 May 2025 16:25:52 +0900 Subject: [PATCH 02/13] Update on "[compiler] Repro for false positive ValidateNoFreezingKnownMutableFunctions" Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. [ghstack-poisoned] From e9b19a67329cf8cde6f61973c0902230293d6d8a Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sat, 3 May 2025 16:25:55 +0900 Subject: [PATCH 03/13] [compiler] Correctly infer context mutation places as outer (context) places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] --- .../src/Inference/InferReferenceEffects.ts | 21 ++++++++++------ ...-use-effect-function-mutates-ref.expect.md | 24 ++++++------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index d1546038edcbe..3dc532c87d6ea 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -37,7 +37,7 @@ import { import {FunctionSignature} from '../HIR/ObjectShape'; import { printIdentifier, - printMixedHIR, + printInstructionValue, printPlace, printSourceLocation, } from '../HIR/PrintHIR'; @@ -669,7 +669,14 @@ class InferenceState { } for (const [value, kind] of this.#values) { const id = identify(value); - result.values[id] = {kind, value: printMixedHIR(value)}; + result.values[id] = { + abstract: { + kind: kind.kind, + context: [...kind.context].map(printPlace), + reason: [...kind.reason], + }, + value: printInstructionValue(value), + }; } for (const [variable, values] of this.#variables) { result.variables[`$${variable}`] = [...values].map(identify); @@ -1844,11 +1851,11 @@ function getContextRefOperand( ): Array { const result = []; for (const place of eachInstructionValueOperand(instrValue)) { - if ( - state.isDefined(place) && - state.kind(place).kind === ValueKind.Context - ) { - result.push(place); + if (state.isDefined(place)) { + const kind = state.kind(place); + if (kind.kind === ValueKind.Context) { + result.push(...kind.context); + } } } return result; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md index 6b244323a304d..b3f1104239938 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md @@ -36,23 +36,13 @@ function Component() { ## Error ``` - 18 | ); - 19 | const ref = useRef(null); -> 20 | useEffect(() => { - | ^^^^^^^ -> 21 | if (ref.current === null) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 22 | update(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 23 | } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 24 | }, [update]); - | ^^^^ 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 (20:24) - -InvalidReact: The function modifies a local variable here (14:14) - 25 | - 26 | return 'ok'; - 27 | } + 12 | ...partialParams, + 13 | }; +> 14 | nextParams.param = 'value'; + | ^^^^^^^^^^ InvalidReact: Mutating a value returned from a function whose return value should not be mutated. Found mutation of `params` (14:14) + 15 | console.log(nextParams); + 16 | }, + 17 | [params] ``` \ No newline at end of file From 15307de4e2cc0b461bfda18786a77f6d66f3e75f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 7 May 2025 17:45:35 -0700 Subject: [PATCH 04/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From b7e8185097cb8793cb763557931c60106c6f1aff Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 8 May 2025 12:45:11 -0700 Subject: [PATCH 05/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From 04aeca7819da38469f3cddb28c209b184104659c Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 8 May 2025 13:04:00 -0700 Subject: [PATCH 06/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From 480f2cb08e6d5fdb9e865afa1df51583589e440e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 8 May 2025 16:24:19 -0700 Subject: [PATCH 07/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From c4741559e4f799f201a42522b0890de488312701 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 9 May 2025 09:01:07 -0700 Subject: [PATCH 08/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From 3e695658d410002cc4b77b9789de8362db1e8720 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 9 May 2025 10:36:05 -0700 Subject: [PATCH 09/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From c27becfe66eabfd6be0939fd34458889b9812333 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 9 May 2025 10:40:47 -0700 Subject: [PATCH 10/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From 52968fa744d183317451614c1398eb24633d0410 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 9 May 2025 10:42:41 -0700 Subject: [PATCH 11/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From 9ca91aa5368c3c600e22641bfe4b5b8e70cd0af3 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 12 May 2025 11:56:09 -0700 Subject: [PATCH 12/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned] From 8e202cf7ec1b61ac7d34961fe766d7a22946f327 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 12 May 2025 17:00:37 -0700 Subject: [PATCH 13/13] Update base for Update on "[compiler] Correctly infer context mutation places as outer (context) places" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned]