From a927040b7a67b2304ad484c903bb56e55f511b3b Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Wed, 14 Feb 2024 13:23:32 -0500 Subject: [PATCH] Add an optional validation to bail out on capitalized function calls Some components stop being components over time and are used as regular functions instead, but they may have lingering hook calls. Those hook calls make it so the capitalized function calling them do not error (they appear to be a function to existing eslint rules), but they are nonetheless unsafe to memoize. This diff adds a conservative option to bail out on all capitalized function calls. There are a handful of known-non-component capitalized functions, like `Boolean`, `String`, and `Number`. This diff also adds the ability to supply capitalized function names that should not be considered in this analysis. I added three tests: 1. Ensure an error occurs in the obvious case 2. Ensure an error occurs when the value is aliased simply 3. Ensure the allowlist works This is my first commit so please go hard on me. I was unsure about where this code should live, so please nitpick. --- .../src/Entrypoint/Pipeline.ts | 5 ++ .../src/HIR/Environment.ts | 18 ++++++ .../Validation/ValidateNoCapitalizedCalls.ts | 61 +++++++++++++++++++ .../src/Validation/index.ts | 1 + .../capitalized-function-allowlist.expect.md | 53 ++++++++++++++++ .../capitalized-function-allowlist.js | 19 ++++++ ...apitalized-function-call-aliased.expect.md | 20 ++++++ ...error.capitalized-function-call-aliased.js | 5 ++ .../error.capitalized-function-call.expect.md | 21 +++++++ .../error.capitalized-function-call.js | 6 ++ 10 files changed, 209 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoCapitalizedCalls.ts create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.js diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index 66cde72c4f..be1fef7d59 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -72,6 +72,7 @@ import { validateContextVariableLValues, validateHooksUsage, validateMemoizedEffectDependencies, + validateNoCapitalizedCalls, validateNoRefAccessInRender, validateNoSetStateInRender, validatePreservedManualMemoization, @@ -154,6 +155,10 @@ function* runWithEnvironment( validateHooksUsage(hir); } + if (env.config.validateNoCapitalizedCalls) { + validateNoCapitalizedCalls(hir); + } + analyseFunctions(hir); yield log({ kind: "hir", name: "AnalyseFunctions", value: hir }); diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts index 0eef92fef7..76f23f315a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts @@ -188,6 +188,18 @@ const EnvironmentConfigSchema = z.object({ */ validateMemoizedEffectDependencies: z.boolean().default(false), + /** + * Validates that there are no capitalized calls other than those allowed by the allowlist. + * Calls to capitalized functions are often functions that used to be components and may + * have lingering hook calls, which makes those calls risky to memoize. + * + * You can specify a list of capitalized calls to allowlist using this option. React Compiler + * always includes its known global functions, including common functions like Boolean and String, + * in this allowlist. You can enable this validation with no additional allowlisted calls by setting + * this option to the empty array. + */ + validateNoCapitalizedCalls: z.nullable(z.array(z.string())).default(null), + /* * When enabled, the compiler assumes that hooks follow the Rules of React: * - Hooks may memoize computation based on any of their parameters, thus @@ -358,6 +370,12 @@ export function parseConfigPragma(pragma: string): EnvironmentConfig { } const keyVal = token.slice(1); let [key, val]: any = keyVal.split(":"); + + if (key === "validateNoCapitalizedCalls") { + maybeConfig[key] = []; + continue; + } + if (typeof defaultConfig[key as keyof EnvironmentConfig] !== "boolean") { // skip parsing non-boolean properties continue; diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoCapitalizedCalls.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoCapitalizedCalls.ts new file mode 100644 index 0000000000..13d49c5be5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoCapitalizedCalls.ts @@ -0,0 +1,61 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +import { CompilerError, EnvironmentConfig } from ".."; +import { HIRFunction, IdentifierId } from "../HIR"; +import { DEFAULT_GLOBALS } from "../HIR/Globals"; + +export function validateNoCapitalizedCalls(fn: HIRFunction): void { + const envConfig: EnvironmentConfig = fn.env.config; + const ALLOW_LIST = new Set([ + ...DEFAULT_GLOBALS.keys(), + ...(envConfig.validateNoCapitalizedCalls ?? []), + ]); + /* + * The hook pattern may allow uppercase names, like React$useState, so we need to be sure that we + * do not error in those cases + */ + const hookPattern = + envConfig.hookPattern != null ? new RegExp(envConfig.hookPattern) : null; + const isAllowed = (name: string): boolean => { + return ( + ALLOW_LIST.has(name) || (hookPattern != null && hookPattern.test(name)) + ); + }; + + const capitalLoadGlobals = new Map(); + for (const [, block] of fn.body.blocks) { + for (const { lvalue, value } of block.instructions) { + switch (value.kind) { + case "LoadGlobal": { + if ( + value.name != "" && + /^[A-Z]/.test(value.name) && + // We don't want to flag CONSTANTS() + !(value.name.toUpperCase() === value.name) && + !isAllowed(value.name) + ) { + capitalLoadGlobals.set(lvalue.identifier.id, value.name); + } + + break; + } + case "CallExpression": { + const calleeIdentifier = value.callee.identifier.id; + const calleeName = capitalLoadGlobals.get(calleeIdentifier); + if (calleeName != null) { + CompilerError.throwInvalidReact({ + reason: `Capitalized function calls may be calling components that use hooks, which make them dangerous to memoize. Ensure there are no hook calls in the function and rename it to begin with a lowercase letter to fix this error`, + description: `${calleeName} may be a component.`, + loc: value.loc, + suggestions: null, + }); + } + } + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts index a6e30b4812..f745b7f626 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts @@ -8,6 +8,7 @@ export { validateContextVariableLValues } from "./ValidateContextVariableLValues"; export { validateHooksUsage } from "./ValidateHooksUsage"; export { validateMemoizedEffectDependencies } from "./ValidateMemoizedEffectDependencies"; +export { validateNoCapitalizedCalls } from "./ValidateNoCapitalizedCalls"; export { validateNoRefAccessInRender } from "./ValidateNoRefAccesInRender"; export { validateNoSetStateInRender } from "./ValidateNoSetStateInRender"; export { validatePreservedManualMemoization } from "./ValidatePreservedManualMemoization"; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.expect.md new file mode 100644 index 0000000000..9b73080c76 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.expect.md @@ -0,0 +1,53 @@ + +## Input + +```javascript +// @validateNoCapitalizedCalls @hookPattern:".*\b(use[^$]+)$" +import * as React from "react"; +const React$useState = React.useState; +const THIS_IS_A_CONSTANT = () => {}; +function Component() { + const b = Boolean(true); // OK + const n = Number(3); // OK + const s = String("foo"); // OK + const [state, setState] = React$useState(0); // OK + const [state2, setState2] = React.useState(1); // OK + const constant = THIS_IS_A_CONSTANT(); // OK + return 3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + isComponent: true, +}; + +``` + +## Code + +```javascript +// @validateNoCapitalizedCalls @hookPattern:".*\b(use[^$]+)$" +import * as React from "react"; +const React$useState = React.useState; +const THIS_IS_A_CONSTANT = () => {}; +function Component() { + Boolean(true); + Number(3); + String("foo"); + React$useState(0); + React.useState(1); + THIS_IS_A_CONSTANT(); + return 3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + isComponent: true, +}; + +``` + +### Eval output +(kind: ok) 3 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.js new file mode 100644 index 0000000000..34c44913a7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capitalized-function-allowlist.js @@ -0,0 +1,19 @@ +// @validateNoCapitalizedCalls @hookPattern:".*\b(use[^$]+)$" +import * as React from "react"; +const React$useState = React.useState; +const THIS_IS_A_CONSTANT = () => {}; +function Component() { + const b = Boolean(true); // OK + const n = Number(3); // OK + const s = String("foo"); // OK + const [state, setState] = React$useState(0); // OK + const [state2, setState2] = React.useState(1); // OK + const constant = THIS_IS_A_CONSTANT(); // OK + return 3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + isComponent: true, +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.expect.md new file mode 100644 index 0000000000..a7e6e81e75 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.expect.md @@ -0,0 +1,20 @@ + +## Input + +```javascript +// @validateNoCapitalizedCalls +function Foo() { + let x = Bar; + x(); // ERROR +} + +``` + + +## Error + +``` +[ReactForget] InvalidReact: Capitalized function calls may be calling components that use hooks, which make them dangerous to memoize. Ensure there are no hook calls in the function and rename it to begin with a lowercase letter to fix this error. Bar may be a component. (4:4) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.js new file mode 100644 index 0000000000..17659c5c32 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call-aliased.js @@ -0,0 +1,5 @@ +// @validateNoCapitalizedCalls +function Foo() { + let x = Bar; + x(); // ERROR +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.expect.md new file mode 100644 index 0000000000..53d30f422d --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.expect.md @@ -0,0 +1,21 @@ + +## Input + +```javascript +// @validateNoCapitalizedCalls +function Component() { + const x = SomeFunc(); + + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidReact: Capitalized function calls may be calling components that use hooks, which make them dangerous to memoize. Ensure there are no hook calls in the function and rename it to begin with a lowercase letter to fix this error. SomeFunc may be a component. (3:3) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.js new file mode 100644 index 0000000000..bfae64ac9e --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.capitalized-function-call.js @@ -0,0 +1,6 @@ +// @validateNoCapitalizedCalls +function Component() { + const x = SomeFunc(); + + return x; +}