Skip to content

Commit 8039f1b

Browse files
authored
[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.
1 parent 4280563 commit 8039f1b

File tree

9 files changed

+78
-9
lines changed

9 files changed

+78
-9
lines changed

compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ export default function BabelPluginReactCompiler(
7373
pass.filename ?? null,
7474
opts.logger,
7575
opts.environment,
76-
result?.retryErrors ?? [],
76+
result,
7777
);
7878
if (ENABLE_REACT_COMPILER_TIMINGS === true) {
7979
performance.mark(`${filename}:end`, {

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

+13-2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
findProgramSuppressions,
3131
suppressionsToCompilerError,
3232
} from './Suppression';
33+
import {GeneratedSource} from '../HIR';
3334

3435
export type CompilerPass = {
3536
opts: PluginOptions;
@@ -267,8 +268,9 @@ function isFilePartOfSources(
267268
return false;
268269
}
269270

270-
type CompileProgramResult = {
271+
export type CompileProgramResult = {
271272
retryErrors: Array<{fn: BabelFn; error: CompilerError}>;
273+
inferredEffectLocations: Set<t.SourceLocation>;
272274
};
273275
/**
274276
* `compileProgram` is directly invoked by the react-compiler babel plugin, so
@@ -369,6 +371,7 @@ export function compileProgram(
369371
},
370372
);
371373
const retryErrors: Array<{fn: BabelFn; error: CompilerError}> = [];
374+
const inferredEffectLocations = new Set<t.SourceLocation>();
372375
const processFn = (
373376
fn: BabelFn,
374377
fnType: ReactFunctionType,
@@ -509,6 +512,14 @@ export function compileProgram(
509512
if (!pass.opts.noEmit) {
510513
return compileResult.compiledFn;
511514
}
515+
/**
516+
* inferEffectDependencies + noEmit is currently only used for linting. In
517+
* this mode, add source locations for where the compiler *can* infer effect
518+
* dependencies.
519+
*/
520+
for (const loc of compileResult.compiledFn.inferredEffectLocations) {
521+
if (loc !== GeneratedSource) inferredEffectLocations.add(loc);
522+
}
512523
return null;
513524
};
514525

@@ -587,7 +598,7 @@ export function compileProgram(
587598
if (compiledFns.length > 0) {
588599
addImportsToProgram(program, programContext);
589600
}
590-
return {retryErrors};
601+
return {retryErrors, inferredEffectLocations};
591602
}
592603

593604
function shouldSkipCompilation(

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts

+13-5
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
import {getOrInsertWith} from '../Utils/utils';
1212
import {Environment} from '../HIR';
1313
import {DEFAULT_EXPORT} from '../HIR/Environment';
14+
import {CompileProgramResult} from './Program';
1415

1516
function throwInvalidReact(
1617
options: Omit<CompilerErrorDetailOptions, 'severity'>,
@@ -36,12 +37,16 @@ function assertValidEffectImportReference(
3637
const parent = path.parentPath;
3738
if (parent != null && parent.isCallExpression()) {
3839
const args = parent.get('arguments');
40+
const maybeCalleeLoc = path.node.loc;
41+
const hasInferredEffect =
42+
maybeCalleeLoc != null &&
43+
context.inferredEffectLocations.has(maybeCalleeLoc);
3944
/**
4045
* Only error on untransformed references of the form `useMyEffect(...)`
4146
* or `moduleNamespace.useMyEffect(...)`, with matching argument counts.
4247
* TODO: do we also want a mode to also hard error on non-call references?
4348
*/
44-
if (args.length === numArgs) {
49+
if (args.length === numArgs && !hasInferredEffect) {
4550
const maybeErrorDiagnostic = matchCompilerDiagnostic(
4651
path,
4752
context.transformErrors,
@@ -97,7 +102,7 @@ export default function validateNoUntransformedReferences(
97102
filename: string | null,
98103
logger: Logger | null,
99104
env: EnvironmentConfig,
100-
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>,
105+
compileResult: CompileProgramResult | null,
101106
): void {
102107
const moduleLoadChecks = new Map<
103108
string,
@@ -126,7 +131,7 @@ export default function validateNoUntransformedReferences(
126131
}
127132
}
128133
if (moduleLoadChecks.size > 0) {
129-
transformProgram(path, moduleLoadChecks, filename, logger, transformErrors);
134+
transformProgram(path, moduleLoadChecks, filename, logger, compileResult);
130135
}
131136
}
132137

@@ -136,6 +141,7 @@ type TraversalState = {
136141
logger: Logger | null;
137142
filename: string | null;
138143
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>;
144+
inferredEffectLocations: Set<t.SourceLocation>;
139145
};
140146
type CheckInvalidReferenceFn = (
141147
paths: Array<NodePath<t.Node>>,
@@ -223,14 +229,16 @@ function transformProgram(
223229
moduleLoadChecks: Map<string, Map<string, CheckInvalidReferenceFn>>,
224230
filename: string | null,
225231
logger: Logger | null,
226-
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>,
232+
compileResult: CompileProgramResult | null,
227233
): void {
228234
const traversalState: TraversalState = {
229235
shouldInvalidateScopes: true,
230236
program: path,
231237
filename,
232238
logger,
233-
transformErrors,
239+
transformErrors: compileResult?.retryErrors ?? [],
240+
inferredEffectLocations:
241+
compileResult?.inferredEffectLocations ?? new Set(),
234242
};
235243
path.traverse({
236244
ImportDeclaration(path: NodePath<t.ImportDeclaration>) {

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

+8
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {fromZodError} from 'zod-validation-error';
1111
import {CompilerError} from '../CompilerError';
1212
import {
1313
CompilationMode,
14+
defaultOptions,
1415
Logger,
1516
PanicThresholdOptions,
1617
parsePluginOptions,
@@ -779,6 +780,7 @@ export function parseConfigPragmaForTests(
779780
const environment = parseConfigPragmaEnvironmentForTest(pragma);
780781
let compilationMode: CompilationMode = defaults.compilationMode;
781782
let panicThreshold: PanicThresholdOptions = 'all_errors';
783+
let noEmit: boolean = defaultOptions.noEmit;
782784
for (const token of pragma.split(' ')) {
783785
if (!token.startsWith('@')) {
784786
continue;
@@ -804,12 +806,17 @@ export function parseConfigPragmaForTests(
804806
panicThreshold = 'none';
805807
break;
806808
}
809+
case '@noEmit': {
810+
noEmit = true;
811+
break;
812+
}
807813
}
808814
}
809815
return parsePluginOptions({
810816
environment,
811817
compilationMode,
812818
panicThreshold,
819+
noEmit,
813820
});
814821
}
815822

@@ -852,6 +859,7 @@ export class Environment {
852859
programContext: ProgramContext;
853860
hasFireRewrite: boolean;
854861
hasInferredEffect: boolean;
862+
inferredEffectLocations: Set<SourceLocation> = new Set();
855863

856864
#contextIdentifiers: Set<t.Identifier>;
857865
#hoistedIdentifiers: Set<t.Identifier>;

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

+2
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ export function inferEffectDependencies(fn: HIRFunction): void {
217217
// Step 2: push the inferred deps array as an argument of the useEffect
218218
value.args.push({...depsPlace, effect: Effect.Freeze});
219219
rewriteInstrs.set(instr.id, newInstructions);
220+
fn.env.inferredEffectLocations.add(callee.loc);
220221
} else if (loadGlobals.has(value.args[0].identifier.id)) {
221222
// Global functions have no reactive dependencies, so we can insert an empty array
222223
newInstructions.push({
@@ -227,6 +228,7 @@ export function inferEffectDependencies(fn: HIRFunction): void {
227228
});
228229
value.args.push({...depsPlace, effect: Effect.Freeze});
229230
rewriteInstrs.set(instr.id, newInstructions);
231+
fn.env.inferredEffectLocations.add(callee.loc);
230232
}
231233
}
232234
}

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts

+2
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ export type CodegenFunction = {
104104
* This is true if the compiler has compiled inferred effect dependencies
105105
*/
106106
hasInferredEffect: boolean;
107+
inferredEffectLocations: Set<SourceLocation>;
107108

108109
/**
109110
* This is true if the compiler has compiled a fire to a useFire call
@@ -389,6 +390,7 @@ function codegenReactiveFunction(
389390
outlined: [],
390391
hasFireRewrite: fn.env.hasFireRewrite,
391392
hasInferredEffect: fn.env.hasInferredEffect,
393+
inferredEffectLocations: fn.env.inferredEffectLocations,
392394
});
393395
}
394396

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies @noEmit
6+
import {print} from 'shared-runtime';
7+
import useEffectWrapper from 'useEffectWrapper';
8+
9+
function ReactiveVariable({propVal}) {
10+
const arr = [propVal];
11+
useEffectWrapper(() => print(arr));
12+
}
13+
14+
```
15+
16+
## Code
17+
18+
```javascript
19+
// @inferEffectDependencies @noEmit
20+
import { print } from "shared-runtime";
21+
import useEffectWrapper from "useEffectWrapper";
22+
23+
function ReactiveVariable({ propVal }) {
24+
const arr = [propVal];
25+
useEffectWrapper(() => print(arr));
26+
}
27+
28+
```
29+
30+
### Eval output
31+
(kind: exception) Fixture not implemented
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
// @inferEffectDependencies @noEmit
2+
import {print} from 'shared-runtime';
3+
import useEffectWrapper from 'useEffectWrapper';
4+
5+
function ReactiveVariable({propVal}) {
6+
const arr = [propVal];
7+
useEffectWrapper(() => print(arr));
8+
}

compiler/packages/snap/src/compiler.ts

-1
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,6 @@ function makePluginOptions(
187187
},
188188
logger,
189189
gating,
190-
noEmit: false,
191190
eslintSuppressionRules,
192191
flowSuppressions,
193192
ignoreUseNoForget,

0 commit comments

Comments
 (0)