Skip to content

Commit a5aca27

Browse files
mvitousekmvitousek
committed
[compiler] Bail out and log calls that likely have side effects
ghstack-source-id: a56426e Pull Request resolved: #30572
1 parent 6537652 commit a5aca27

File tree

2 files changed

+188
-5
lines changed

2 files changed

+188
-5
lines changed

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,13 @@ export type HIRFunction = {
294294
};
295295

296296
export type FunctionEffect =
297+
| {
298+
kind: 'ImmutableFunctionCall';
299+
loc: SourceLocation;
300+
lvalue: IdentifierId;
301+
callee: IdentifierId;
302+
global: boolean;
303+
}
297304
| {
298305
kind: 'GlobalMutation';
299306
error: CompilerErrorDetailOptions;

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts‎

Lines changed: 181 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import {
3131
isMutableEffect,
3232
isObjectType,
3333
isRefValueType,
34+
isSetStateType,
3435
isUseRefType,
3536
} from '../HIR/HIR';
3637
import {FunctionSignature} from '../HIR/ObjectShape';
@@ -251,16 +252,133 @@ export default function inferReferenceEffects(
251252
loc: eff.loc,
252253
});
253254
}
255+
case 'ImmutableFunctionCall': {
256+
// Handled below
257+
break;
258+
}
254259
default:
255260
assertExhaustive(
256261
eff,
257262
`Unexpected function effect kind \`${(eff as any).kind}\``,
258263
);
259264
}
260265
});
261-
} else {
262-
fn.effects = functionEffects;
266+
267+
if (functionEffects.length > 0) {
268+
const error = new CompilerError();
269+
let usedIdentifiers = new Set<IdentifierId>();
270+
let names = new Map<IdentifierId, string | undefined>();
271+
272+
function visitFunction(fn: HIRFunction): void {
273+
for (const [, block] of fn.body.blocks) {
274+
for (const instr of block.instructions) {
275+
switch (instr.value.kind) {
276+
case 'FunctionExpression':
277+
case 'ObjectMethod':
278+
names.set(
279+
instr.lvalue.identifier.id,
280+
instr.value.loweredFunc.func.id ?? '(anonymous function)',
281+
);
282+
visitFunction(instr.value.loweredFunc.func);
283+
break;
284+
case 'LoadGlobal':
285+
names.set(instr.lvalue.identifier.id, instr.value.binding.name);
286+
break;
287+
case 'LoadLocal':
288+
case 'LoadContext':
289+
names.set(
290+
instr.lvalue.identifier.id,
291+
instr.value.place.identifier.name?.value ??
292+
names.get(instr.value.place.identifier.id),
293+
);
294+
break;
295+
case 'StoreContext':
296+
case 'StoreLocal':
297+
names.set(
298+
instr.lvalue.identifier.id,
299+
instr.value.value.identifier.name?.value ??
300+
names.get(instr.value.value.identifier.id),
301+
);
302+
names.set(
303+
instr.value.lvalue.place.identifier.id,
304+
instr.value.value.identifier.name?.value ??
305+
names.get(instr.value.value.identifier.id),
306+
);
307+
break;
308+
case 'PropertyLoad':
309+
names.set(
310+
instr.lvalue.identifier.id,
311+
`${instr.value.object.identifier.name?.value ?? names.get(instr.value.object.identifier.id) ?? '(unknown)'}.${instr.value.property}`,
312+
);
313+
break;
314+
case 'ComputedLoad':
315+
names.set(
316+
instr.lvalue.identifier.id,
317+
`${instr.value.object.identifier.name?.value ?? names.get(instr.value.object.identifier.id) ?? '(unknown)'}[...]`,
318+
);
319+
break;
320+
case 'Destructure': {
321+
const destructuredName =
322+
instr.value.value.identifier.name?.value ??
323+
names.get(instr.value.value.identifier.id);
324+
const destructuredMsg = destructuredName
325+
? `(destructured from \`${destructuredName}\`)`
326+
: '(destructured)';
327+
Array.from(
328+
eachPatternOperand(instr.value.lvalue.pattern),
329+
).forEach(place =>
330+
names.set(
331+
place.identifier.id,
332+
`${place.identifier.name?.value ?? 'value'} ${destructuredMsg}`,
333+
),
334+
);
335+
}
336+
}
337+
Array.from(eachInstructionOperand(instr)).forEach(operand =>
338+
usedIdentifiers.add(operand.identifier.id),
339+
);
340+
}
341+
for (const phi of block.phis) {
342+
Array.from(phi.operands.values()).forEach(operand =>
343+
usedIdentifiers.add(operand.id),
344+
);
345+
}
346+
Array.from(eachTerminalOperand(block.terminal)).forEach(operand =>
347+
usedIdentifiers.add(operand.identifier.id),
348+
);
349+
}
350+
}
351+
visitFunction(fn);
352+
353+
const allowedNames = new Set(['invariant', 'recoverableViolation']);
354+
355+
for (const effect of functionEffects) {
356+
CompilerError.invariant(effect.kind === 'ImmutableFunctionCall', {
357+
reason:
358+
'All effects other than ImmutableFunctionCall should have been handled earlier',
359+
loc: null,
360+
});
361+
if (
362+
!usedIdentifiers.has(effect.lvalue) &&
363+
(!effect.global ||
364+
!names.has(effect.callee) ||
365+
!allowedNames.has(names.get(effect.callee)!))
366+
) {
367+
const name = names.get(effect.callee) ?? '(unknown)';
368+
error.push({
369+
reason: `Function \'${name}\' is called with arguments that React Compiler expects to be immutable and its return value is ignored. This call is likely to perform unsafe side effects, which violates the rules of React.`,
370+
loc: effect.loc,
371+
severity: ErrorSeverity.InvalidReact,
372+
});
373+
}
374+
}
375+
376+
if (error.hasErrors()) {
377+
throw error;
378+
}
379+
}
263380
}
381+
fn.effects = functionEffects;
264382
}
265383

266384
// Maintains a mapping of top-level variables to the kind of value they hold
@@ -409,11 +527,12 @@ class InferenceState {
409527
for (const effect of value.loweredFunc.func.effects) {
410528
if (
411529
effect.kind === 'GlobalMutation' ||
412-
effect.kind === 'ReactMutation'
530+
effect.kind === 'ReactMutation' ||
531+
effect.kind === 'ImmutableFunctionCall'
413532
) {
414533
// Known effects are always propagated upwards
415534
functionEffects.push(effect);
416-
} else {
535+
} else if (effect.kind === 'ContextMutation') {
417536
/**
418537
* Contextual effects need to be replayed against the current inference
419538
* state, which may know more about the value to which the effect applied.
@@ -444,6 +563,8 @@ class InferenceState {
444563
} // else case 2, local mutable value so this effect was fine
445564
}
446565
}
566+
} else {
567+
assertExhaustive(effect, `Unexpected function effect kind`);
447568
}
448569
}
449570
}
@@ -1378,6 +1499,32 @@ function inferBlock(
13781499
}
13791500
hasCaptureArgument ||= instrValue.callee.effect === Effect.Capture;
13801501

1502+
if (
1503+
!isSetStateType(instrValue.callee.identifier) &&
1504+
instrValue.callee.effect === Effect.Read &&
1505+
signature?.hookKind == null
1506+
) {
1507+
const allRead = instrValue.args.every(arg => {
1508+
switch (arg.kind) {
1509+
case 'Identifier':
1510+
return arg.effect === Effect.Read;
1511+
case 'Spread':
1512+
return arg.place.effect === Effect.Read;
1513+
default:
1514+
assertExhaustive(arg, 'Unexpected arg kind');
1515+
}
1516+
});
1517+
if (allRead) {
1518+
functionEffects.push({
1519+
kind: 'ImmutableFunctionCall',
1520+
lvalue: instr.lvalue.identifier.id,
1521+
callee: instrValue.callee.identifier.id,
1522+
loc: instrValue.loc,
1523+
global: state.kind(instrValue.callee).kind === ValueKind.Global,
1524+
});
1525+
}
1526+
}
1527+
13811528
state.initialize(instrValue, returnValueKind);
13821529
state.define(instr.lvalue, instrValue);
13831530
instr.lvalue.effect = hasCaptureArgument
@@ -1506,6 +1653,33 @@ function inferBlock(
15061653
}
15071654
hasCaptureArgument ||= instrValue.receiver.effect === Effect.Capture;
15081655

1656+
if (
1657+
!isSetStateType(instrValue.property.identifier) &&
1658+
instrValue.receiver.effect === Effect.Read &&
1659+
instrValue.property.effect === Effect.Read &&
1660+
signature?.hookKind == null
1661+
) {
1662+
const allRead = instrValue.args.every(arg => {
1663+
switch (arg.kind) {
1664+
case 'Identifier':
1665+
return arg.effect === Effect.Read;
1666+
case 'Spread':
1667+
return arg.place.effect === Effect.Read;
1668+
default:
1669+
assertExhaustive(arg, 'Unexpected arg kind');
1670+
}
1671+
});
1672+
if (allRead) {
1673+
functionEffects.push({
1674+
kind: 'ImmutableFunctionCall',
1675+
lvalue: instr.lvalue.identifier.id,
1676+
callee: instrValue.property.identifier.id,
1677+
loc: instrValue.loc,
1678+
global: state.kind(instrValue.property).kind === ValueKind.Global,
1679+
});
1680+
}
1681+
}
1682+
15091683
state.initialize(instrValue, returnValueKind);
15101684
state.define(instr.lvalue, instrValue);
15111685
instr.lvalue.effect = hasCaptureArgument
@@ -2131,7 +2305,9 @@ function areArgumentsImmutableAndNonMutating(
21312305
}
21322306

21332307
function isEffectSafeOutsideRender(effect: FunctionEffect): boolean {
2134-
return effect.kind === 'GlobalMutation';
2308+
return (
2309+
effect.kind === 'GlobalMutation' || effect.kind === 'ImmutableFunctionCall'
2310+
);
21352311
}
21362312

21372313
function getWriteErrorReason(abstractValue: AbstractValue): string {

0 commit comments

Comments
 (0)