From 19476aa5f6f448a97733dc95739b4873a8f8ab12 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 9 Feb 2024 14:21:07 -0800 Subject: [PATCH] Option to bail on Flow react-rule suppressions --- .../src/Entrypoint/Options.ts | 3 + .../src/Entrypoint/Program.ts | 23 ++++--- .../{EslintSuppression.ts => Suppression.ts} | 68 ++++++++++++++----- .../src/Entrypoint/index.ts | 2 +- ...rror.bailout-on-flow-suppression.expect.md | 22 ++++++ .../error.bailout-on-flow-suppression.js | 7 ++ .../no-flow-bailout-unrelated.expect.md | 39 +++++++++++ .../compiler/no-flow-bailout-unrelated.js | 8 +++ .../fixture-test-utils/src/compiler-utils.ts | 13 +++- 9 files changed, 152 insertions(+), 33 deletions(-) rename compiler/packages/babel-plugin-react-forget/src/Entrypoint/{EslintSuppression.ts => Suppression.ts} (67%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.js diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Options.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Options.ts index 90a8bb4fd2..54beb4e7fc 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Options.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Options.ts @@ -102,6 +102,8 @@ export type PluginOptions = { * even if the default ESLint is suppressed), pass an empty array. */ eslintSuppressionRules?: Array | null | undefined; + + flowSuppressions: boolean; }; const CompilationModeSchema = z.enum([ @@ -169,6 +171,7 @@ export const defaultOptions: PluginOptions = { noEmit: false, enableUseMemoCachePolyfill: false, eslintSuppressionRules: null, + flowSuppressions: false, } as const; export function parsePluginOptions(obj: unknown): PluginOptions { diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts index 92bee3fc81..6201af4d0d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts @@ -20,15 +20,15 @@ import { import { CodegenFunction } from "../ReactiveScopes"; import { isComponentDeclaration } from "../Utils/ComponentDeclaration"; import { assertExhaustive } from "../Utils/utils"; -import { - filterEslintSuppressionsThatAffectFunction, - findProgramEslintSuppressions, - suppressionsToCompilerError, -} from "./EslintSuppression"; import { insertGatedFunctionDeclaration } from "./Gating"; import { addImportsToProgram, updateUseMemoCacheImport } from "./Imports"; import { PluginOptions, parsePluginOptions } from "./Options"; import { compileFn } from "./Pipeline"; +import { + filterSuppressionsThatAffectFunction, + findProgramSuppressions, + suppressionsToCompilerError, +} from "./Suppression"; export type CompilerPass = { opts: PluginOptions; @@ -196,11 +196,12 @@ export function compileProgram( * we may still need to run Forget's analysis on every function (even if we * have already encountered errors) for reporting. */ - const eslintSuppressions = findProgramEslintSuppressions( + const suppressions = findProgramSuppressions( pass.comments, - options.eslintSuppressionRules ?? DEFAULT_ESLINT_SUPPRESSIONS + options.eslintSuppressionRules ?? DEFAULT_ESLINT_SUPPRESSIONS, + options.flowSuppressions, ); - const lintError = suppressionsToCompilerError(eslintSuppressions); + const lintError = suppressionsToCompilerError(suppressions); let hasCriticalError = lintError != null; const compiledFns: CompileResult[] = []; @@ -223,9 +224,9 @@ export function compileProgram( * Program node itself. We need to figure out whether an eslint suppression range * applies to this function first. */ - const eslintSuppressionsInFunction = - filterEslintSuppressionsThatAffectFunction(eslintSuppressions, fn); - if (eslintSuppressionsInFunction.length > 0) { + const suppressionsInFunction = + filterSuppressionsThatAffectFunction(suppressions, fn); + if (suppressionsInFunction.length > 0) { handleError(lintError, pass, fn.node.loc ?? null); } } diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/EslintSuppression.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Suppression.ts similarity index 67% rename from compiler/packages/babel-plugin-react-forget/src/Entrypoint/EslintSuppression.ts rename to compiler/packages/babel-plugin-react-forget/src/Entrypoint/Suppression.ts index eec9fd0237..84064cb213 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/EslintSuppression.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Suppression.ts @@ -16,26 +16,31 @@ import { /** * Captures the start and end range of a pair of eslint-disable ... eslint-enable comments. In the - * case of a CommentLine, both the disable and enable point to the same comment. + * case of a CommentLine or a relevant Flow suppression, both the disable and enable point to the + * same comment. * * The enable comment can be missing in the case where only a disable block is present, ie the rest * of the file has potential React violations. */ -export type EslintSuppressionRange = { +export type SuppressionRange = { disableComment: t.Comment; enableComment: t.Comment | null; + source: SuppressionSource; }; +type SuppressionSource = + 'Eslint' | 'Flow' + /** - * An eslint suppression affects a function if: + * An suppression affects a function if: * 1. The suppression is within the function's body; or * 2. The suppression wraps the function */ -export function filterEslintSuppressionsThatAffectFunction( - suppressionRanges: Array, +export function filterSuppressionsThatAffectFunction( + suppressionRanges: Array, fn: NodePath -): Array { - const suppressionsInScope: Array = []; +): Array { + const suppressionsInScope: Array = []; const fnNode = fn.node; for (const suppressionRange of suppressionRanges) { if ( @@ -70,13 +75,15 @@ export function filterEslintSuppressionsThatAffectFunction( return suppressionsInScope; } -export function findProgramEslintSuppressions( +export function findProgramSuppressions( programComments: Array, - ruleNames: Array -): Array { - const suppressionRanges: Array = []; + ruleNames: Array, + flowSuppressions: boolean, +): Array { + const suppressionRanges: Array = []; let disableComment: t.Comment | null = null; let enableComment: t.Comment | null = null; + let source: SuppressionSource | null = null; const rulePattern = `(${ruleNames.join("|")})`; const disableNextLinePattern = new RegExp( @@ -84,6 +91,9 @@ export function findProgramEslintSuppressions( ); const disablePattern = new RegExp(`eslint-disable ${rulePattern}`); const enablePattern = new RegExp(`eslint-enable ${rulePattern}`); + const flowSuppressionPattern = new RegExp( + '\\$(FlowFixMe\\w*|FlowExpectedError|FlowIssue)\\[react\\-rule' + ); for (const comment of programComments) { if (comment.start == null || comment.end == null) { @@ -100,36 +110,48 @@ export function findProgramEslintSuppressions( ) { disableComment = comment; enableComment = comment; + source = 'Eslint'; + } + + if ( + flowSuppressions && + disableComment == null && + flowSuppressionPattern.test(comment.value) + ) { + disableComment = comment; + enableComment = comment; + source = 'Flow'; } if (disablePattern.test(comment.value)) { disableComment = comment; + source = 'Eslint'; } - if (enablePattern.test(comment.value)) { + if (enablePattern.test(comment.value) && source === 'Eslint') { enableComment = comment; } - if (disableComment != null) { + if (disableComment != null && source != null) { suppressionRanges.push({ disableComment: disableComment, enableComment: enableComment, + source, }); disableComment = null; enableComment = null; + source = null; } } return suppressionRanges; } export function suppressionsToCompilerError( - suppressionRanges: Array + suppressionRanges: Array ): CompilerError | null { if (suppressionRanges.length === 0) { return null; } - const reason = - "React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior"; const error = new CompilerError(); for (const suppressionRange of suppressionRanges) { if ( @@ -138,15 +160,25 @@ export function suppressionsToCompilerError( ) { continue; } + let reason, suggestion; + switch (suppressionRange.source) { + case 'Eslint': + reason = "React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled"; + suggestion = "Remove the eslint disable"; + break; + case 'Flow': + reason = "React Forget has bailed out of optimizing this component as one or more React rule violations were reported by Flow"; + suggestion = "Remove the Flow suppression and address the React error"; + } error.pushErrorDetail( new CompilerErrorDetail({ - reason, + reason: `${reason}. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior`, description: suppressionRange.disableComment.value.trim(), severity: ErrorSeverity.InvalidReact, loc: suppressionRange.disableComment.loc ?? null, suggestions: [ { - description: "Remove the eslint disable", + description: suggestion, range: [ suppressionRange.disableComment.start, suppressionRange.disableComment.end, diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/index.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/index.ts index 25ad79f5fa..9118d04ec2 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/index.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/index.ts @@ -5,9 +5,9 @@ * LICENSE file in the root directory of this source tree. */ -export * from "./EslintSuppression"; export * from "./Gating"; export * from "./Imports"; export * from "./Options"; export * from "./Pipeline"; export * from "./Program"; +export * from "./Suppression"; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.expect.md new file mode 100644 index 0000000000..d1cbeaff62 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.expect.md @@ -0,0 +1,22 @@ + +## Input + +```javascript +// @enableFlowSuppressions + +function Foo(props) { + // $FlowFixMe[react-rule-hook] + useX(); + return null; +} + +``` + + +## Error + +``` +[ReactForget] InvalidReact: React Forget has bailed out of optimizing this component as one or more React rule violations were reported by Flow. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior. $FlowFixMe[react-rule-hook] (4:4) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.js new file mode 100644 index 0000000000..9a3e99daab --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bailout-on-flow-suppression.js @@ -0,0 +1,7 @@ +// @enableFlowSuppressions + +function Foo(props) { + // $FlowFixMe[react-rule-hook] + useX(); + return null; +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.expect.md new file mode 100644 index 0000000000..4b2b5633e7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +// @enableFlowSuppressions + +function Foo(props) { + // $FlowFixMe[incompatible-type] + useX(); + const x = new Foo(...props.foo, null, ...[props.bar]); + return x; +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; // @enableFlowSuppressions + +function Foo(props) { + const $ = useMemoCache(3); + + useX(); + let t0; + if ($[0] !== props.bar || $[1] !== props.foo) { + t0 = new Foo(...props.foo, null, ...[props.bar]); + $[0] = props.bar; + $[1] = props.foo; + $[2] = t0; + } else { + t0 = $[2]; + } + const x = t0; + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.js new file mode 100644 index 0000000000..f99299f45e --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/no-flow-bailout-unrelated.js @@ -0,0 +1,8 @@ +// @enableFlowSuppressions + +function Foo(props) { + // $FlowFixMe[incompatible-type] + useX(); + const x = new Foo(...props.foo, null, ...[props.bar]); + return x; +} diff --git a/compiler/packages/fixture-test-utils/src/compiler-utils.ts b/compiler/packages/fixture-test-utils/src/compiler-utils.ts index 59d773182b..b0d8d92128 100644 --- a/compiler/packages/fixture-test-utils/src/compiler-utils.ts +++ b/compiler/packages/fixture-test-utils/src/compiler-utils.ts @@ -93,6 +93,12 @@ export function transformFixtureInput( eslintSuppressionRules = eslintSuppressionMatch[1].split("|"); } + let flowSuppressions: boolean = false; + if (firstLine.includes("@enableFlowSuppressions")) { + flowSuppressions = true; + } + + const hookPatternMatch = /@hookPattern:"([^"]+)"/.exec(firstLine); if ( hookPatternMatch && @@ -156,6 +162,7 @@ export function transformFixtureInput( noEmit: false, enableUseMemoCachePolyfill, eslintSuppressionRules, + flowSuppressions, }, includeAst ); @@ -165,9 +172,9 @@ export function transformFixtureInput( code: result.code != null ? prettier.format(result.code, { - semi: true, - parser: language === "typescript" ? "babel-ts" : "flow", - }) + semi: true, + parser: language === "typescript" ? "babel-ts" : "flow", + }) : result.code, }; }