mirror of
https://github.com/zebrajr/react.git
synced 2025-12-06 00:20:04 +01:00
NOTE: this is a merged version of @mofeiZ's original PR along with my edits per offline discussion. The description is updated to reflect the latest approach. The key problem we're trying to solve with this PR is to allow developers more control over the compiler's various validations. The idea is to have a number of rules targeting a specific category of issues, such as enforcing immutability of props/state/etc or disallowing access to refs during render. We don't want to have to run the compiler again for every single rule, though, so @mofeiZ added an LRU cache that caches the full compilation output of N most recent files. The first rule to run on a given file will cause it to get cached, and then subsequent rules can pull from the cache, with each rule filtering down to its specific category of errors. For the categories, I went through and assigned a category roughly 1:1 to existing validations, and then used my judgement on some places that felt distinct enough to warrant a separate error. Every error in the compiler now has to supply both a severity (for legacy reasons) and a category (for ESLint). Each category corresponds 1:1 to a ESLint rule definition, so that the set of rules is automatically populated based on the defined categories. Categories include a flag for whether they should be in the recommended set or not. Note that as with the original version of this PR, only eslint-plugin-react-compiler is changed. We still have to update the main lint rule. ## Test Plan * Created a sample project using ESLint v9 and verified that the plugin can be configured correctly and detects errors * Edited `fixtures/eslint-v9` and introduced errors, verified that the w latest config changes in that fixture it correctly detects the errors * In the sample project, confirmed that the LRU caching is correctly caching compiler output, ie compiling files just once. Co-authored-by: Mofei Zhang <feifei0@meta.com>
178 lines
6.1 KiB
TypeScript
178 lines
6.1 KiB
TypeScript
/**
|
|
* Copyright (c) Meta Platforms, Inc. and affiliates.
|
|
*
|
|
* This source code is licensed under the MIT license found in the
|
|
* LICENSE file in the root directory of this source tree.
|
|
*/
|
|
|
|
import {
|
|
CompilerDiagnostic,
|
|
CompilerError,
|
|
ErrorCategory,
|
|
ErrorSeverity,
|
|
} from '../CompilerError';
|
|
import {
|
|
HIRFunction,
|
|
IdentifierId,
|
|
isSetStateType,
|
|
isUseEffectHookType,
|
|
isUseInsertionEffectHookType,
|
|
isUseLayoutEffectHookType,
|
|
Place,
|
|
} from '../HIR';
|
|
import {eachInstructionValueOperand} from '../HIR/visitors';
|
|
import {Result} from '../Utils/Result';
|
|
|
|
/**
|
|
* Validates against calling setState in the body of an effect (useEffect and friends),
|
|
* while allowing calling setState in callbacks scheduled by the effect.
|
|
*
|
|
* Calling setState during execution of a useEffect triggers a re-render, which is
|
|
* often bad for performance and frequently has more efficient and straightforward
|
|
* alternatives. See https://react.dev/learn/you-might-not-need-an-effect for examples.
|
|
*/
|
|
export function validateNoSetStateInEffects(
|
|
fn: HIRFunction,
|
|
): Result<void, CompilerError> {
|
|
const setStateFunctions: Map<IdentifierId, Place> = new Map();
|
|
const errors = new CompilerError();
|
|
for (const [, block] of fn.body.blocks) {
|
|
for (const instr of block.instructions) {
|
|
switch (instr.value.kind) {
|
|
case 'LoadLocal': {
|
|
if (setStateFunctions.has(instr.value.place.identifier.id)) {
|
|
setStateFunctions.set(
|
|
instr.lvalue.identifier.id,
|
|
instr.value.place,
|
|
);
|
|
}
|
|
break;
|
|
}
|
|
case 'StoreLocal': {
|
|
if (setStateFunctions.has(instr.value.value.identifier.id)) {
|
|
setStateFunctions.set(
|
|
instr.value.lvalue.place.identifier.id,
|
|
instr.value.value,
|
|
);
|
|
setStateFunctions.set(
|
|
instr.lvalue.identifier.id,
|
|
instr.value.value,
|
|
);
|
|
}
|
|
break;
|
|
}
|
|
case 'FunctionExpression': {
|
|
if (
|
|
// faster-path to check if the function expression references a setState
|
|
[...eachInstructionValueOperand(instr.value)].some(
|
|
operand =>
|
|
isSetStateType(operand.identifier) ||
|
|
setStateFunctions.has(operand.identifier.id),
|
|
)
|
|
) {
|
|
const callee = getSetStateCall(
|
|
instr.value.loweredFunc.func,
|
|
setStateFunctions,
|
|
);
|
|
if (callee !== null) {
|
|
setStateFunctions.set(instr.lvalue.identifier.id, callee);
|
|
}
|
|
}
|
|
break;
|
|
}
|
|
case 'MethodCall':
|
|
case 'CallExpression': {
|
|
const callee =
|
|
instr.value.kind === 'MethodCall'
|
|
? instr.value.receiver
|
|
: instr.value.callee;
|
|
if (
|
|
isUseEffectHookType(callee.identifier) ||
|
|
isUseLayoutEffectHookType(callee.identifier) ||
|
|
isUseInsertionEffectHookType(callee.identifier)
|
|
) {
|
|
const arg = instr.value.args[0];
|
|
if (arg !== undefined && arg.kind === 'Identifier') {
|
|
const setState = setStateFunctions.get(arg.identifier.id);
|
|
if (setState !== undefined) {
|
|
errors.pushDiagnostic(
|
|
CompilerDiagnostic.create({
|
|
category: ErrorCategory.EffectSetState,
|
|
reason:
|
|
'Calling setState synchronously within an effect can trigger cascading renders',
|
|
description:
|
|
'Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. ' +
|
|
'In general, the body of an effect should do one or both of the following:\n' +
|
|
'* Update external systems with the latest state from React.\n' +
|
|
'* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\n' +
|
|
'Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. ' +
|
|
'(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() directly within an effect',
|
|
}),
|
|
);
|
|
}
|
|
}
|
|
}
|
|
break;
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
return errors.asResult();
|
|
}
|
|
|
|
function getSetStateCall(
|
|
fn: HIRFunction,
|
|
setStateFunctions: Map<IdentifierId, Place>,
|
|
): Place | null {
|
|
for (const [, block] of fn.body.blocks) {
|
|
for (const instr of block.instructions) {
|
|
switch (instr.value.kind) {
|
|
case 'LoadLocal': {
|
|
if (setStateFunctions.has(instr.value.place.identifier.id)) {
|
|
setStateFunctions.set(
|
|
instr.lvalue.identifier.id,
|
|
instr.value.place,
|
|
);
|
|
}
|
|
break;
|
|
}
|
|
case 'StoreLocal': {
|
|
if (setStateFunctions.has(instr.value.value.identifier.id)) {
|
|
setStateFunctions.set(
|
|
instr.value.lvalue.place.identifier.id,
|
|
instr.value.value,
|
|
);
|
|
setStateFunctions.set(
|
|
instr.lvalue.identifier.id,
|
|
instr.value.value,
|
|
);
|
|
}
|
|
break;
|
|
}
|
|
case 'CallExpression': {
|
|
const callee = instr.value.callee;
|
|
if (
|
|
isSetStateType(callee.identifier) ||
|
|
setStateFunctions.has(callee.identifier.id)
|
|
) {
|
|
/*
|
|
* TODO: once we support multiple locations per error, we should link to the
|
|
* original Place in the case that setStateFunction.has(callee)
|
|
*/
|
|
return callee;
|
|
}
|
|
}
|
|
}
|
|
}
|
|
}
|
|
return null;
|
|
}
|