Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
15ea89b753 | ||
|
|
cd1d0dc2dc |
@@ -22,6 +22,7 @@ import {
|
||||
BasicBlock,
|
||||
isUseRefType,
|
||||
SourceLocation,
|
||||
ArrayExpression,
|
||||
} from '../HIR';
|
||||
import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors';
|
||||
import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
|
||||
@@ -36,11 +37,17 @@ type DerivationMetadata = {
|
||||
isStateSource: boolean;
|
||||
};
|
||||
|
||||
type EffectMetadata = {
|
||||
effect: HIRFunction;
|
||||
dependencies: ArrayExpression;
|
||||
};
|
||||
|
||||
type ValidationContext = {
|
||||
readonly functions: Map<IdentifierId, FunctionExpression>;
|
||||
readonly candidateDependencies: Map<IdentifierId, ArrayExpression>;
|
||||
readonly errors: CompilerError;
|
||||
readonly derivationCache: DerivationCache;
|
||||
readonly effects: Set<HIRFunction>;
|
||||
readonly effectsCache: Map<IdentifierId, EffectMetadata>;
|
||||
readonly setStateLoads: Map<IdentifierId, IdentifierId | null>;
|
||||
readonly setStateUsages: Map<IdentifierId, Set<SourceLocation>>;
|
||||
};
|
||||
@@ -175,18 +182,20 @@ export function validateNoDerivedComputationsInEffects_exp(
|
||||
fn: HIRFunction,
|
||||
): Result<void, CompilerError> {
|
||||
const functions: Map<IdentifierId, FunctionExpression> = new Map();
|
||||
const candidateDependencies: Map<IdentifierId, ArrayExpression> = new Map();
|
||||
const derivationCache = new DerivationCache();
|
||||
const errors = new CompilerError();
|
||||
const effects: Set<HIRFunction> = new Set();
|
||||
const effectsCache: Map<IdentifierId, EffectMetadata> = new Map();
|
||||
|
||||
const setStateLoads: Map<IdentifierId, IdentifierId> = new Map();
|
||||
const setStateUsages: Map<IdentifierId, Set<SourceLocation>> = new Map();
|
||||
|
||||
const context: ValidationContext = {
|
||||
functions,
|
||||
candidateDependencies,
|
||||
errors,
|
||||
derivationCache,
|
||||
effects,
|
||||
effectsCache,
|
||||
setStateLoads,
|
||||
setStateUsages,
|
||||
};
|
||||
@@ -229,8 +238,8 @@ export function validateNoDerivedComputationsInEffects_exp(
|
||||
isFirstPass = false;
|
||||
} while (context.derivationCache.snapshot());
|
||||
|
||||
for (const effect of effects) {
|
||||
validateEffect(effect, context);
|
||||
for (const [, effect] of effectsCache) {
|
||||
validateEffect(effect.effect, effect.dependencies, context);
|
||||
}
|
||||
|
||||
return errors.asResult();
|
||||
@@ -354,8 +363,14 @@ function recordInstructionDerivations(
|
||||
value.args[1].kind === 'Identifier'
|
||||
) {
|
||||
const effectFunction = context.functions.get(value.args[0].identifier.id);
|
||||
if (effectFunction != null) {
|
||||
context.effects.add(effectFunction.loweredFunc.func);
|
||||
const deps = context.candidateDependencies.get(
|
||||
value.args[1].identifier.id,
|
||||
);
|
||||
if (effectFunction != null && deps != null) {
|
||||
context.effectsCache.set(value.args[0].identifier.id, {
|
||||
effect: effectFunction.loweredFunc.func,
|
||||
dependencies: deps,
|
||||
});
|
||||
}
|
||||
} else if (isUseStateType(lvalue.identifier) && value.args.length > 0) {
|
||||
typeOfValue = 'fromState';
|
||||
@@ -367,6 +382,8 @@ function recordInstructionDerivations(
|
||||
);
|
||||
return;
|
||||
}
|
||||
} else if (value.kind === 'ArrayExpression') {
|
||||
context.candidateDependencies.set(lvalue.identifier.id, value);
|
||||
}
|
||||
|
||||
for (const operand of eachInstructionOperand(instr)) {
|
||||
@@ -596,6 +613,7 @@ function getFnLocalDeps(
|
||||
|
||||
function validateEffect(
|
||||
effectFunction: HIRFunction,
|
||||
dependencies: ArrayExpression,
|
||||
context: ValidationContext,
|
||||
): void {
|
||||
const seenBlocks: Set<BlockId> = new Set();
|
||||
@@ -612,6 +630,16 @@ function validateEffect(
|
||||
Set<SourceLocation>
|
||||
> = new Map();
|
||||
|
||||
// Consider setStates in the effect's dependency array as being part of effectSetStateUsages
|
||||
for (const dep of dependencies.elements) {
|
||||
if (dep.kind === 'Identifier') {
|
||||
const root = getRootSetState(dep.identifier.id, context.setStateLoads);
|
||||
if (root !== null) {
|
||||
effectSetStateUsages.set(root, new Set([dep.loc]));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let cleanUpFunctionDeps: Set<IdentifierId> | undefined;
|
||||
|
||||
const globals: Set<IdentifierId> = new Set();
|
||||
@@ -662,6 +690,18 @@ function validateEffect(
|
||||
instr.value.args.length === 1 &&
|
||||
instr.value.args[0].kind === 'Identifier'
|
||||
) {
|
||||
const calleeMetadata = context.derivationCache.cache.get(
|
||||
instr.value.callee.identifier.id,
|
||||
);
|
||||
|
||||
/*
|
||||
* If the setState comes from a source other than local state skip
|
||||
* since the fix is not to calculate in render
|
||||
*/
|
||||
if (calleeMetadata?.typeOfValue != 'fromState') {
|
||||
continue;
|
||||
}
|
||||
|
||||
const argMetadata = context.derivationCache.cache.get(
|
||||
instr.value.args[0].identifier.id,
|
||||
);
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
|
||||
|
||||
function Component({prop}) {
|
||||
const [s, setS] = useState(0);
|
||||
useEffect(() => {
|
||||
setS(prop);
|
||||
}, [prop, setS]);
|
||||
|
||||
return <div>{prop}</div>;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Code
|
||||
|
||||
```javascript
|
||||
import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
|
||||
|
||||
function Component(t0) {
|
||||
const $ = _c(5);
|
||||
const { prop } = t0;
|
||||
const [, setS] = useState(0);
|
||||
let t1;
|
||||
let t2;
|
||||
if ($[0] !== prop) {
|
||||
t1 = () => {
|
||||
setS(prop);
|
||||
};
|
||||
t2 = [prop, setS];
|
||||
$[0] = prop;
|
||||
$[1] = t1;
|
||||
$[2] = t2;
|
||||
} else {
|
||||
t1 = $[1];
|
||||
t2 = $[2];
|
||||
}
|
||||
useEffect(t1, t2);
|
||||
let t3;
|
||||
if ($[3] !== prop) {
|
||||
t3 = <div>{prop}</div>;
|
||||
$[3] = prop;
|
||||
$[4] = t3;
|
||||
} else {
|
||||
t3 = $[4];
|
||||
}
|
||||
return t3;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Logs
|
||||
|
||||
```
|
||||
{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nProps: [prop]\n\nData Flow Tree:\n└── prop (Prop)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":6,"column":4,"index":150},"end":{"line":6,"column":8,"index":154},"filename":"effect-used-in-dep-array-still-errors.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null}
|
||||
{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":10,"column":1,"index":212},"filename":"effect-used-in-dep-array-still-errors.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0}
|
||||
```
|
||||
|
||||
### Eval output
|
||||
(kind: exception) Fixture not implemented
|
||||
@@ -0,0 +1,10 @@
|
||||
// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
|
||||
|
||||
function Component({prop}) {
|
||||
const [s, setS] = useState(0);
|
||||
useEffect(() => {
|
||||
setS(prop);
|
||||
}, [prop, setS]);
|
||||
|
||||
return <div>{prop}</div>;
|
||||
}
|
||||
@@ -0,0 +1,65 @@
|
||||
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
|
||||
|
||||
function Component({setParentState, prop}) {
|
||||
useEffect(() => {
|
||||
setParentState(prop);
|
||||
}, [prop]);
|
||||
|
||||
return <div>{prop}</div>;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Code
|
||||
|
||||
```javascript
|
||||
import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
|
||||
|
||||
function Component(t0) {
|
||||
const $ = _c(7);
|
||||
const { setParentState, prop } = t0;
|
||||
let t1;
|
||||
if ($[0] !== prop || $[1] !== setParentState) {
|
||||
t1 = () => {
|
||||
setParentState(prop);
|
||||
};
|
||||
$[0] = prop;
|
||||
$[1] = setParentState;
|
||||
$[2] = t1;
|
||||
} else {
|
||||
t1 = $[2];
|
||||
}
|
||||
let t2;
|
||||
if ($[3] !== prop) {
|
||||
t2 = [prop];
|
||||
$[3] = prop;
|
||||
$[4] = t2;
|
||||
} else {
|
||||
t2 = $[4];
|
||||
}
|
||||
useEffect(t1, t2);
|
||||
let t3;
|
||||
if ($[5] !== prop) {
|
||||
t3 = <div>{prop}</div>;
|
||||
$[5] = prop;
|
||||
$[6] = t3;
|
||||
} else {
|
||||
t3 = $[6];
|
||||
}
|
||||
return t3;
|
||||
}
|
||||
|
||||
```
|
||||
|
||||
## Logs
|
||||
|
||||
```
|
||||
{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":105},"end":{"line":9,"column":1,"index":240},"filename":"from-props-setstate-in-effect-no-error.ts"},"fnName":"Component","memoSlots":7,"memoBlocks":3,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0}
|
||||
```
|
||||
|
||||
### Eval output
|
||||
(kind: exception) Fixture not implemented
|
||||
@@ -0,0 +1,9 @@
|
||||
// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
|
||||
|
||||
function Component({setParentState, prop}) {
|
||||
useEffect(() => {
|
||||
setParentState(prop);
|
||||
}, [prop]);
|
||||
|
||||
return <div>{prop}</div>;
|
||||
}
|
||||
@@ -64,7 +64,6 @@ function Component(t0) {
|
||||
## Logs
|
||||
|
||||
```
|
||||
{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [second]\n\nData Flow Tree:\n└── second (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":14,"column":4,"index":443},"end":{"line":14,"column":8,"index":447},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null}
|
||||
{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":18,"column":1,"index":500},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0}
|
||||
```
|
||||
|
||||
|
||||
Reference in New Issue
Block a user