From e41a8d6a1ea481be0fd1b4b3cbe4930919525546 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sun, 4 Jun 2023 11:29:55 -0700 Subject: [PATCH] Validate destructuring assignment to globals Fixes one more category of bug. For assignment expressions, we validating against redeclaring a global variable when the assignment target was an identifier, but not when the global was reassigned via destructuring. This PR adds a `lowerIdentifierForAssignment()` helper and uses it for assignment of all identifier variants, including destructuring. --- compiler/forget/src/HIR/BuildHIR.ts | 101 +++++++++++++----- compiler/forget/src/SSA/EnterSSA.ts | 2 +- ...destructure-assignment-to-global.expect.md | 29 ----- ...ucture-to-local-global-variables.expect.md | 35 ------ ...ror.hoisted-function-declaration.expect.md | 2 +- ...destructure-assignment-to-global.expect.md | 25 +++++ ...valid-destructure-assignment-to-global.js} | 0 ...ucture-to-local-global-variables.expect.md | 28 +++++ ...-destructure-to-local-global-variables.js} | 0 ...r.mutate-captured-arg-separately.expect.md | 2 +- 10 files changed, 129 insertions(+), 95 deletions(-) delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-assignment-to-global.expect.md delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-to-local-global-variables.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.destructure-assignment-to-global.js => error.invalid-destructure-assignment-to-global.js} (100%) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.destructure-to-local-global-variables.js => error.invalid-destructure-to-local-global-variables.js} (100%) diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index c393662714..a81ebd2180 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -2439,6 +2439,41 @@ function getLoadKind( return isContext ? "LoadContext" : "LoadLocal"; } +function lowerIdentifierForAssignment( + builder: HIRBuilder, + loc: SourceLocation, + kind: InstructionKind, + path: NodePath +): Place | null { + const identifier = builder.resolveIdentifier(path); + if (identifier == null) { + if (kind === InstructionKind.Reassign) { + // Trying to reassign a global is not allowed + builder.errors.push({ + reason: `(BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported.`, + severity: ErrorSeverity.InvalidInput, + nodePath: path, + }); + } else { + // Else its an internal error bc we couldn't find the binding + builder.errors.push({ + reason: `(BuildHIR::lowerAssignment) Could not find binding for declaration.`, + severity: ErrorSeverity.Invariant, + nodePath: path, + }); + } + return null; + } + + const place: Place = { + kind: "Identifier", + identifier: identifier, + effect: Effect.Unknown, + loc, + }; + return place; +} + function lowerAssignment( builder: HIRBuilder, loc: SourceLocation, @@ -2450,23 +2485,8 @@ function lowerAssignment( switch (lvalueNode.type) { case "Identifier": { const lvalue = lvaluePath as NodePath; - const identifier = builder.resolveIdentifier(lvalue); - if (identifier == null) { - if (kind === InstructionKind.Reassign) { - // Trying to reassign a global is not allowed - builder.errors.push({ - reason: `(BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported.`, - severity: ErrorSeverity.InvalidInput, - nodePath: lvalue, - }); - } else { - // Else its an internal error bc we couldn't find the binding - builder.errors.push({ - reason: `(BuildHIR::lowerAssignment) Could not find binding for declaration.`, - severity: ErrorSeverity.Invariant, - nodePath: lvalue, - }); - } + const place = lowerIdentifierForAssignment(builder, loc, kind, lvalue); + if (place === null) { return { kind: "UnsupportedNode", loc: lvalue.node.loc ?? GeneratedSource, @@ -2474,13 +2494,6 @@ function lowerAssignment( }; } - const place: Place = { - kind: "Identifier", - identifier: identifier, - effect: Effect.Unknown, - loc: lvalue.node.loc ?? GeneratedSource, - }; - let temporary; if (builder.isContextIdentifier(lvalue)) { if (kind !== InstructionKind.Reassign) { @@ -2585,13 +2598,29 @@ function lowerAssignment( }); continue; } - const identifier = lowerIdentifier(builder, argument); + const identifier = lowerIdentifierForAssignment( + builder, + element.node.loc ?? GeneratedSource, + kind, + argument + ); + if (identifier === null) { + continue; + } items.push({ kind: "Spread", place: identifier, }); } else if (element.isIdentifier()) { - const identifier = lowerIdentifier(builder, element); + const identifier = lowerIdentifierForAssignment( + builder, + element.node.loc ?? GeneratedSource, + kind, + element + ); + if (identifier === null) { + continue; + } items.push(identifier); } else { const temp = buildTemporaryPlace( @@ -2636,7 +2665,15 @@ function lowerAssignment( }); continue; } - const identifier = lowerIdentifier(builder, argument); + const identifier = lowerIdentifierForAssignment( + builder, + property.node.loc ?? GeneratedSource, + kind, + argument + ); + if (identifier === null) { + continue; + } properties.push({ kind: "Spread", place: identifier, @@ -2678,7 +2715,15 @@ function lowerAssignment( continue; } if (element.isIdentifier()) { - const identifier = lowerIdentifier(builder, element); + const identifier = lowerIdentifierForAssignment( + builder, + element.node.loc ?? GeneratedSource, + kind, + element + ); + if (identifier === null) { + continue; + } properties.push({ kind: "ObjectProperty", name: key.node.name, diff --git a/compiler/forget/src/SSA/EnterSSA.ts b/compiler/forget/src/SSA/EnterSSA.ts index 073b220882..4e78c359a8 100644 --- a/compiler/forget/src/SSA/EnterSSA.ts +++ b/compiler/forget/src/SSA/EnterSSA.ts @@ -88,7 +88,7 @@ class SSABuilder { CompilerError.invariant( `EnterSSA: Expected identifier to be defined before being used`, oldPlace.loc, - `Identifier ${printIdentifier(oldId)} is undfined` + `Identifier ${printIdentifier(oldId)} is undefined` ); } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-assignment-to-global.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-assignment-to-global.expect.md deleted file mode 100644 index 351aee8e97..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-assignment-to-global.expect.md +++ /dev/null @@ -1,29 +0,0 @@ - -## Input - -```javascript -function useFoo(props) { - [x] = props; - return { x }; -} - -``` - -## Code - -```javascript -import { unstable_useMemoCache as useMemoCache } from "react"; -function useFoo(props) { - const $ = useMemoCache(1); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = { x }; - $[0] = t0; - } else { - t0 = $[0]; - } - return t0; -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-to-local-global-variables.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-to-local-global-variables.expect.md deleted file mode 100644 index 3c96bb23ae..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-to-local-global-variables.expect.md +++ /dev/null @@ -1,35 +0,0 @@ - -## Input - -```javascript -function Component(props) { - let a; - [a, b] = props.value; - - return [a, b]; -} - -``` - -## Code - -```javascript -import { unstable_useMemoCache as useMemoCache } from "react"; -function Component(props) { - const $ = useMemoCache(2); - - const [a] = props.value; - const c_0 = $[0] !== a; - let t0; - if (c_0) { - t0 = [a, b]; - $[0] = a; - $[1] = t0; - } else { - t0 = $[1]; - } - return t0; -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md index 12a57c3c19..9f531de0cf 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md @@ -17,7 +17,7 @@ function component(a) { ## Error ``` -[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$6 is undfined (4:4) +[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$6 is undefined ``` \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md new file mode 100644 index 0000000000..67aeaf1bd2 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md @@ -0,0 +1,25 @@ + +## Input + +```javascript +function useFoo(props) { + [x] = props; + return { x }; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInputError: (BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported. + 1 | function useFoo(props) { +> 2 | [x] = props; + | ^ + 3 | return { x }; + 4 | } + 5 | +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-assignment-to-global.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-assignment-to-global.js rename to compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md new file mode 100644 index 0000000000..21447f418e --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md @@ -0,0 +1,28 @@ + +## Input + +```javascript +function Component(props) { + let a; + [a, b] = props.value; + + return [a, b]; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInputError: (BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported. + 1 | function Component(props) { + 2 | let a; +> 3 | [a, b] = props.value; + | ^ + 4 | + 5 | return [a, b]; + 6 | } +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-to-local-global-variables.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-to-local-global-variables.js rename to compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md index b23037e49a..967b7a86e8 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md @@ -19,7 +19,7 @@ function component(a) { ## Error ``` -[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$2 is undfined (7:7) +[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$2 is undefined (7:7) ``` \ No newline at end of file