[compiler] Fix inferEffectDependencies lint false positives (#32769)

Currently, inferred effect dependencies are considered a
"compiler-required" feature. This means that untransformed callsites
should escalate to a build error.

`ValidateNoUntransformedReferences` iterates 'special effect' callsites
and checks that the compiler was able to successfully transform them.
Prior to this PR, this relied on checking the number of arguments passed
to this special effect.

This obviously doesn't work with `noEmit: true`, which is used for our
eslint plugin (this avoids mutating the babel program as other linters
run with the same ast). This PR adds a set of `babel.SourceLocation`s to
do best effort matching in this mode.
This commit is contained in:
mofeiZ 2025-03-27 12:18:50 -04:00 committed by GitHub
parent 4280563b04
commit 8039f1b2a0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 78 additions and 9 deletions

View File

@ -73,7 +73,7 @@ export default function BabelPluginReactCompiler(
pass.filename ?? null,
opts.logger,
opts.environment,
result?.retryErrors ?? [],
result,
);
if (ENABLE_REACT_COMPILER_TIMINGS === true) {
performance.mark(`${filename}:end`, {

View File

@ -30,6 +30,7 @@ import {
findProgramSuppressions,
suppressionsToCompilerError,
} from './Suppression';
import {GeneratedSource} from '../HIR';
export type CompilerPass = {
opts: PluginOptions;
@ -267,8 +268,9 @@ function isFilePartOfSources(
return false;
}
type CompileProgramResult = {
export type CompileProgramResult = {
retryErrors: Array<{fn: BabelFn; error: CompilerError}>;
inferredEffectLocations: Set<t.SourceLocation>;
};
/**
* `compileProgram` is directly invoked by the react-compiler babel plugin, so
@ -369,6 +371,7 @@ export function compileProgram(
},
);
const retryErrors: Array<{fn: BabelFn; error: CompilerError}> = [];
const inferredEffectLocations = new Set<t.SourceLocation>();
const processFn = (
fn: BabelFn,
fnType: ReactFunctionType,
@ -509,6 +512,14 @@ export function compileProgram(
if (!pass.opts.noEmit) {
return compileResult.compiledFn;
}
/**
* inferEffectDependencies + noEmit is currently only used for linting. In
* this mode, add source locations for where the compiler *can* infer effect
* dependencies.
*/
for (const loc of compileResult.compiledFn.inferredEffectLocations) {
if (loc !== GeneratedSource) inferredEffectLocations.add(loc);
}
return null;
};
@ -587,7 +598,7 @@ export function compileProgram(
if (compiledFns.length > 0) {
addImportsToProgram(program, programContext);
}
return {retryErrors};
return {retryErrors, inferredEffectLocations};
}
function shouldSkipCompilation(

View File

@ -11,6 +11,7 @@ import {
import {getOrInsertWith} from '../Utils/utils';
import {Environment} from '../HIR';
import {DEFAULT_EXPORT} from '../HIR/Environment';
import {CompileProgramResult} from './Program';
function throwInvalidReact(
options: Omit<CompilerErrorDetailOptions, 'severity'>,
@ -36,12 +37,16 @@ function assertValidEffectImportReference(
const parent = path.parentPath;
if (parent != null && parent.isCallExpression()) {
const args = parent.get('arguments');
const maybeCalleeLoc = path.node.loc;
const hasInferredEffect =
maybeCalleeLoc != null &&
context.inferredEffectLocations.has(maybeCalleeLoc);
/**
* Only error on untransformed references of the form `useMyEffect(...)`
* or `moduleNamespace.useMyEffect(...)`, with matching argument counts.
* TODO: do we also want a mode to also hard error on non-call references?
*/
if (args.length === numArgs) {
if (args.length === numArgs && !hasInferredEffect) {
const maybeErrorDiagnostic = matchCompilerDiagnostic(
path,
context.transformErrors,
@ -97,7 +102,7 @@ export default function validateNoUntransformedReferences(
filename: string | null,
logger: Logger | null,
env: EnvironmentConfig,
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>,
compileResult: CompileProgramResult | null,
): void {
const moduleLoadChecks = new Map<
string,
@ -126,7 +131,7 @@ export default function validateNoUntransformedReferences(
}
}
if (moduleLoadChecks.size > 0) {
transformProgram(path, moduleLoadChecks, filename, logger, transformErrors);
transformProgram(path, moduleLoadChecks, filename, logger, compileResult);
}
}
@ -136,6 +141,7 @@ type TraversalState = {
logger: Logger | null;
filename: string | null;
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>;
inferredEffectLocations: Set<t.SourceLocation>;
};
type CheckInvalidReferenceFn = (
paths: Array<NodePath<t.Node>>,
@ -223,14 +229,16 @@ function transformProgram(
moduleLoadChecks: Map<string, Map<string, CheckInvalidReferenceFn>>,
filename: string | null,
logger: Logger | null,
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>,
compileResult: CompileProgramResult | null,
): void {
const traversalState: TraversalState = {
shouldInvalidateScopes: true,
program: path,
filename,
logger,
transformErrors,
transformErrors: compileResult?.retryErrors ?? [],
inferredEffectLocations:
compileResult?.inferredEffectLocations ?? new Set(),
};
path.traverse({
ImportDeclaration(path: NodePath<t.ImportDeclaration>) {

View File

@ -11,6 +11,7 @@ import {fromZodError} from 'zod-validation-error';
import {CompilerError} from '../CompilerError';
import {
CompilationMode,
defaultOptions,
Logger,
PanicThresholdOptions,
parsePluginOptions,
@ -779,6 +780,7 @@ export function parseConfigPragmaForTests(
const environment = parseConfigPragmaEnvironmentForTest(pragma);
let compilationMode: CompilationMode = defaults.compilationMode;
let panicThreshold: PanicThresholdOptions = 'all_errors';
let noEmit: boolean = defaultOptions.noEmit;
for (const token of pragma.split(' ')) {
if (!token.startsWith('@')) {
continue;
@ -804,12 +806,17 @@ export function parseConfigPragmaForTests(
panicThreshold = 'none';
break;
}
case '@noEmit': {
noEmit = true;
break;
}
}
}
return parsePluginOptions({
environment,
compilationMode,
panicThreshold,
noEmit,
});
}
@ -852,6 +859,7 @@ export class Environment {
programContext: ProgramContext;
hasFireRewrite: boolean;
hasInferredEffect: boolean;
inferredEffectLocations: Set<SourceLocation> = new Set();
#contextIdentifiers: Set<t.Identifier>;
#hoistedIdentifiers: Set<t.Identifier>;

View File

@ -217,6 +217,7 @@ export function inferEffectDependencies(fn: HIRFunction): void {
// Step 2: push the inferred deps array as an argument of the useEffect
value.args.push({...depsPlace, effect: Effect.Freeze});
rewriteInstrs.set(instr.id, newInstructions);
fn.env.inferredEffectLocations.add(callee.loc);
} else if (loadGlobals.has(value.args[0].identifier.id)) {
// Global functions have no reactive dependencies, so we can insert an empty array
newInstructions.push({
@ -227,6 +228,7 @@ export function inferEffectDependencies(fn: HIRFunction): void {
});
value.args.push({...depsPlace, effect: Effect.Freeze});
rewriteInstrs.set(instr.id, newInstructions);
fn.env.inferredEffectLocations.add(callee.loc);
}
}
}

View File

@ -104,6 +104,7 @@ export type CodegenFunction = {
* This is true if the compiler has compiled inferred effect dependencies
*/
hasInferredEffect: boolean;
inferredEffectLocations: Set<SourceLocation>;
/**
* This is true if the compiler has compiled a fire to a useFire call
@ -389,6 +390,7 @@ function codegenReactiveFunction(
outlined: [],
hasFireRewrite: fn.env.hasFireRewrite,
hasInferredEffect: fn.env.hasInferredEffect,
inferredEffectLocations: fn.env.inferredEffectLocations,
});
}

View File

@ -0,0 +1,31 @@
## Input
```javascript
// @inferEffectDependencies @noEmit
import {print} from 'shared-runtime';
import useEffectWrapper from 'useEffectWrapper';
function ReactiveVariable({propVal}) {
const arr = [propVal];
useEffectWrapper(() => print(arr));
}
```
## Code
```javascript
// @inferEffectDependencies @noEmit
import { print } from "shared-runtime";
import useEffectWrapper from "useEffectWrapper";
function ReactiveVariable({ propVal }) {
const arr = [propVal];
useEffectWrapper(() => print(arr));
}
```
### Eval output
(kind: exception) Fixture not implemented

View File

@ -0,0 +1,8 @@
// @inferEffectDependencies @noEmit
import {print} from 'shared-runtime';
import useEffectWrapper from 'useEffectWrapper';
function ReactiveVariable({propVal}) {
const arr = [propVal];
useEffectWrapper(() => print(arr));
}

View File

@ -187,7 +187,6 @@ function makePluginOptions(
},
logger,
gating,
noEmit: false,
eslintSuppressionRules,
flowSuppressions,
ignoreUseNoForget,