From caf09d1e60eecd6318102fb54d09165982cdf403 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Thu, 27 Mar 2025 10:52:16 -0400 Subject: [PATCH] [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. --- .../src/Babel/BabelPlugin.ts | 2 +- .../src/Entrypoint/Program.ts | 15 +++++++-- .../ValidateNoUntransformedReferences.ts | 18 ++++++++--- .../src/HIR/Environment.ts | 8 +++++ .../src/Inference/InferEffectDependencies.ts | 2 ++ .../ReactiveScopes/CodegenReactiveFunction.ts | 2 ++ .../no-emit-lint-repro.expect.md | 31 +++++++++++++++++++ .../no-emit-lint-repro.js | 8 +++++ compiler/packages/snap/src/compiler.ts | 1 - 9 files changed, 78 insertions(+), 9 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts b/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts index ff9817380f..5816719424 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts @@ -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`, { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index c00c672b2c..622b7f72da 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -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; }; /** * `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(); 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( diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts index 07ab3b2b6a..a221b0485c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts @@ -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, @@ -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; 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; error: CompilerError}>; + inferredEffectLocations: Set; }; type CheckInvalidReferenceFn = ( paths: Array>, @@ -223,14 +229,16 @@ function transformProgram( moduleLoadChecks: Map>, filename: string | null, logger: Logger | null, - transformErrors: Array<{fn: NodePath; 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) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 2594ac31c6..276e4f7b40 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -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 = new Set(); #contextIdentifiers: Set; #hoistedIdentifiers: Set; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts index 85cb023665..03bd9fd382 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts @@ -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); } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index b90e4e417c..9d41663d56 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -104,6 +104,7 @@ export type CodegenFunction = { * This is true if the compiler has compiled inferred effect dependencies */ hasInferredEffect: boolean; + inferredEffectLocations: Set; /** * 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, }); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.expect.md new file mode 100644 index 0000000000..b5bbd032ba --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.expect.md @@ -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 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.js new file mode 100644 index 0000000000..939b604530 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/no-emit-lint-repro.js @@ -0,0 +1,8 @@ +// @inferEffectDependencies @noEmit +import {print} from 'shared-runtime'; +import useEffectWrapper from 'useEffectWrapper'; + +function ReactiveVariable({propVal}) { + const arr = [propVal]; + useEffectWrapper(() => print(arr)); +} diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index 6e59276c1c..6fce644542 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -187,7 +187,6 @@ function makePluginOptions( }, logger, gating, - noEmit: false, eslintSuppressionRules, flowSuppressions, ignoreUseNoForget,