Skip to content

Commit e5169ca

Browse files
mvitousekmvitousek
committed
[compiler] Bail out and log calls that likely have side effects
ghstack-source-id: 19c23b6 Pull Request resolved: #30572
1 parent 06d0b89 commit e5169ca

File tree

2 files changed

+196
-7
lines changed

2 files changed

+196
-7
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: 189 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
isMutableEffect,
3131
isObjectType,
3232
isRefValueType,
33+
isSetStateType,
3334
isUseRefType,
3435
} from '../HIR/HIR';
3536
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
}
@@ -1151,7 +1272,9 @@ function inferBlock(
11511272
);
11521273
functionEffects.push(
11531274
...propEffects.filter(
1154-
propEffect => propEffect.kind !== 'GlobalMutation',
1275+
propEffect =>
1276+
propEffect.kind !== 'GlobalMutation' &&
1277+
propEffect.kind !== 'ImmutableFunctionCall',
11551278
),
11561279
);
11571280
}
@@ -1357,7 +1480,10 @@ function inferBlock(
13571480
functionEffects.push(
13581481
...argumentEffects.filter(
13591482
argEffect =>
1360-
!isUseEffect || i !== 0 || argEffect.kind !== 'GlobalMutation',
1483+
!isUseEffect ||
1484+
i !== 0 ||
1485+
(argEffect.kind !== 'GlobalMutation' &&
1486+
argEffect.kind !== 'ImmutableFunctionCall'),
13611487
),
13621488
);
13631489
hasCaptureArgument ||= place.effect === Effect.Capture;
@@ -1379,6 +1505,32 @@ function inferBlock(
13791505
}
13801506
hasCaptureArgument ||= instrValue.callee.effect === Effect.Capture;
13811507

1508+
if (
1509+
!isSetStateType(instrValue.callee.identifier) &&
1510+
instrValue.callee.effect === Effect.Read &&
1511+
signature?.hookKind == null
1512+
) {
1513+
const allRead = instrValue.args.every(arg => {
1514+
switch (arg.kind) {
1515+
case 'Identifier':
1516+
return arg.effect === Effect.Read;
1517+
case 'Spread':
1518+
return arg.place.effect === Effect.Read;
1519+
default:
1520+
assertExhaustive(arg, 'Unexpected arg kind');
1521+
}
1522+
});
1523+
if (allRead) {
1524+
functionEffects.push({
1525+
kind: 'ImmutableFunctionCall',
1526+
lvalue: instr.lvalue.identifier.id,
1527+
callee: instrValue.callee.identifier.id,
1528+
loc: instrValue.loc,
1529+
global: state.kind(instrValue.callee).kind === ValueKind.Global,
1530+
});
1531+
}
1532+
}
1533+
13821534
state.initialize(instrValue, returnValueKind);
13831535
state.define(instr.lvalue, instrValue);
13841536
instr.lvalue.effect = hasCaptureArgument
@@ -1486,7 +1638,10 @@ function inferBlock(
14861638
functionEffects.push(
14871639
...argumentEffects.filter(
14881640
argEffect =>
1489-
!isUseEffect || i !== 0 || argEffect.kind !== 'GlobalMutation',
1641+
!isUseEffect ||
1642+
i !== 0 ||
1643+
(argEffect.kind !== 'GlobalMutation' &&
1644+
argEffect.kind !== 'ImmutableFunctionCall'),
14901645
),
14911646
);
14921647
hasCaptureArgument ||= place.effect === Effect.Capture;
@@ -1508,6 +1663,33 @@ function inferBlock(
15081663
}
15091664
hasCaptureArgument ||= instrValue.receiver.effect === Effect.Capture;
15101665

1666+
if (
1667+
!isSetStateType(instrValue.property.identifier) &&
1668+
instrValue.receiver.effect === Effect.Read &&
1669+
instrValue.property.effect === Effect.Read &&
1670+
signature?.hookKind == null
1671+
) {
1672+
const allRead = instrValue.args.every(arg => {
1673+
switch (arg.kind) {
1674+
case 'Identifier':
1675+
return arg.effect === Effect.Read;
1676+
case 'Spread':
1677+
return arg.place.effect === Effect.Read;
1678+
default:
1679+
assertExhaustive(arg, 'Unexpected arg kind');
1680+
}
1681+
});
1682+
if (allRead) {
1683+
functionEffects.push({
1684+
kind: 'ImmutableFunctionCall',
1685+
lvalue: instr.lvalue.identifier.id,
1686+
callee: instrValue.property.identifier.id,
1687+
loc: instrValue.loc,
1688+
global: state.kind(instrValue.property).kind === ValueKind.Global,
1689+
});
1690+
}
1691+
}
1692+
15111693
state.initialize(instrValue, returnValueKind);
15121694
state.define(instr.lvalue, instrValue);
15131695
instr.lvalue.effect = hasCaptureArgument

0 commit comments

Comments
 (0)