Skip to content

Commit bdfee6a

Browse files
jorge-cabjorge-cab
committed
[Compiler] Improve error for calculate in render useEffect validation (#34580)
Summary: Change error and update snapshots The error now mentions what values are causing the issue which should provide better context on how to fix the issue --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34580). * __->__ #34580 * #34579 * #34578 * #34577 * #34575 * #34574 DiffTrain build for [09056ab](09056ab)
1 parent 76beb64 commit bdfee6a

35 files changed

+425
-88
lines changed

‎compiled/eslint-plugin-react-hooks/index.js‎

Lines changed: 339 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32144,6 +32144,7 @@ const EnvironmentConfigSchema = v4.z.object({
3214432144
validateNoSetStateInRender: v4.z.boolean().default(true),
3214532145
validateNoSetStateInEffects: v4.z.boolean().default(false),
3214632146
validateNoDerivedComputationsInEffects: v4.z.boolean().default(false),
32147+
validateNoDerivedComputationsInEffects_exp: v4.z.boolean().default(false),
3214732148
validateNoJSXInTryStatements: v4.z.boolean().default(false),
3214832149
validateStaticComponents: v4.z.boolean().default(false),
3214932150
validateMemoizedEffectDependencies: v4.z.boolean().default(false),
@@ -52086,7 +52087,7 @@ function validateNoDerivedComputationsInEffects(fn) {
5208652087
});
5208752088
return (_a = locals.get(dep.identifier.id)) !== null && _a !== void 0 ? _a : dep.identifier.id;
5208852089
});
52089-
validateEffect(effectFunction.loweredFunc.func, dependencies, errors);
52090+
validateEffect$1(effectFunction.loweredFunc.func, dependencies, errors);
5209052091
}
5209152092
}
5209252093
}
@@ -52096,7 +52097,7 @@ function validateNoDerivedComputationsInEffects(fn) {
5209652097
throw errors;
5209752098
}
5209852099
}
52099-
function validateEffect(effectFunction, effectDeps, errors) {
52100+
function validateEffect$1(effectFunction, effectDeps, errors) {
5210052101
for (const operand of effectFunction.context) {
5210152102
if (isSetStateType(operand.identifier)) {
5210252103
continue;
@@ -52209,6 +52210,339 @@ function validateEffect(effectFunction, effectDeps, errors) {
5220952210
}
5221052211
}
5221152212

52213+
class DerivationCache {
52214+
constructor() {
52215+
this.hasChanges = false;
52216+
this.cache = new Map();
52217+
}
52218+
snapshot() {
52219+
const hasChanges = this.hasChanges;
52220+
this.hasChanges = false;
52221+
return hasChanges;
52222+
}
52223+
addDerivationEntry(derivedVar, sourcesIds, typeOfValue) {
52224+
var _a, _b;
52225+
let newValue = {
52226+
place: derivedVar,
52227+
sourcesIds: new Set(),
52228+
typeOfValue: typeOfValue !== null && typeOfValue !== void 0 ? typeOfValue : 'ignored',
52229+
};
52230+
if (sourcesIds !== undefined) {
52231+
for (const id of sourcesIds) {
52232+
const sourcePlace = (_a = this.cache.get(id)) === null || _a === void 0 ? void 0 : _a.place;
52233+
if (sourcePlace === undefined) {
52234+
continue;
52235+
}
52236+
if (sourcePlace.identifier.name === null ||
52237+
((_b = sourcePlace.identifier.name) === null || _b === void 0 ? void 0 : _b.kind) === 'promoted') {
52238+
newValue.sourcesIds.add(derivedVar.identifier.id);
52239+
}
52240+
else {
52241+
newValue.sourcesIds.add(sourcePlace.identifier.id);
52242+
}
52243+
}
52244+
}
52245+
if (newValue.sourcesIds.size === 0) {
52246+
newValue.sourcesIds.add(derivedVar.identifier.id);
52247+
}
52248+
const existingValue = this.cache.get(derivedVar.identifier.id);
52249+
if (existingValue === undefined ||
52250+
!this.isDerivationEqual(existingValue, newValue)) {
52251+
this.cache.set(derivedVar.identifier.id, newValue);
52252+
this.hasChanges = true;
52253+
}
52254+
}
52255+
isDerivationEqual(a, b) {
52256+
if (a.typeOfValue !== b.typeOfValue) {
52257+
return false;
52258+
}
52259+
if (a.sourcesIds.size !== b.sourcesIds.size) {
52260+
return false;
52261+
}
52262+
for (const id of a.sourcesIds) {
52263+
if (!b.sourcesIds.has(id)) {
52264+
return false;
52265+
}
52266+
}
52267+
return true;
52268+
}
52269+
}
52270+
function validateNoDerivedComputationsInEffects_exp(fn) {
52271+
const functions = new Map();
52272+
const derivationCache = new DerivationCache();
52273+
const errors = new CompilerError();
52274+
const effects = new Set();
52275+
const setStateCache = new Map();
52276+
const effectSetStateCache = new Map();
52277+
const context = {
52278+
functions,
52279+
errors,
52280+
derivationCache,
52281+
effects,
52282+
setStateCache,
52283+
effectSetStateCache,
52284+
};
52285+
if (fn.fnType === 'Hook') {
52286+
for (const param of fn.params) {
52287+
if (param.kind === 'Identifier') {
52288+
context.derivationCache.cache.set(param.identifier.id, {
52289+
place: param,
52290+
sourcesIds: new Set([param.identifier.id]),
52291+
typeOfValue: 'fromProps',
52292+
});
52293+
context.derivationCache.hasChanges = true;
52294+
}
52295+
}
52296+
}
52297+
else if (fn.fnType === 'Component') {
52298+
const props = fn.params[0];
52299+
if (props != null && props.kind === 'Identifier') {
52300+
context.derivationCache.cache.set(props.identifier.id, {
52301+
place: props,
52302+
sourcesIds: new Set([props.identifier.id]),
52303+
typeOfValue: 'fromProps',
52304+
});
52305+
context.derivationCache.hasChanges = true;
52306+
}
52307+
}
52308+
let isFirstPass = true;
52309+
do {
52310+
for (const block of fn.body.blocks.values()) {
52311+
recordPhiDerivations(block, context);
52312+
for (const instr of block.instructions) {
52313+
recordInstructionDerivations(instr, context, isFirstPass);
52314+
}
52315+
}
52316+
isFirstPass = false;
52317+
} while (context.derivationCache.snapshot());
52318+
for (const effect of effects) {
52319+
validateEffect(effect, context);
52320+
}
52321+
if (errors.hasAnyErrors()) {
52322+
throw errors;
52323+
}
52324+
}
52325+
function recordPhiDerivations(block, context) {
52326+
for (const phi of block.phis) {
52327+
let typeOfValue = 'ignored';
52328+
let sourcesIds = new Set();
52329+
for (const operand of phi.operands.values()) {
52330+
const operandMetadata = context.derivationCache.cache.get(operand.identifier.id);
52331+
if (operandMetadata === undefined) {
52332+
continue;
52333+
}
52334+
typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue);
52335+
sourcesIds.add(operand.identifier.id);
52336+
}
52337+
if (typeOfValue !== 'ignored') {
52338+
context.derivationCache.addDerivationEntry(phi.place, sourcesIds, typeOfValue);
52339+
}
52340+
}
52341+
}
52342+
function joinValue(lvalueType, valueType) {
52343+
if (lvalueType === 'ignored')
52344+
return valueType;
52345+
if (valueType === 'ignored')
52346+
return lvalueType;
52347+
if (lvalueType === valueType)
52348+
return lvalueType;
52349+
return 'fromPropsAndState';
52350+
}
52351+
function recordInstructionDerivations(instr, context, isFirstPass) {
52352+
let typeOfValue = 'ignored';
52353+
const sources = new Set();
52354+
const { lvalue, value } = instr;
52355+
if (value.kind === 'FunctionExpression') {
52356+
context.functions.set(lvalue.identifier.id, value);
52357+
for (const [, block] of value.loweredFunc.func.body.blocks) {
52358+
for (const instr of block.instructions) {
52359+
recordInstructionDerivations(instr, context, isFirstPass);
52360+
}
52361+
}
52362+
}
52363+
else if (value.kind === 'CallExpression' || value.kind === 'MethodCall') {
52364+
const callee = value.kind === 'CallExpression' ? value.callee : value.property;
52365+
if (isUseEffectHookType(callee.identifier) &&
52366+
value.args.length === 2 &&
52367+
value.args[0].kind === 'Identifier' &&
52368+
value.args[1].kind === 'Identifier') {
52369+
const effectFunction = context.functions.get(value.args[0].identifier.id);
52370+
if (effectFunction != null) {
52371+
context.effects.add(effectFunction.loweredFunc.func);
52372+
}
52373+
}
52374+
else if (isUseStateType(lvalue.identifier) && value.args.length > 0) {
52375+
const stateValueSource = value.args[0];
52376+
if (stateValueSource.kind === 'Identifier') {
52377+
sources.add(stateValueSource.identifier.id);
52378+
}
52379+
typeOfValue = joinValue(typeOfValue, 'fromState');
52380+
}
52381+
}
52382+
for (const operand of eachInstructionOperand(instr)) {
52383+
if (isSetStateType(operand.identifier) &&
52384+
operand.loc !== GeneratedSource &&
52385+
isFirstPass) {
52386+
if (context.setStateCache.has(operand.loc.identifierName)) {
52387+
context.setStateCache.get(operand.loc.identifierName).push(operand);
52388+
}
52389+
else {
52390+
context.setStateCache.set(operand.loc.identifierName, [operand]);
52391+
}
52392+
}
52393+
const operandMetadata = context.derivationCache.cache.get(operand.identifier.id);
52394+
if (operandMetadata === undefined) {
52395+
continue;
52396+
}
52397+
typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue);
52398+
for (const id of operandMetadata.sourcesIds) {
52399+
sources.add(id);
52400+
}
52401+
}
52402+
if (typeOfValue === 'ignored') {
52403+
return;
52404+
}
52405+
for (const lvalue of eachInstructionLValue(instr)) {
52406+
context.derivationCache.addDerivationEntry(lvalue, sources, typeOfValue);
52407+
}
52408+
for (const operand of eachInstructionOperand(instr)) {
52409+
switch (operand.effect) {
52410+
case Effect.Capture:
52411+
case Effect.Store:
52412+
case Effect.ConditionallyMutate:
52413+
case Effect.ConditionallyMutateIterator:
52414+
case Effect.Mutate: {
52415+
if (isMutable(instr, operand)) {
52416+
context.derivationCache.addDerivationEntry(operand, sources, typeOfValue);
52417+
}
52418+
break;
52419+
}
52420+
case Effect.Freeze:
52421+
case Effect.Read: {
52422+
break;
52423+
}
52424+
case Effect.Unknown: {
52425+
CompilerError.invariant(false, {
52426+
reason: 'Unexpected unknown effect',
52427+
description: null,
52428+
details: [
52429+
{
52430+
kind: 'error',
52431+
loc: operand.loc,
52432+
message: 'Unexpected unknown effect',
52433+
},
52434+
],
52435+
});
52436+
}
52437+
default: {
52438+
assertExhaustive$1(operand.effect, `Unexpected effect kind \`${operand.effect}\``);
52439+
}
52440+
}
52441+
}
52442+
}
52443+
function validateEffect(effectFunction, context) {
52444+
const seenBlocks = new Set();
52445+
const effectDerivedSetStateCalls = [];
52446+
const globals = new Set();
52447+
for (const block of effectFunction.body.blocks.values()) {
52448+
for (const pred of block.preds) {
52449+
if (!seenBlocks.has(pred)) {
52450+
return;
52451+
}
52452+
}
52453+
for (const instr of block.instructions) {
52454+
if (isUseRefType(instr.lvalue.identifier)) {
52455+
return;
52456+
}
52457+
for (const operand of eachInstructionOperand(instr)) {
52458+
if (isSetStateType(operand.identifier) &&
52459+
operand.loc !== GeneratedSource) {
52460+
if (context.effectSetStateCache.has(operand.loc.identifierName)) {
52461+
context.effectSetStateCache
52462+
.get(operand.loc.identifierName)
52463+
.push(operand);
52464+
}
52465+
else {
52466+
context.effectSetStateCache.set(operand.loc.identifierName, [
52467+
operand,
52468+
]);
52469+
}
52470+
}
52471+
}
52472+
if (instr.value.kind === 'CallExpression' &&
52473+
isSetStateType(instr.value.callee.identifier) &&
52474+
instr.value.args.length === 1 &&
52475+
instr.value.args[0].kind === 'Identifier') {
52476+
const argMetadata = context.derivationCache.cache.get(instr.value.args[0].identifier.id);
52477+
if (argMetadata !== undefined) {
52478+
effectDerivedSetStateCalls.push({
52479+
value: instr.value,
52480+
loc: instr.value.callee.loc,
52481+
sourceIds: argMetadata.sourcesIds,
52482+
typeOfValue: argMetadata.typeOfValue,
52483+
});
52484+
}
52485+
}
52486+
else if (instr.value.kind === 'CallExpression') {
52487+
const calleeMetadata = context.derivationCache.cache.get(instr.value.callee.identifier.id);
52488+
if (calleeMetadata !== undefined &&
52489+
(calleeMetadata.typeOfValue === 'fromProps' ||
52490+
calleeMetadata.typeOfValue === 'fromPropsAndState')) {
52491+
return;
52492+
}
52493+
if (globals.has(instr.value.callee.identifier.id)) {
52494+
return;
52495+
}
52496+
}
52497+
else if (instr.value.kind === 'LoadGlobal') {
52498+
globals.add(instr.lvalue.identifier.id);
52499+
for (const operand of eachInstructionOperand(instr)) {
52500+
globals.add(operand.identifier.id);
52501+
}
52502+
}
52503+
}
52504+
seenBlocks.add(block.id);
52505+
}
52506+
for (const derivedSetStateCall of effectDerivedSetStateCalls) {
52507+
if (derivedSetStateCall.loc !== GeneratedSource &&
52508+
context.effectSetStateCache.has(derivedSetStateCall.loc.identifierName) &&
52509+
context.setStateCache.has(derivedSetStateCall.loc.identifierName) &&
52510+
context.effectSetStateCache.get(derivedSetStateCall.loc.identifierName)
52511+
.length ===
52512+
context.setStateCache.get(derivedSetStateCall.loc.identifierName)
52513+
.length -
52514+
1) {
52515+
const derivedDepsStr = Array.from(derivedSetStateCall.sourceIds)
52516+
.map(sourceId => {
52517+
var _a;
52518+
const sourceMetadata = context.derivationCache.cache.get(sourceId);
52519+
return (_a = sourceMetadata === null || sourceMetadata === void 0 ? void 0 : sourceMetadata.place.identifier.name) === null || _a === void 0 ? void 0 : _a.value;
52520+
})
52521+
.filter(Boolean)
52522+
.join(', ');
52523+
let description;
52524+
if (derivedSetStateCall.typeOfValue === 'fromProps') {
52525+
description = `From props: [${derivedDepsStr}]`;
52526+
}
52527+
else if (derivedSetStateCall.typeOfValue === 'fromState') {
52528+
description = `From local state: [${derivedDepsStr}]`;
52529+
}
52530+
else {
52531+
description = `From props and local state: [${derivedDepsStr}]`;
52532+
}
52533+
context.errors.pushDiagnostic(CompilerDiagnostic.create({
52534+
description: `Derived values (${description}) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user`,
52535+
category: ErrorCategory.EffectDerivationsOfState,
52536+
reason: 'You might not need an effect. Derive values in render, not effects.',
52537+
}).withDetails({
52538+
kind: 'error',
52539+
loc: derivedSetStateCall.value.callee.loc,
52540+
message: 'This should be computed during render, not in an effect',
52541+
}));
52542+
}
52543+
}
52544+
}
52545+
5221252546
function nameAnonymousFunctions(fn) {
5221352547
if (fn.id == null) {
5221452548
return;
@@ -52450,6 +52784,9 @@ function runWithEnvironment(func, env) {
5245052784
if (env.config.validateNoDerivedComputationsInEffects) {
5245152785
validateNoDerivedComputationsInEffects(hir);
5245252786
}
52787+
if (env.config.validateNoDerivedComputationsInEffects_exp) {
52788+
validateNoDerivedComputationsInEffects_exp(hir);
52789+
}
5245352790
if (env.config.validateNoSetStateInEffects) {
5245452791
env.logErrors(validateNoSetStateInEffects(hir, env));
5245552792
}

‎compiled/facebook-www/REVISION‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
ea0c17b0952e2b866a1f0dd4b5ac28c7df9d8518
1+
09056abde76c464f4632f322a0ac30cd3984cee6

‎compiled/facebook-www/REVISION_TRANSFORMS‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
ea0c17b0952e2b866a1f0dd4b5ac28c7df9d8518
1+
09056abde76c464f4632f322a0ac30cd3984cee6

‎compiled/facebook-www/React-dev.classic.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ __DEV__ &&
14991499
exports.useTransition = function () {
15001500
return resolveDispatcher().useTransition();
15011501
};
1502-
exports.version = "19.3.0-www-classic-ea0c17b0-20251020";
1502+
exports.version = "19.3.0-www-classic-09056abd-20251023";
15031503
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
15041504
"function" ===
15051505
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

‎compiled/facebook-www/React-dev.modern.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ __DEV__ &&
14991499
exports.useTransition = function () {
15001500
return resolveDispatcher().useTransition();
15011501
};
1502-
exports.version = "19.3.0-www-modern-ea0c17b0-20251020";
1502+
exports.version = "19.3.0-www-modern-09056abd-20251023";
15031503
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
15041504
"function" ===
15051505
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

0 commit comments

Comments
 (0)