Skip to content

Commit 6b22f31

Browse files
authored
[compiler] Aggregate all errors reported from DropManualMemoization (facebook#34002)
Noticed this from my previous PR that this pass was throwing on the first error. This PR is a small refactor to aggregate every violation and report them all at once. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34002). * facebook#34022 * __->__ facebook#34002
1 parent b5c1637 commit 6b22f31

File tree

3 files changed

+80
-29
lines changed

3 files changed

+80
-29
lines changed

compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts

Lines changed: 76 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -289,50 +289,84 @@ function extractManualMemoizationArgs(
289289
instr: TInstruction<CallExpression> | TInstruction<MethodCall>,
290290
kind: 'useCallback' | 'useMemo',
291291
sidemap: IdentifierSidemap,
292+
errors: CompilerError,
292293
): {
293-
fnPlace: Place;
294+
fnPlace: Place | null;
294295
depsList: Array<ManualMemoDependency> | null;
295296
} {
296297
const [fnPlace, depsListPlace] = instr.value.args as Array<
297298
Place | SpreadPattern | undefined
298299
>;
299300
if (fnPlace == null) {
300-
CompilerError.throwInvalidReact({
301-
reason: `Expected a callback function to be passed to ${kind}`,
302-
loc: instr.value.loc,
303-
suggestions: null,
304-
});
301+
errors.pushDiagnostic(
302+
CompilerDiagnostic.create({
303+
severity: ErrorSeverity.InvalidReact,
304+
category: `Expected a callback function to be passed to ${kind}`,
305+
description: `Expected a callback function to be passed to ${kind}`,
306+
suggestions: null,
307+
}).withDetail({
308+
kind: 'error',
309+
loc: instr.value.loc,
310+
message: `Expected a callback function to be passed to ${kind}`,
311+
}),
312+
);
313+
return {fnPlace: null, depsList: null};
305314
}
306315
if (fnPlace.kind === 'Spread' || depsListPlace?.kind === 'Spread') {
307-
CompilerError.throwInvalidReact({
308-
reason: `Unexpected spread argument to ${kind}`,
309-
loc: instr.value.loc,
310-
suggestions: null,
311-
});
316+
errors.pushDiagnostic(
317+
CompilerDiagnostic.create({
318+
severity: ErrorSeverity.InvalidReact,
319+
category: `Unexpected spread argument to ${kind}`,
320+
description: `Unexpected spread argument to ${kind}`,
321+
suggestions: null,
322+
}).withDetail({
323+
kind: 'error',
324+
loc: instr.value.loc,
325+
message: `Unexpected spread argument to ${kind}`,
326+
}),
327+
);
328+
return {fnPlace: null, depsList: null};
312329
}
313330
let depsList: Array<ManualMemoDependency> | null = null;
314331
if (depsListPlace != null) {
315332
const maybeDepsList = sidemap.maybeDepsLists.get(
316333
depsListPlace.identifier.id,
317334
);
318335
if (maybeDepsList == null) {
319-
CompilerError.throwInvalidReact({
320-
reason: `Expected the dependency list for ${kind} to be an array literal`,
321-
suggestions: null,
322-
loc: depsListPlace.loc,
323-
});
336+
errors.pushDiagnostic(
337+
CompilerDiagnostic.create({
338+
severity: ErrorSeverity.InvalidReact,
339+
category: `Expected the dependency list for ${kind} to be an array literal`,
340+
description: `Expected the dependency list for ${kind} to be an array literal`,
341+
suggestions: null,
342+
}).withDetail({
343+
kind: 'error',
344+
loc: depsListPlace.loc,
345+
message: `Expected the dependency list for ${kind} to be an array literal`,
346+
}),
347+
);
348+
return {fnPlace, depsList: null};
324349
}
325-
depsList = maybeDepsList.map(dep => {
350+
depsList = [];
351+
for (const dep of maybeDepsList) {
326352
const maybeDep = sidemap.maybeDeps.get(dep.identifier.id);
327353
if (maybeDep == null) {
328-
CompilerError.throwInvalidReact({
329-
reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
330-
suggestions: null,
331-
loc: dep.loc,
332-
});
354+
errors.pushDiagnostic(
355+
CompilerDiagnostic.create({
356+
severity: ErrorSeverity.InvalidReact,
357+
category: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
358+
description: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
359+
suggestions: null,
360+
}).withDetail({
361+
kind: 'error',
362+
loc: dep.loc,
363+
message: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`,
364+
}),
365+
);
366+
} else {
367+
depsList.push(maybeDep);
333368
}
334-
return maybeDep;
335-
});
369+
}
336370
}
337371
return {
338372
fnPlace,
@@ -401,8 +435,13 @@ export function dropManualMemoization(
401435
instr as TInstruction<CallExpression> | TInstruction<MethodCall>,
402436
manualMemo.kind,
403437
sidemap,
438+
errors,
404439
);
405440

441+
if (fnPlace == null) {
442+
continue;
443+
}
444+
406445
/**
407446
* Bailout on void return useMemos. This is an anti-pattern where code might be using
408447
* useMemo like useEffect: running arbirtary side-effects synced to changes in specific
@@ -457,11 +496,19 @@ export function dropManualMemoization(
457496
* is rare and likely sketchy.
458497
*/
459498
if (!sidemap.functions.has(fnPlace.identifier.id)) {
460-
CompilerError.throwInvalidReact({
461-
reason: `Expected the first argument to be an inline function expression`,
462-
suggestions: [],
463-
loc: fnPlace.loc,
464-
});
499+
errors.pushDiagnostic(
500+
CompilerDiagnostic.create({
501+
severity: ErrorSeverity.InvalidReact,
502+
category: `Expected the first argument to be an inline function expression`,
503+
description: `Expected the first argument to be an inline function expression`,
504+
suggestions: [],
505+
}).withDetail({
506+
kind: 'error',
507+
loc: fnPlace.loc,
508+
message: `Expected the first argument to be an inline function expression`,
509+
}),
510+
);
511+
continue;
465512
}
466513
const memoDecl: Place =
467514
manualMemo.kind === 'useMemo'

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-non-literal-depslist.expect.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ Found 1 error:
3232
3333
Error: Expected the dependency list for useMemo to be an array literal
3434
35+
Expected the dependency list for useMemo to be an array literal
36+
3537
error.useMemo-non-literal-depslist.ts:10:4
3638
8 | return text.toUpperCase();
3739
9 | },

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.validate-useMemo-named-function.expect.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ Found 1 error:
2424
2525
Error: Expected the first argument to be an inline function expression
2626
27+
Expected the first argument to be an inline function expression
28+
2729
error.validate-useMemo-named-function.ts:9:20
2830
7 | // for now.
2931
8 | function Component(props) {

0 commit comments

Comments
 (0)