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.
This commit is contained in:
Jordan Brown
2024-02-14 13:23:32 -05:00
parent 8a3def923b
commit a927040b7a
10 changed files with 209 additions and 0 deletions
@@ -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 });
@@ -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;
@@ -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<IdentifierId, string>();
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,
});
}
}
}
}
}
}
@@ -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";
@@ -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
@@ -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,
};
@@ -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)
```
@@ -0,0 +1,5 @@
// @validateNoCapitalizedCalls
function Foo() {
let x = Bar;
x(); // ERROR
}
@@ -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)
```
@@ -0,0 +1,6 @@
// @validateNoCapitalizedCalls
function Component() {
const x = SomeFunc();
return x;
}