Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export type CompilerDiagnosticDetail =
*/
{
kind: 'error';
loc: SourceLocation;
loc: SourceLocation | null;
message: string;
};

Expand Down Expand Up @@ -100,6 +100,12 @@ export class CompilerDiagnostic {
this.options = options;
}

static create(
options: Omit<CompilerDiagnosticOptions, 'details'>,
): CompilerDiagnostic {
return new CompilerDiagnostic({...options, details: []});
}

get category(): CompilerDiagnosticOptions['category'] {
return this.options.category;
}
Expand All @@ -113,6 +119,11 @@ export class CompilerDiagnostic {
return this.options.suggestions;
}

withDetail(detail: CompilerDiagnosticDetail): CompilerDiagnostic {
this.options.details.push(detail);
return this;
}

primaryLocation(): SourceLocation | null {
return this.options.details.filter(d => d.kind === 'error')[0]?.loc ?? null;
}
Expand All @@ -127,7 +138,7 @@ export class CompilerDiagnostic {
switch (detail.kind) {
case 'error': {
const loc = detail.loc;
if (typeof loc === 'symbol') {
if (loc == null || typeof loc === 'symbol') {
continue;
}
let codeFrame: string;
Expand Down
56 changes: 37 additions & 19 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {NodePath, Scope} from '@babel/traverse';
import * as t from '@babel/types';
import invariant from 'invariant';
import {
CompilerDiagnostic,
CompilerError,
CompilerSuggestionOperation,
ErrorSeverity,
Expand Down Expand Up @@ -104,12 +105,18 @@ export function lower(
if (param.isIdentifier()) {
const binding = builder.resolveIdentifier(param);
if (binding.kind !== 'Identifier') {
builder.errors.push({
reason: `(BuildHIR::lower) Could not find binding for param \`${param.node.name}\``,
severity: ErrorSeverity.Invariant,
loc: param.node.loc ?? null,
suggestions: null,
});
builder.errors.pushDiagnostic(
CompilerDiagnostic.create({
category: 'Could not find binding',
description: `[BuildHIR] Could not find binding for param \`${param.node.name}\``,
severity: ErrorSeverity.Invariant,
suggestions: null,
}).withDetail({
kind: 'error',
loc: param.node.loc ?? null,
message: 'Could not find binding',
}),
);
return;
}
const place: Place = {
Expand Down Expand Up @@ -163,12 +170,18 @@ export function lower(
'Assignment',
);
} else {
builder.errors.push({
reason: `(BuildHIR::lower) Handle ${param.node.type} params`,
severity: ErrorSeverity.Todo,
loc: param.node.loc ?? null,
suggestions: null,
});
builder.errors.pushDiagnostic(
CompilerDiagnostic.create({
category: `Handle ${param.node.type} parameters`,
description: `[BuildHIR] Add support for ${param.node.type} parameters`,
severity: ErrorSeverity.Todo,
suggestions: null,
}).withDetail({
kind: 'error',
loc: param.node.loc ?? null,
message: 'Unsupported parameter type',
}),
);
}
});

Expand All @@ -188,13 +201,18 @@ export function lower(
lowerStatement(builder, body);
directives = body.get('directives').map(d => d.node.value.value);
} else {
builder.errors.push({
severity: ErrorSeverity.InvalidJS,
reason: `Unexpected function body kind`,
description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
loc: body.node.loc ?? null,
suggestions: null,
});
builder.errors.pushDiagnostic(
CompilerDiagnostic.create({
severity: ErrorSeverity.InvalidJS,
category: `Unexpected function body kind`,
description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
suggestions: null,
}).withDetail({
kind: 'error',
loc: body.node.loc ?? null,
message: 'Expected a block statement or expression',
}),
);
}

if (builder.errors.hasErrors()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, Effect} from '..';
import {CompilerDiagnostic, CompilerError, Effect, ErrorSeverity} from '..';
import {HIRFunction, IdentifierId, Place} from '../HIR';
import {
eachInstructionLValue,
Expand All @@ -28,16 +28,19 @@ export function validateLocalsNotReassignedAfterRender(fn: HIRFunction): void {
false,
);
if (reassignment !== null) {
CompilerError.throwInvalidReact({
reason:
'Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead',
description:
reassignment.identifier.name !== null &&
reassignment.identifier.name.kind === 'named'
? `Variable \`${reassignment.identifier.name.value}\` cannot be reassigned after render`
: '',
loc: reassignment.loc,
});
const errors = new CompilerError();
errors.pushDiagnostic(
CompilerDiagnostic.create({
severity: ErrorSeverity.InvalidReact,
category: 'Cannot reassign a variable after render completes',
description: `Reassigning ${reassignment.identifier.name != null && reassignment.identifier.name.kind === 'named' ? `variable \`${reassignment.identifier.name.value}\`` : 'a variable'} after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead`,
}).withDetail({
kind: 'error',
loc: reassignment.loc,
message: 'Cannot reassign variable after render completes',
}),
);
throw errors;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, Effect, ErrorSeverity} from '..';
import {CompilerDiagnostic, CompilerError, Effect, ErrorSeverity} from '..';
import {
FunctionEffect,
HIRFunction,
Expand Down Expand Up @@ -57,16 +57,24 @@ export function validateNoFreezingKnownMutableFunctions(
if (operand.effect === Effect.Freeze) {
const effect = contextMutationEffects.get(operand.identifier.id);
if (effect != null) {
errors.push({
reason: `This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead`,
loc: operand.loc,
severity: ErrorSeverity.InvalidReact,
});
errors.push({
reason: `The function modifies a local variable here`,
loc: effect.loc,
severity: ErrorSeverity.InvalidReact,
});
errors.pushDiagnostic(
CompilerDiagnostic.create({
severity: ErrorSeverity.InvalidReact,
category: 'Cannot modify local variables after render completes',
description: `This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead`,
})
.withDetail({
kind: 'error',
loc: operand.loc,
message:
'This function may (indirectly) reassign or modify local variables after render',
})
.withDetail({
kind: 'error',
loc: effect.loc,
message: 'This modifies a local variable',
}),
);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, ErrorSeverity} from '..';
import {CompilerDiagnostic, CompilerError, ErrorSeverity} from '..';
import {HIRFunction} from '../HIR';
import {getFunctionCallSignature} from '../Inference/InferReferenceEffects';
import {Result} from '../Utils/Result';
Expand Down Expand Up @@ -34,17 +34,22 @@ export function validateNoImpureFunctionsInRender(
callee.identifier.type,
);
if (signature != null && signature.impure === true) {
errors.push({
reason:
'Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)',
description:
signature.canonicalName != null
? `\`${signature.canonicalName}\` is an impure function whose results may change on every call`
: null,
severity: ErrorSeverity.InvalidReact,
loc: callee.loc,
suggestions: null,
});
errors.pushDiagnostic(
CompilerDiagnostic.create({
category: 'Cannot call impure function during render',
description:
(signature.canonicalName != null
? `\`${signature.canonicalName}\` is an impure function. `
: '') +
'Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)',
severity: ErrorSeverity.InvalidReact,
suggestions: null,
}).withDetail({
kind: 'error',
loc: callee.loc,
message: 'Cannot call impure function',
}),
);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, ErrorSeverity} from '..';
import {CompilerDiagnostic, CompilerError, ErrorSeverity} from '..';
import {BlockId, HIRFunction} from '../HIR';
import {Result} from '../Utils/Result';
import {retainWhere} from '../Utils/utils';
Expand Down Expand Up @@ -34,11 +34,17 @@ export function validateNoJSXInTryStatement(
switch (value.kind) {
case 'JsxExpression':
case 'JsxFragment': {
errors.push({
severity: ErrorSeverity.InvalidReact,
reason: `Unexpected JSX element within a try statement. To catch errors in rendering a given component, wrap that component in an error boundary. (https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary)`,
loc: value.loc,
});
errors.pushDiagnostic(
CompilerDiagnostic.create({
severity: ErrorSeverity.InvalidReact,
category: 'Avoid constructing JSX within try/catch',
description: `React does not immediately render components when JSX is rendered, so any errors from this component will not be caught by the try/catch. To catch errors in rendering a given component, wrap that component in an error boundary. (https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary)`,
}).withDetail({
kind: 'error',
loc: value.loc,
message: 'Avoid constructing JSX within try/catch',
}),
);
break;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, ErrorSeverity} from '../CompilerError';
import {
CompilerDiagnostic,
CompilerError,
ErrorSeverity,
} from '../CompilerError';
import {
HIRFunction,
IdentifierId,
Expand Down Expand Up @@ -90,14 +94,21 @@ export function validateNoSetStateInEffects(
if (arg !== undefined && arg.kind === 'Identifier') {
const setState = setStateFunctions.get(arg.identifier.id);
if (setState !== undefined) {
errors.push({
reason:
'Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)',
description: null,
severity: ErrorSeverity.InvalidReact,
loc: setState.loc,
suggestions: null,
});
errors.pushDiagnostic(
CompilerDiagnostic.create({
category:
'Calling setState within an effect can trigger cascading renders',
description:
'Calling setState directly within a useEffect causes cascading renders that can hurt performance, and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)',
severity: ErrorSeverity.InvalidReact,
suggestions: null,
}).withDetail({
kind: 'error',
loc: setState.loc,
message:
'Avoid calling setState() in the top-level of an effect',
}),
);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, ErrorSeverity} from '../CompilerError';
import {
CompilerDiagnostic,
CompilerError,
ErrorSeverity,
} from '../CompilerError';
import {HIRFunction, IdentifierId, isSetStateType} from '../HIR';
import {computeUnconditionalBlocks} from '../HIR/ComputeUnconditionalBlocks';
import {eachInstructionValueOperand} from '../HIR/visitors';
Expand Down Expand Up @@ -122,23 +126,35 @@ function validateNoSetStateInRenderImpl(
unconditionalSetStateFunctions.has(callee.identifier.id)
) {
if (activeManualMemoId !== null) {
errors.push({
reason:
'Calling setState from useMemo may trigger an infinite loop. (https://react.dev/reference/react/useState)',
description: null,
severity: ErrorSeverity.InvalidReact,
loc: callee.loc,
suggestions: null,
});
errors.pushDiagnostic(
CompilerDiagnostic.create({
category:
'Calling setState from useMemo may trigger an infinite loop',
description:
'Each time the memo callback is evaluated it will change state. This can cause a memoization dependency to change, running the memo function again and causing an infinite loop. Instead of setting state in useMemo(), prefer deriving the value during render. (https://react.dev/reference/react/useState)',
severity: ErrorSeverity.InvalidReact,
suggestions: null,
}).withDetail({
kind: 'error',
loc: callee.loc,
message: 'Found setState() within useMemo()',
}),
);
} else if (unconditionalBlocks.has(block.id)) {
errors.push({
reason:
'This is an unconditional set state during render, which will trigger an infinite loop. (https://react.dev/reference/react/useState)',
description: null,
severity: ErrorSeverity.InvalidReact,
loc: callee.loc,
suggestions: null,
});
errors.pushDiagnostic(
CompilerDiagnostic.create({
category:
'Calling setState during render may trigger an infinite loop',
description:
'Calling setState during render will trigger another render, and can lead to infinite loops. (https://react.dev/reference/react/useState)',
severity: ErrorSeverity.InvalidReact,
suggestions: null,
}).withDetail({
kind: 'error',
loc: callee.loc,
message: 'Found setState() within useMemo()',
}),
);
}
}
break;
Expand Down
Loading
Loading