diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 5440035183d42..55bcd0bc5ce89 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -817,6 +817,11 @@ export type StartMemoize = { * (e.g. useMemo without a second arg) */ deps: Array | null; + /** + * The source location of the dependencies argument. Used for + * emitting diagnostics with a suggested replacement + */ + depsLoc: SourceLocation | null; loc: SourceLocation; }; export type FinishMemoize = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts index 0876424e28c14..bcfc53413ab41 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -42,7 +42,7 @@ type IdentifierSidemap = { functions: Map>; manualMemos: Map; react: Set; - maybeDepsLists: Map>; + maybeDepsLists: Map}>; maybeDeps: Map; optionals: Set; }; @@ -159,10 +159,10 @@ function collectTemporaries( } case 'ArrayExpression': { if (value.elements.every(e => e.kind === 'Identifier')) { - sidemap.maybeDepsLists.set( - instr.lvalue.identifier.id, - value.elements as Array, - ); + sidemap.maybeDepsLists.set(instr.lvalue.identifier.id, { + loc: value.loc, + deps: value.elements as Array, + }); } break; } @@ -182,6 +182,7 @@ function makeManualMemoizationMarkers( fnExpr: Place, env: Environment, depsList: Array | null, + depsLoc: SourceLocation | null, memoDecl: Place, manualMemoId: number, ): [TInstruction, TInstruction] { @@ -197,6 +198,7 @@ function makeManualMemoizationMarkers( * as dependencies */ deps: depsList, + depsLoc, loc: fnExpr.loc, }, effects: null, @@ -287,86 +289,85 @@ function extractManualMemoizationArgs( sidemap: IdentifierSidemap, errors: CompilerError, ): { - fnPlace: Place | null; + fnPlace: Place; depsList: Array | null; -} { + depsLoc: SourceLocation | null; +} | null { const [fnPlace, depsListPlace] = instr.value.args as Array< Place | SpreadPattern | undefined >; - if (fnPlace == null) { + if (fnPlace == null || fnPlace.kind !== 'Identifier') { errors.pushDiagnostic( CompilerDiagnostic.create({ category: ErrorCategory.UseMemo, reason: `Expected a callback function to be passed to ${kind}`, - description: `Expected a callback function to be passed to ${kind}`, + description: + kind === 'useCallback' + ? 'The first argument to useCallback() must be a function to cache' + : 'The first argument to useMemo() must be a function that calculates a result to cache', suggestions: null, }).withDetails({ kind: 'error', loc: instr.value.loc, - message: `Expected a callback function to be passed to ${kind}`, + message: + kind === 'useCallback' + ? `Expected a callback function` + : `Expected a memoization function`, }), ); - return {fnPlace: null, depsList: null}; + return null; } - if (fnPlace.kind === 'Spread' || depsListPlace?.kind === 'Spread') { + if (depsListPlace == null) { + return { + fnPlace, + depsList: null, + depsLoc: null, + }; + } + const maybeDepsList = + depsListPlace.kind === 'Identifier' + ? sidemap.maybeDepsLists.get(depsListPlace.identifier.id) + : null; + if (maybeDepsList == null) { errors.pushDiagnostic( CompilerDiagnostic.create({ category: ErrorCategory.UseMemo, - reason: `Unexpected spread argument to ${kind}`, - description: `Unexpected spread argument to ${kind}`, + reason: `Expected the dependency list for ${kind} to be an array literal`, + description: `Expected the dependency list for ${kind} to be an array literal`, suggestions: null, }).withDetails({ kind: 'error', - loc: instr.value.loc, - message: `Unexpected spread argument to ${kind}`, + loc: + depsListPlace?.kind === 'Identifier' ? depsListPlace.loc : instr.loc, + message: `Expected the dependency list for ${kind} to be an array literal`, }), ); - return {fnPlace: null, depsList: null}; + return null; } - let depsList: Array | null = null; - if (depsListPlace != null) { - const maybeDepsList = sidemap.maybeDepsLists.get( - depsListPlace.identifier.id, - ); - if (maybeDepsList == null) { + const depsList: Array = []; + for (const dep of maybeDepsList.deps) { + const maybeDep = sidemap.maybeDeps.get(dep.identifier.id); + if (maybeDep == null) { errors.pushDiagnostic( CompilerDiagnostic.create({ category: ErrorCategory.UseMemo, - reason: `Expected the dependency list for ${kind} to be an array literal`, - description: `Expected the dependency list for ${kind} to be an array literal`, + reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, + description: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, suggestions: null, }).withDetails({ kind: 'error', - loc: depsListPlace.loc, - message: `Expected the dependency list for ${kind} to be an array literal`, + loc: dep.loc, + message: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, }), ); - return {fnPlace, depsList: null}; - } - depsList = []; - for (const dep of maybeDepsList) { - const maybeDep = sidemap.maybeDeps.get(dep.identifier.id); - if (maybeDep == null) { - errors.pushDiagnostic( - CompilerDiagnostic.create({ - category: ErrorCategory.UseMemo, - reason: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, - description: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, - suggestions: null, - }).withDetails({ - kind: 'error', - loc: dep.loc, - message: `Expected the dependency list to be an array of simple expressions (e.g. \`x\`, \`x.y.z\`, \`x?.y?.z\`)`, - }), - ); - } else { - depsList.push(maybeDep); - } + } else { + depsList.push(maybeDep); } } return { fnPlace, depsList, + depsLoc: maybeDepsList.loc, }; } @@ -427,16 +428,17 @@ export function dropManualMemoization( const manualMemo = sidemap.manualMemos.get(id); if (manualMemo != null) { - const {fnPlace, depsList} = extractManualMemoizationArgs( + const memoDetails = extractManualMemoizationArgs( instr as TInstruction | TInstruction, manualMemo.kind, sidemap, errors, ); - if (fnPlace == null) { + if (memoDetails == null) { continue; } + const {fnPlace, depsList, depsLoc} = memoDetails; instr.value = getManualMemoizationReplacement( fnPlace, @@ -487,6 +489,7 @@ export function dropManualMemoization( fnPlace, func.env, depsList, + depsLoc, memoDecl, nextManualMemoId++, ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts index ad9b83e392753..fb1f9445aa0d3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -6,8 +6,13 @@ */ import prettyFormat from 'pretty-format'; -import {CompilerDiagnostic, CompilerError, SourceLocation} from '..'; -import {ErrorCategory} from '../CompilerError'; +import { + CompilerDiagnostic, + CompilerError, + CompilerSuggestionOperation, + SourceLocation, +} from '..'; +import {CompilerSuggestion, ErrorCategory} from '../CompilerError'; import { areEqualPaths, BlockId, @@ -229,38 +234,52 @@ export function validateExhaustiveDependencies( extra.push(dep); } - if (missing.length !== 0) { - // Error - const diagnostic = CompilerDiagnostic.create({ - category: ErrorCategory.PreserveManualMemo, - reason: 'Found non-exhaustive dependencies', - description: - 'Missing dependencies can cause a value not to update when those inputs change, ' + - 'resulting in stale UI. This memoization cannot be safely rewritten by the compiler.', - }); - for (const dep of missing) { + if (missing.length !== 0 || extra.length !== 0) { + let suggestions: Array | null = null; + if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') { + suggestions = [ + { + description: 'Update dependencies', + range: [startMemo.depsLoc.start.index, startMemo.depsLoc.end.index], + op: CompilerSuggestionOperation.Replace, + text: `[${inferred.map(printInferredDependency).join(', ')}]`, + }, + ]; + } + if (missing.length !== 0) { + // Error + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + reason: 'Found non-exhaustive dependencies', + description: + 'Missing dependencies can cause a value not to update when those inputs change, ' + + 'resulting in stale UI. This memoization cannot be safely rewritten by the compiler.', + suggestions, + }); + for (const dep of missing) { + diagnostic.withDetails({ + kind: 'error', + message: `Missing dependency \`${printInferredDependency(dep)}\``, + loc: dep.loc, + }); + } + error.pushDiagnostic(diagnostic); + } else if (extra.length !== 0) { + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.PreserveManualMemo, + reason: 'Found unnecessary memoization dependencies', + description: + 'Unnecessary dependencies can cause a value to update more often than necessary, ' + + 'which can cause effects to run more than expected. This memoization cannot be safely ' + + 'rewritten by the compiler', + }); diagnostic.withDetails({ kind: 'error', - message: `Missing dependency \`${printInferredDependency(dep)}\``, - loc: dep.loc, + message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`, + loc: value.loc, }); + error.pushDiagnostic(diagnostic); } - error.pushDiagnostic(diagnostic); - } else if (extra.length !== 0) { - const diagnostic = CompilerDiagnostic.create({ - category: ErrorCategory.PreserveManualMemo, - reason: 'Found unnecessary memoization dependencies', - description: - 'Unnecessary dependencies can cause a value to update more often than necessary, ' + - 'which can cause effects to run more than expected. This memoization cannot be safely ' + - 'rewritten by the compiler', - }); - diagnostic.withDetails({ - kind: 'error', - message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`, - loc: value.loc, - }); - error.pushDiagnostic(diagnostic); } dependencies.clear();