From c84ff16afba0d5b72d0252e0e96be71fcb16848f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 18 Apr 2024 07:46:22 -0700 Subject: [PATCH] [dx] Update error message for reassigning globals ghstack-source-id: 48ce4b55a8b4b5a22c8ec29f6787732ab2a67778 Pull Request resolved: https://github.com/facebook/react-forget/pull/2860 --- .../babel-plugin-react-forget/src/HIR/BuildHIR.ts | 13 ++++++++++--- .../src/HIR/Environment.ts | 14 ++++++++++++++ ...alid-destructure-assignment-to-global.expect.md | 2 +- ...destructure-to-local-global-variables.expect.md | 2 +- ...ate-global-increment-op-invalid-react.expect.md | 2 +- .../error.reassignment-to-global.expect.md | 4 ++-- 6 files changed, 29 insertions(+), 8 deletions(-) diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts index f8ea459f0b..4addb51f87 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts @@ -16,7 +16,7 @@ import { } from "../CompilerError"; import { Err, Ok, Result } from "../Utils/Result"; import { assertExhaustive, hasNode } from "../Utils/utils"; -import { Environment } from "./Environment"; +import { Environment, printFunctionType } from "./Environment"; import { ArrayExpression, ArrayPattern, @@ -3273,9 +3273,16 @@ function lowerIdentifierForAssignment( const identifier = builder.resolveIdentifier(path); if (identifier == null) { if (kind === InstructionKind.Reassign) { - // Trying to reassign a global is not allowed + /* + * Trying to reassign a global is not allowed + * TODO: add support for StoreGlobal or similar, and move this error to run conditionally only for render-phase + * functions in InferReferenceEffects + */ builder.errors.push({ - reason: `This reassigns a variable which was not defined inside of the component. Components should be pure and side-effect free. If this variable is used in rendering, use useState instead. (https://react.dev/learn/keeping-components-pure)`, + reason: `Unexpected reassignment of a variable which was defined outside of the ${printFunctionType( + builder.environment.fnType + )}`, + description: `Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)`, severity: ErrorSeverity.InvalidReact, loc: path.parentPath.node.loc ?? null, suggestions: null, 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 6ada70c488..471fa5798b 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts @@ -405,6 +405,20 @@ export type PartialEnvironmentConfig = Partial; export type ReactFunctionType = "Component" | "Hook" | "Other"; +export function printFunctionType(type: ReactFunctionType): string { + switch (type) { + case "Component": { + return "component"; + } + case "Hook": { + return "hook"; + } + default: { + return "function"; + } + } +} + export class Environment { #globals: GlobalRegistry; #shapes: ShapeRegistry; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md index 34da8be98e..160504dd73 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md @@ -15,7 +15,7 @@ function useFoo(props) { ``` 1 | function useFoo(props) { > 2 | [x] = props; - | ^^^ InvalidReact: This reassigns a variable which was not defined inside of the component. Components should be pure and side-effect free. If this variable is used in rendering, use useState instead. (https://react.dev/learn/keeping-components-pure) (2:2) + | ^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (2:2) 3 | return { x }; 4 | } 5 | diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md index 831b04b65e..3322bf6c5d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md @@ -18,7 +18,7 @@ function Component(props) { 1 | function Component(props) { 2 | let a; > 3 | [a, b] = props.value; - | ^^^^^^ InvalidReact: This reassigns a variable which was not defined inside of the component. Components should be pure and side-effect free. If this variable is used in rendering, use useState instead. (https://react.dev/learn/keeping-components-pure) (3:3) + | ^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3) 4 | 5 | return [a, b]; 6 | } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-global-increment-op-invalid-react.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-global-increment-op-invalid-react.expect.md index 27280ebfe4..d623b6975f 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-global-increment-op-invalid-react.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-global-increment-op-invalid-react.expect.md @@ -18,7 +18,7 @@ function NoHooks() { 2 | 3 | function NoHooks() { > 4 | renderCount++; - | ^^^^^^^^^^^^^ InvalidReact: This reassigns a variable which was not defined inside of the component. Components should be pure and side-effect free. If this variable is used in rendering, use useState instead. (https://react.dev/learn/keeping-components-pure) (4:4) + | ^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4) 5 | return
; 6 | } 7 | diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global.expect.md index ed1fe2fb59..e3a2859bde 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global.expect.md @@ -17,9 +17,9 @@ function Component() { 1 | function Component() { 2 | // Cannot assign to globals > 3 | someUnknownGlobal = true; - | ^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: This reassigns a variable which was not defined inside of the component. Components should be pure and side-effect free. If this variable is used in rendering, use useState instead. (https://react.dev/learn/keeping-components-pure) (3:3) + | ^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3) -InvalidReact: This reassigns a variable which was not defined inside of the component. Components should be pure and side-effect free. If this variable is used in rendering, use useState instead. (https://react.dev/learn/keeping-components-pure) (4:4) +InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4) 4 | moduleLocal = true; 5 | } 6 |