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 46fbae1f39..cd95500667 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, printFunctionType } from "./Environment"; +import { Environment } from "./Environment"; import { ArrayExpression, ArrayPattern, @@ -2334,6 +2334,14 @@ function lowerExpression( }); } return { kind: "UnsupportedNode", node: exprNode, loc: exprLoc }; + } else if (lvalue.kind === "Global") { + builder.errors.push({ + reason: `(BuildHIR::lowerExpression) Support UpdateExpression where argument is a global`, + severity: ErrorSeverity.Todo, + loc: exprLoc, + suggestions: null, + }); + return { kind: "UnsupportedNode", node: exprNode, loc: exprLoc }; } const value = lowerIdentifier(builder, argument); if (expr.node.prefix) { @@ -3269,24 +3277,11 @@ function lowerIdentifierForAssignment( loc: SourceLocation, kind: InstructionKind, path: NodePath -): Place | null { +): Place | { kind: "Global"; name: string } | null { const identifier = builder.resolveIdentifier(path); if (identifier == null) { if (kind === InstructionKind.Reassign) { - /* - * 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: `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, - }); + return { kind: "Global", name: path.node.name }; } else { // Else its an internal error bc we couldn't find the binding builder.errors.push({ @@ -3295,8 +3290,8 @@ function lowerIdentifierForAssignment( loc: path.node.loc ?? null, suggestions: null, }); + return null; } - return null; } const place: Place = { @@ -3328,6 +3323,14 @@ function lowerAssignment( loc: lvalue.node.loc ?? GeneratedSource, node: lvalue.node, }; + } else if (place.kind === "Global") { + const temporary = lowerValueToTemporary(builder, { + kind: "StoreGlobal", + name: place.name, + value, + loc, + }); + return { kind: "LoadLocal", place: temporary, loc: temporary.loc }; } const isHoistedIdentifier = builder.environment.isHoistedIdentifier( lvalue.node @@ -3452,7 +3455,8 @@ function lowerAssignment( elements.some( (element) => element.isIdentifier() && - getStoreKind(builder, element) !== "StoreLocal" + (getStoreKind(builder, element) !== "StoreLocal" || + builder.resolveIdentifier(element) == null) )); for (let i = 0; i < elements.length; i++) { const element = elements[i]; @@ -3478,6 +3482,14 @@ function lowerAssignment( ); if (identifier === null) { continue; + } else if (identifier.kind === "Global") { + builder.errors.push({ + severity: ErrorSeverity.Todo, + reason: + "Expected reassignment of globals to enable forceTemporaries", + loc: element.node.loc ?? GeneratedSource, + }); + continue; } items.push({ kind: "Spread", @@ -3509,6 +3521,14 @@ function lowerAssignment( ); if (identifier === null) { continue; + } else if (identifier.kind === "Global") { + builder.errors.push({ + severity: ErrorSeverity.Todo, + reason: + "Expected reassignment of globals to enable forceTemporaries", + loc: element.node.loc ?? GeneratedSource, + }); + continue; } items.push(identifier); } else { @@ -3564,7 +3584,10 @@ function lowerAssignment( (property) => property.isRestElement() || (property.isObjectProperty() && - !property.get("value").isIdentifier()) + (!property.get("value").isIdentifier() || + builder.resolveIdentifier( + property.get("value") as NodePath + ) == null)) ); for (let i = 0; i < propertiesPaths.length; i++) { const property = propertiesPaths[i]; @@ -3602,6 +3625,14 @@ function lowerAssignment( ); if (identifier === null) { continue; + } else if (identifier.kind === "Global") { + builder.errors.push({ + severity: ErrorSeverity.Todo, + reason: + "Expected reassignment of globals to enable forceTemporaries", + loc: property.node.loc ?? GeneratedSource, + }); + continue; } properties.push({ kind: "Spread", @@ -3656,6 +3687,14 @@ function lowerAssignment( ); if (identifier === null) { continue; + } else if (identifier.kind === "Global") { + builder.errors.push({ + severity: ErrorSeverity.Todo, + reason: + "Expected reassignment of globals to enable forceTemporaries", + loc: element.node.loc ?? GeneratedSource, + }); + continue; } properties.push({ kind: "ObjectProperty", diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts index b779e0a284..32d060489f 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts @@ -1581,6 +1581,17 @@ function inferBlock( ); const lvalue = instr.lvalue; lvalue.effect = Effect.Store; + + functionEffects.push({ + kind: "GlobalMutation", + error: { + reason: + "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)", + loc: instr.loc, + suggestions: null, + severity: ErrorSeverity.InvalidReact, + }, + }); continue; } case "Destructure": { diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.expect.md new file mode 100644 index 0000000000..610e6f9dd4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.expect.md @@ -0,0 +1,97 @@ + +## Input + +```javascript +import { useEffect, useState } from "react"; + +let someGlobal = false; + +function Component() { + const [state, setState] = useState(someGlobal); + + useEffect(() => { + someGlobal = true; + }, []); + + useEffect(() => { + setState(someGlobal); + }, [someGlobal]); + + return
{String(state)}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { + useEffect, + useState, + unstable_useMemoCache as useMemoCache, +} from "react"; + +let someGlobal = false; + +function Component() { + const $ = useMemoCache(6); + const [state, setState] = useState(someGlobal); + let t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + someGlobal = true; + }; + + t1 = []; + $[0] = t0; + $[1] = t1; + } else { + t0 = $[0]; + t1 = $[1]; + } + useEffect(t0, t1); + let t2; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = () => { + setState(someGlobal); + }; + $[2] = t2; + } else { + t2 = $[2]; + } + let t3; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t3 = [someGlobal]; + $[3] = t3; + } else { + t3 = $[3]; + } + useEffect(t2, t3); + + const t4 = String(state); + let t5; + if ($[4] !== t4) { + t5 =
{t4}
; + $[4] = t4; + $[5] = t5; + } else { + t5 = $[5]; + } + return t5; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
true
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.js new file mode 100644 index 0000000000..a360a20ac8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.js @@ -0,0 +1,22 @@ +import { useEffect, useState } from "react"; + +let someGlobal = false; + +function Component() { + const [state, setState] = useState(someGlobal); + + useEffect(() => { + someGlobal = true; + }, []); + + useEffect(() => { + setState(someGlobal); + }, [someGlobal]); + + return
{String(state)}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; 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 160504dd73..cc4fcbc1cc 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: 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) + | ^ 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) (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 3322bf6c5d..b3303fa189 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: 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: 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) (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 d623b6975f..d9a87f21fe 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: 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) + | ^^^^^^^^^^^^^ Todo: (BuildHIR::lowerExpression) Support UpdateExpression where argument is a global (4:4) 5 | return
; 6 | } 7 | diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.expect.md new file mode 100644 index 0000000000..56132c34c1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.expect.md @@ -0,0 +1,32 @@ + +## Input + +```javascript +function Component() { + const foo = () => { + // Cannot assign to globals + someUnknownGlobal = true; + moduleLocal = true; + }; + // It's possible that this could be an event handler / effect function, + // but we don't know that and conservatively assume it's a render helper + // where it's disallowed to modify globals + return ; +} + +``` + + +## Error + +``` + 2 | const foo = () => { + 3 | // Cannot assign to globals +> 4 | someUnknownGlobal = true; + | ^^^^^^^^^^^^^^^^^ 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 | moduleLocal = true; + 6 | }; + 7 | // It's possible that this could be an event handler / effect function, +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.js new file mode 100644 index 0000000000..926f4c048f --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.js @@ -0,0 +1,11 @@ +function Component() { + const foo = () => { + // Cannot assign to globals + someUnknownGlobal = true; + moduleLocal = true; + }; + // It's possible that this could be an event handler / effect function, + // but we don't know that and conservatively assume it's a render helper + // where it's disallowed to modify globals + return ; +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.expect.md new file mode 100644 index 0000000000..e4073947f7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.expect.md @@ -0,0 +1,29 @@ + +## Input + +```javascript +function Component() { + const foo = () => { + // Cannot assign to globals + someUnknownGlobal = true; + moduleLocal = true; + }; + foo(); +} + +``` + + +## Error + +``` + 2 | const foo = () => { + 3 | // Cannot assign to globals +> 4 | someUnknownGlobal = true; + | ^^^^^^^^^^^^^^^^^ 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 | moduleLocal = true; + 6 | }; + 7 | foo(); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.js new file mode 100644 index 0000000000..708fe643d5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.js @@ -0,0 +1,8 @@ +function Component() { + const foo = () => { + // Cannot assign to globals + someUnknownGlobal = true; + moduleLocal = true; + }; + foo(); +} 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 e3a2859bde..4619cd27cb 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,7 @@ function Component() { 1 | function Component() { 2 | // Cannot assign to globals > 3 | someUnknownGlobal = true; - | ^^^^^^^^^^^^^^^^^^^^^^^^ 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: 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) + | ^^^^^^^^^^^^^^^^^ 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) (3:3) 4 | moduleLocal = true; 5 | } 6 | diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.expect.md new file mode 100644 index 0000000000..dd58d84086 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.expect.md @@ -0,0 +1,47 @@ + +## Input + +```javascript +import { useEffect, useState } from "react"; + +let someGlobal = false; + +function Component() { + const [state, setState] = useState(someGlobal); + + const setGlobal = () => { + // TODO: this should be allowed since setGlobal is only used in an effect + someGlobal = true; + }; + useEffect(() => { + setGlobal(); + }, []); + + useEffect(() => { + setState(someGlobal); + }, [someGlobal]); + + return
{String(state)}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + + +## Error + +``` + 8 | const setGlobal = () => { + 9 | // TODO: this should be allowed since setGlobal is only used in an effect +> 10 | someGlobal = true; + | ^^^^^^^^^^ 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) (10:10) + 11 | }; + 12 | useEffect(() => { + 13 | setGlobal(); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.js new file mode 100644 index 0000000000..0cf7779b8a --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.js @@ -0,0 +1,26 @@ +import { useEffect, useState } from "react"; + +let someGlobal = false; + +function Component() { + const [state, setState] = useState(someGlobal); + + const setGlobal = () => { + // TODO: this should be allowed since setGlobal is only used in an effect + someGlobal = true; + }; + useEffect(() => { + setGlobal(); + }, []); + + useEffect(() => { + setState(someGlobal); + }, [someGlobal]); + + return
{String(state)}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +};