Skip to content

Commit 2976f25

Browse files
committed
[compiler] Improve impurity/ref validation
# Summary note: This implements the idea discussed in reactwg/react#389 (comment) This is a large PR that significantly changes our impurity and ref validation to address multiple issues. The goal is to reduce false positives and make the errors we do report more actionable. ## Validating Against Impure Values In Render Currently we create `Impure` effects for impure functions like `Date.now()` or `Math.random()`, and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok). This PR updates to validate that impure values are not transitively consumed during render, building on the new effects system: rather than look at instruction types, we use effects like `Capture a -> b`, `Impure a`, and `Render b` to determine how impure values are introduced and where they flow through the program. We're intentionally conservative, and do _not_ propagate impurity through MaybeCapture effects, which is used for functions we don't have signatures for. This means that things like `identity(performance.now())` drop the impurity. This feels like a good compromise since it means we have very high confidence in the errors that we report and can always add increased strictness later as our confidence increases. An example error: ``` Error: Cannot access impure value during render Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). error.invalid-impure-value-in-render-helper.ts:5:17 3 | const now = () => Date.now(); 4 | const render = () => { > 5 | return <div>{now()}</div>; | ^^^^^ Cannot access impure value during render 6 | }; 7 | return <div>{render()}</div>; 8 | } error.invalid-impure-value-in-render-helper.ts:3:20 1 | // @validateNoImpureFunctionsInRender 2 | function Component() { > 3 | const now = () => Date.now(); | ^^^^^^^^^^ `Date.now` is an impure function. 4 | const render = () => { 5 | return <div>{now()}</div>; 6 | }; ``` Impure values are allowed to flow into refs, meaning that we now allow `useRef(Date.now())` or `useRef(localFunctionThatReturnsMathDotRandom())` which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well. ## Refs Now Treated As Impure Values in Render Reading a ref now produces an `Impure` effect, and reading refs in render is validated using the above validation against impure values in render. The error category and message is customized for refs, we're just reusing the validation implementation. This means you get consistent results for `performance.now()` as for `ref.current`. A nice consistency win. ## Simplified writing-ref validation Now that _reading_ a ref in render is validated using the impure values infra, I also dramatically simplified ValidateNoRefAccessInRender to focus solely on validation against _writing_ refs during render. It was harder to use the new effects infra for this since we intentionally do not record ref mutations as a `Mutate` effect. So for now, the pass switches on InstructionValue variants. We continue to support the `if (ref.current == null) { ref.current = <init> }` pattern and reasonable variants of it. We're conservative about what we consider to be a write of a ref - `foo(ref)` now assumes you're not mutating rather than the inverse. # Takeaways * Impure-values-in-render logic is more conservative about what it considers an error (ie to users it appears more persmissive). We follow clearly identifiable (and if we wanted traceable/explainable) paths from impure sources through to where they are rendered. We allow many more cases than before, notably `x = foo(ref)` optimistically assumes you don't read the ref. Concretely, this follows from not tracking impure values across MaybeCapture effects. * No-writing-refs-in-render is also more conservative about what it counts as a write, appearing to users as allowing more cases. We look for very direct evidence of writing to refs, ie `ref.current = <value>` or `ref.current.property = <value>`, and allow potential writes through eg `writeToRef(ref, value)`.
1 parent 5aec1b2 commit 2976f25

File tree

110 files changed

+3152
-1858
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

110 files changed

+3152
-1858
lines changed

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,6 @@ import {propagateScopeDependenciesHIR} from '../HIR/PropagateScopeDependenciesHI
9696
import {outlineJSX} from '../Optimization/OutlineJsx';
9797
import {optimizePropsMethodCalls} from '../Optimization/OptimizePropsMethodCalls';
9898
import {transformFire} from '../Transform';
99-
import {validateNoImpureFunctionsInRender} from '../Validation/ValidateNoImpureFunctionsInRender';
10099
import {validateStaticComponents} from '../Validation/ValidateStaticComponents';
101100
import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoFreezingKnownMutableFunctions';
102101
import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects';
@@ -107,6 +106,7 @@ import {nameAnonymousFunctions} from '../Transform/NameAnonymousFunctions';
107106
import {optimizeForSSR} from '../Optimization/OptimizeForSSR';
108107
import {validateExhaustiveDependencies} from '../Validation/ValidateExhaustiveDependencies';
109108
import {validateSourceLocations} from '../Validation/ValidateSourceLocations';
109+
import {validateNoImpureValuesInRender} from '../Validation/ValidateNoImpureValuesInRender';
110110

111111
export type CompilerPipelineValue =
112112
| {kind: 'ast'; name: string; value: CodegenFunction}
@@ -271,10 +271,6 @@ function runWithEnvironment(
271271
assertValidMutableRanges(hir);
272272
}
273273

274-
if (env.config.validateRefAccessDuringRender) {
275-
validateNoRefAccessInRender(hir).unwrap();
276-
}
277-
278274
if (env.config.validateNoSetStateInRender) {
279275
validateNoSetStateInRender(hir).unwrap();
280276
}
@@ -296,8 +292,15 @@ function runWithEnvironment(
296292
env.logErrors(validateNoJSXInTryStatement(hir));
297293
}
298294

299-
if (env.config.validateNoImpureFunctionsInRender) {
300-
validateNoImpureFunctionsInRender(hir).unwrap();
295+
if (
296+
env.config.validateNoImpureFunctionsInRender ||
297+
env.config.validateRefAccessDuringRender
298+
) {
299+
validateNoImpureValuesInRender(hir).unwrap();
300+
}
301+
302+
if (env.config.validateRefAccessDuringRender) {
303+
validateNoRefAccessInRender(hir).unwrap();
301304
}
302305

303306
validateNoFreezingKnownMutableFunctions(hir).unwrap();

compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts

Lines changed: 162 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import {
3838
addObject,
3939
} from './ObjectShape';
4040
import {BuiltInType, ObjectType, PolyType} from './Types';
41-
import {TypeConfig} from './TypeSchema';
41+
import {AliasingSignatureConfig, TypeConfig} from './TypeSchema';
4242
import {assertExhaustive} from '../Utils/utils';
4343
import {isHookName} from './Environment';
4444
import {CompilerError, SourceLocation} from '..';
@@ -626,11 +626,136 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
626626
// TODO: rest of Global objects
627627
];
628628

629+
const createRenderHookAliasing: (
630+
reason: ValueReason,
631+
) => AliasingSignatureConfig = reason => ({
632+
receiver: '@receiver',
633+
params: [],
634+
rest: '@rest',
635+
returns: '@returns',
636+
temporaries: [],
637+
effects: [
638+
// Freeze the arguments
639+
{
640+
kind: 'Freeze',
641+
value: '@rest',
642+
reason: ValueReason.HookCaptured,
643+
},
644+
// Render the arguments
645+
{
646+
kind: 'Render',
647+
place: '@rest',
648+
},
649+
// Returns a frozen value
650+
{
651+
kind: 'Create',
652+
into: '@returns',
653+
value: ValueKind.Frozen,
654+
reason,
655+
},
656+
// May alias any arguments into the return
657+
{
658+
kind: 'Alias',
659+
from: '@rest',
660+
into: '@returns',
661+
},
662+
],
663+
});
664+
665+
const EffectHookAliasing: AliasingSignatureConfig = {
666+
receiver: '@receiver',
667+
params: ['@fn', '@deps'],
668+
rest: '@rest',
669+
returns: '@returns',
670+
temporaries: ['@effect'],
671+
effects: [
672+
// Freezes the function and deps
673+
{
674+
kind: 'Freeze',
675+
value: '@rest',
676+
reason: ValueReason.Effect,
677+
},
678+
{
679+
kind: 'Freeze',
680+
value: '@fn',
681+
reason: ValueReason.Effect,
682+
},
683+
{
684+
kind: 'Freeze',
685+
value: '@deps',
686+
reason: ValueReason.Effect,
687+
},
688+
// Deps are accessed during render
689+
{
690+
kind: 'Render',
691+
place: '@deps',
692+
},
693+
// Internally creates an effect object that captures the function and deps
694+
{
695+
kind: 'Create',
696+
into: '@effect',
697+
value: ValueKind.Frozen,
698+
reason: ValueReason.KnownReturnSignature,
699+
},
700+
// The effect stores the function and dependencies
701+
{
702+
kind: 'Capture',
703+
from: '@rest',
704+
into: '@effect',
705+
},
706+
{
707+
kind: 'Capture',
708+
from: '@fn',
709+
into: '@effect',
710+
},
711+
// Returns undefined
712+
{
713+
kind: 'Create',
714+
into: '@returns',
715+
value: ValueKind.Primitive,
716+
reason: ValueReason.KnownReturnSignature,
717+
},
718+
],
719+
};
720+
629721
/*
630722
* TODO(mofeiZ): We currently only store rest param effects for hooks.
631723
* now that FeatureFlag `enableTreatHooksAsFunctions` is removed we can
632724
* use positional params too (?)
633725
*/
726+
const useEffectEvent = addHook(
727+
DEFAULT_SHAPES,
728+
{
729+
positionalParams: [],
730+
restParam: Effect.Freeze,
731+
returnType: {
732+
kind: 'Function',
733+
return: {kind: 'Poly'},
734+
shapeId: BuiltInEffectEventId,
735+
isConstructor: false,
736+
},
737+
calleeEffect: Effect.Read,
738+
hookKind: 'useEffectEvent',
739+
// Frozen because it should not mutate any locally-bound values
740+
returnValueKind: ValueKind.Frozen,
741+
aliasing: {
742+
receiver: '@receiver',
743+
params: ['@value'],
744+
rest: null,
745+
returns: '@return',
746+
temporaries: [],
747+
effects: [
748+
{
749+
kind: 'Freeze',
750+
value: '@value',
751+
reason: ValueReason.HookCaptured,
752+
},
753+
{kind: 'Assign', from: '@value', into: '@return'},
754+
],
755+
},
756+
},
757+
BuiltInUseEffectEventId,
758+
);
634759
const REACT_APIS: Array<[string, BuiltInType]> = [
635760
[
636761
'useContext',
@@ -644,6 +769,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
644769
hookKind: 'useContext',
645770
returnValueKind: ValueKind.Frozen,
646771
returnValueReason: ValueReason.Context,
772+
aliasing: createRenderHookAliasing(ValueReason.Context),
647773
},
648774
BuiltInUseContextHookId,
649775
),
@@ -658,6 +784,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
658784
hookKind: 'useState',
659785
returnValueKind: ValueKind.Frozen,
660786
returnValueReason: ValueReason.State,
787+
aliasing: createRenderHookAliasing(ValueReason.State),
661788
}),
662789
],
663790
[
@@ -670,6 +797,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
670797
hookKind: 'useActionState',
671798
returnValueKind: ValueKind.Frozen,
672799
returnValueReason: ValueReason.State,
800+
aliasing: createRenderHookAliasing(ValueReason.HookCaptured),
673801
}),
674802
],
675803
[
@@ -682,6 +810,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
682810
hookKind: 'useReducer',
683811
returnValueKind: ValueKind.Frozen,
684812
returnValueReason: ValueReason.ReducerState,
813+
aliasing: createRenderHookAliasing(ValueReason.ReducerState),
685814
}),
686815
],
687816
[
@@ -693,6 +822,22 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
693822
calleeEffect: Effect.Read,
694823
hookKind: 'useRef',
695824
returnValueKind: ValueKind.Mutable,
825+
aliasing: {
826+
receiver: '@receiver',
827+
params: [],
828+
rest: '@rest',
829+
returns: '@return',
830+
temporaries: [],
831+
effects: [
832+
{
833+
kind: 'Create',
834+
into: '@return',
835+
value: ValueKind.Mutable,
836+
reason: ValueReason.KnownReturnSignature,
837+
},
838+
{kind: 'Capture', from: '@rest', into: '@return'},
839+
],
840+
},
696841
}),
697842
],
698843
[
@@ -715,17 +860,24 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
715860
calleeEffect: Effect.Read,
716861
hookKind: 'useMemo',
717862
returnValueKind: ValueKind.Frozen,
863+
aliasing: createRenderHookAliasing(ValueReason.HookCaptured),
718864
}),
719865
],
720866
[
721867
'useCallback',
722868
addHook(DEFAULT_SHAPES, {
723869
positionalParams: [],
724870
restParam: Effect.Freeze,
725-
returnType: {kind: 'Poly'},
871+
returnType: {
872+
kind: 'Function',
873+
isConstructor: false,
874+
return: {kind: 'Poly'},
875+
shapeId: null,
876+
},
726877
calleeEffect: Effect.Read,
727878
hookKind: 'useCallback',
728879
returnValueKind: ValueKind.Frozen,
880+
aliasing: createRenderHookAliasing(ValueReason.HookCaptured),
729881
}),
730882
],
731883
[
@@ -739,41 +891,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
739891
calleeEffect: Effect.Read,
740892
hookKind: 'useEffect',
741893
returnValueKind: ValueKind.Frozen,
742-
aliasing: {
743-
receiver: '@receiver',
744-
params: [],
745-
rest: '@rest',
746-
returns: '@returns',
747-
temporaries: ['@effect'],
748-
effects: [
749-
// Freezes the function and deps
750-
{
751-
kind: 'Freeze',
752-
value: '@rest',
753-
reason: ValueReason.Effect,
754-
},
755-
// Internally creates an effect object that captures the function and deps
756-
{
757-
kind: 'Create',
758-
into: '@effect',
759-
value: ValueKind.Frozen,
760-
reason: ValueReason.KnownReturnSignature,
761-
},
762-
// The effect stores the function and dependencies
763-
{
764-
kind: 'Capture',
765-
from: '@rest',
766-
into: '@effect',
767-
},
768-
// Returns undefined
769-
{
770-
kind: 'Create',
771-
into: '@returns',
772-
value: ValueKind.Primitive,
773-
reason: ValueReason.KnownReturnSignature,
774-
},
775-
],
776-
},
894+
aliasing: EffectHookAliasing,
777895
},
778896
BuiltInUseEffectHookId,
779897
),
@@ -789,6 +907,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
789907
calleeEffect: Effect.Read,
790908
hookKind: 'useLayoutEffect',
791909
returnValueKind: ValueKind.Frozen,
910+
aliasing: EffectHookAliasing,
792911
},
793912
BuiltInUseLayoutEffectHookId,
794913
),
@@ -804,6 +923,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
804923
calleeEffect: Effect.Read,
805924
hookKind: 'useInsertionEffect',
806925
returnValueKind: ValueKind.Frozen,
926+
aliasing: EffectHookAliasing,
807927
},
808928
BuiltInUseInsertionEffectHookId,
809929
),
@@ -817,6 +937,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
817937
calleeEffect: Effect.Read,
818938
hookKind: 'useTransition',
819939
returnValueKind: ValueKind.Frozen,
940+
aliasing: createRenderHookAliasing(ValueReason.HookCaptured),
820941
}),
821942
],
822943
[
@@ -829,6 +950,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
829950
hookKind: 'useOptimistic',
830951
returnValueKind: ValueKind.Frozen,
831952
returnValueReason: ValueReason.State,
953+
aliasing: createRenderHookAliasing(ValueReason.HookCaptured),
832954
}),
833955
],
834956
[
@@ -842,6 +964,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
842964
returnType: {kind: 'Poly'},
843965
calleeEffect: Effect.Read,
844966
returnValueKind: ValueKind.Frozen,
967+
aliasing: createRenderHookAliasing(ValueReason.HookCaptured),
845968
},
846969
BuiltInUseOperatorId,
847970
),
@@ -866,27 +989,8 @@ const REACT_APIS: Array<[string, BuiltInType]> = [
866989
BuiltInFireId,
867990
),
868991
],
869-
[
870-
'useEffectEvent',
871-
addHook(
872-
DEFAULT_SHAPES,
873-
{
874-
positionalParams: [],
875-
restParam: Effect.Freeze,
876-
returnType: {
877-
kind: 'Function',
878-
return: {kind: 'Poly'},
879-
shapeId: BuiltInEffectEventId,
880-
isConstructor: false,
881-
},
882-
calleeEffect: Effect.Read,
883-
hookKind: 'useEffectEvent',
884-
// Frozen because it should not mutate any locally-bound values
885-
returnValueKind: ValueKind.Frozen,
886-
},
887-
BuiltInUseEffectEventId,
888-
),
889-
],
992+
['useEffectEvent', useEffectEvent],
993+
['experimental_useEffectEvent', useEffectEvent],
890994
['AUTODEPS', addObject(DEFAULT_SHAPES, BuiltInAutodepsId, [])],
891995
];
892996

0 commit comments

Comments
 (0)