Compare commits

...

1 Commits

Author SHA1 Message Date
Mofei Zhang
caf09d1e60 [compiler] Fix inferEffectDependencies lint false positives
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.
2025-03-27 10:56:35 -04:00
9 changed files with 78 additions and 9 deletions

View File

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

View File

@@ -30,6 +30,7 @@ import {
findProgramSuppressions, findProgramSuppressions,
suppressionsToCompilerError, suppressionsToCompilerError,
} from './Suppression'; } from './Suppression';
import {GeneratedSource} from '../HIR';
export type CompilerPass = { export type CompilerPass = {
opts: PluginOptions; opts: PluginOptions;
@@ -267,8 +268,9 @@ function isFilePartOfSources(
return false; return false;
} }
type CompileProgramResult = { export type CompileProgramResult = {
retryErrors: Array<{fn: BabelFn; error: CompilerError}>; retryErrors: Array<{fn: BabelFn; error: CompilerError}>;
inferredEffectLocations: Set<t.SourceLocation>;
}; };
/** /**
* `compileProgram` is directly invoked by the react-compiler babel plugin, so * `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 retryErrors: Array<{fn: BabelFn; error: CompilerError}> = [];
const inferredEffectLocations = new Set<t.SourceLocation>();
const processFn = ( const processFn = (
fn: BabelFn, fn: BabelFn,
fnType: ReactFunctionType, fnType: ReactFunctionType,
@@ -509,6 +512,14 @@ export function compileProgram(
if (!pass.opts.noEmit) { if (!pass.opts.noEmit) {
return compileResult.compiledFn; 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; return null;
}; };
@@ -587,7 +598,7 @@ export function compileProgram(
if (compiledFns.length > 0) { if (compiledFns.length > 0) {
addImportsToProgram(program, programContext); addImportsToProgram(program, programContext);
} }
return {retryErrors}; return {retryErrors, inferredEffectLocations};
} }
function shouldSkipCompilation( function shouldSkipCompilation(

View File

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

View File

@@ -11,6 +11,7 @@ import {fromZodError} from 'zod-validation-error';
import {CompilerError} from '../CompilerError'; import {CompilerError} from '../CompilerError';
import { import {
CompilationMode, CompilationMode,
defaultOptions,
Logger, Logger,
PanicThresholdOptions, PanicThresholdOptions,
parsePluginOptions, parsePluginOptions,
@@ -779,6 +780,7 @@ export function parseConfigPragmaForTests(
const environment = parseConfigPragmaEnvironmentForTest(pragma); const environment = parseConfigPragmaEnvironmentForTest(pragma);
let compilationMode: CompilationMode = defaults.compilationMode; let compilationMode: CompilationMode = defaults.compilationMode;
let panicThreshold: PanicThresholdOptions = 'all_errors'; let panicThreshold: PanicThresholdOptions = 'all_errors';
let noEmit: boolean = defaultOptions.noEmit;
for (const token of pragma.split(' ')) { for (const token of pragma.split(' ')) {
if (!token.startsWith('@')) { if (!token.startsWith('@')) {
continue; continue;
@@ -804,12 +806,17 @@ export function parseConfigPragmaForTests(
panicThreshold = 'none'; panicThreshold = 'none';
break; break;
} }
case '@noEmit': {
noEmit = true;
break;
}
} }
} }
return parsePluginOptions({ return parsePluginOptions({
environment, environment,
compilationMode, compilationMode,
panicThreshold, panicThreshold,
noEmit,
}); });
} }
@@ -852,6 +859,7 @@ export class Environment {
programContext: ProgramContext; programContext: ProgramContext;
hasFireRewrite: boolean; hasFireRewrite: boolean;
hasInferredEffect: boolean; hasInferredEffect: boolean;
inferredEffectLocations: Set<SourceLocation> = new Set();
#contextIdentifiers: Set<t.Identifier>; #contextIdentifiers: Set<t.Identifier>;
#hoistedIdentifiers: 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 // Step 2: push the inferred deps array as an argument of the useEffect
value.args.push({...depsPlace, effect: Effect.Freeze}); value.args.push({...depsPlace, effect: Effect.Freeze});
rewriteInstrs.set(instr.id, newInstructions); rewriteInstrs.set(instr.id, newInstructions);
fn.env.inferredEffectLocations.add(callee.loc);
} else if (loadGlobals.has(value.args[0].identifier.id)) { } else if (loadGlobals.has(value.args[0].identifier.id)) {
// Global functions have no reactive dependencies, so we can insert an empty array // Global functions have no reactive dependencies, so we can insert an empty array
newInstructions.push({ newInstructions.push({
@@ -227,6 +228,7 @@ export function inferEffectDependencies(fn: HIRFunction): void {
}); });
value.args.push({...depsPlace, effect: Effect.Freeze}); value.args.push({...depsPlace, effect: Effect.Freeze});
rewriteInstrs.set(instr.id, newInstructions); 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 * This is true if the compiler has compiled inferred effect dependencies
*/ */
hasInferredEffect: boolean; hasInferredEffect: boolean;
inferredEffectLocations: Set<SourceLocation>;
/** /**
* This is true if the compiler has compiled a fire to a useFire call * This is true if the compiler has compiled a fire to a useFire call
@@ -389,6 +390,7 @@ function codegenReactiveFunction(
outlined: [], outlined: [],
hasFireRewrite: fn.env.hasFireRewrite, hasFireRewrite: fn.env.hasFireRewrite,
hasInferredEffect: fn.env.hasInferredEffect, 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, logger,
gating, gating,
noEmit: false,
eslintSuppressionRules, eslintSuppressionRules,
flowSuppressions, flowSuppressions,
ignoreUseNoForget, ignoreUseNoForget,