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.
Joe Savona committed
Jun 4, 2023 at 11:29 UTC
e41a8d6a1ea481be0fd1b4b3cbe4930919525546
10 files changed
+129
-95
compiler/forget/src/HIR/BuildHIR.ts
+73
-28
@@ -2439,6 +2439,41 @@ function getLoadKind(
2439
return isContext ? "LoadContext" : "LoadLocal";
2440
}
2441
2442
+function lowerIdentifierForAssignment(
2443
+ builder: HIRBuilder,
2444
+ loc: SourceLocation,
2445
+ kind: InstructionKind,
2446
+ path: NodePath<t.Identifier>
2447
+): Place | null {
2448
+ const identifier = builder.resolveIdentifier(path);
2449
+ if (identifier == null) {
2450
+ if (kind === InstructionKind.Reassign) {
2451
+ // Trying to reassign a global is not allowed
2452
+ builder.errors.push({
2453
+ reason: `(BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported.`,
2454
+ severity: ErrorSeverity.InvalidInput,
2455
+ nodePath: path,
2456
+ });
2457
+ } else {
2458
+ // Else its an internal error bc we couldn't find the binding
2459
+ builder.errors.push({
2460
+ reason: `(BuildHIR::lowerAssignment) Could not find binding for declaration.`,
2461
+ severity: ErrorSeverity.Invariant,
2462
+ nodePath: path,
2463
+ });
2464
+ }
2465
+ return null;
2466
+ }
2467
+
2468
+ const place: Place = {
2469
+ kind: "Identifier",
2470
+ identifier: identifier,
2471
+ effect: Effect.Unknown,
2472
+ loc,
2473
+ };
2474
+ return place;
2475
+}
2476
+
2477
function lowerAssignment(
2478
builder: HIRBuilder,
2479
loc: SourceLocation,
@@ -2450,23 +2485,8 @@ function lowerAssignment(
2485
switch (lvalueNode.type) {
2486
case "Identifier": {
2487
const lvalue = lvaluePath as NodePath<t.Identifier>;
2453
- const identifier = builder.resolveIdentifier(lvalue);
2454
- if (identifier == null) {
2455
- if (kind === InstructionKind.Reassign) {
2456
- // Trying to reassign a global is not allowed
2457
- builder.errors.push({
2458
- reason: `(BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported.`,
2459
- severity: ErrorSeverity.InvalidInput,
2460
- nodePath: lvalue,
2461
- });
2462
- } else {
2463
- // Else its an internal error bc we couldn't find the binding
2464
- builder.errors.push({
2465
- reason: `(BuildHIR::lowerAssignment) Could not find binding for declaration.`,
2466
- severity: ErrorSeverity.Invariant,
2467
- nodePath: lvalue,
2468
- });
2469
- }
2488
+ const place = lowerIdentifierForAssignment(builder, loc, kind, lvalue);
2489
+ if (place === null) {
2490
return {
2491
kind: "UnsupportedNode",
2492
loc: lvalue.node.loc ?? GeneratedSource,
@@ -2474,13 +2494,6 @@ function lowerAssignment(
2494
};
2495
}
2496
2477
- const place: Place = {
2478
- kind: "Identifier",
2479
- identifier: identifier,
2480
- effect: Effect.Unknown,
2481
- loc: lvalue.node.loc ?? GeneratedSource,
2482
- };
2483
-
2497
let temporary;
2498
if (builder.isContextIdentifier(lvalue)) {
2499
if (kind !== InstructionKind.Reassign) {
@@ -2585,13 +2598,29 @@ function lowerAssignment(
2598
});
2599
continue;
2600
}
2588
- const identifier = lowerIdentifier(builder, argument);
2601
+ const identifier = lowerIdentifierForAssignment(
2602
+ builder,
2603
+ element.node.loc ?? GeneratedSource,
2604
+ kind,
2605
+ argument
2606
+ );
2607
+ if (identifier === null) {
2608
+ continue;
2609
+ }
2610
items.push({
2611
kind: "Spread",
2612
place: identifier,
2613
});
2614
} else if (element.isIdentifier()) {
2594
- const identifier = lowerIdentifier(builder, element);
2615
+ const identifier = lowerIdentifierForAssignment(
2616
+ builder,
2617
+ element.node.loc ?? GeneratedSource,
2618
+ kind,
2619
+ element
2620
+ );
2621
+ if (identifier === null) {
2622
+ continue;
2623
+ }
2624
items.push(identifier);
2625
} else {
2626
const temp = buildTemporaryPlace(
@@ -2636,7 +2665,15 @@ function lowerAssignment(
2665
});
2666
continue;
2667
}
2639
- const identifier = lowerIdentifier(builder, argument);
2668
+ const identifier = lowerIdentifierForAssignment(
2669
+ builder,
2670
+ property.node.loc ?? GeneratedSource,
2671
+ kind,
2672
+ argument
2673
+ );
2674
+ if (identifier === null) {
2675
+ continue;
2676
+ }
2677
properties.push({
2678
kind: "Spread",
2679
place: identifier,
@@ -2678,7 +2715,15 @@ function lowerAssignment(
2715
continue;
2716
}
2717
if (element.isIdentifier()) {
2681
- const identifier = lowerIdentifier(builder, element);
2718
+ const identifier = lowerIdentifierForAssignment(
2719
+ builder,
2720
+ element.node.loc ?? GeneratedSource,
2721
+ kind,
2722
+ element
2723
+ );
2724
+ if (identifier === null) {
2725
+ continue;
2726
+ }
2727
properties.push({
2728
kind: "ObjectProperty",
2729
name: key.node.name,
compiler/forget/src/SSA/EnterSSA.ts
+1
-1
@@ -88,7 +88,7 @@ class SSABuilder {
88
CompilerError.invariant(
89
`EnterSSA: Expected identifier to be defined before being used`,
90
oldPlace.loc,
91
- `Identifier ${printIdentifier(oldId)} is undfined`
91
+ `Identifier ${printIdentifier(oldId)} is undefined`
92
);
93
}
94
compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-assignment-to-global.expect.md
deleted
-29
@@ -1,29 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function useFoo(props) {
6
- [x] = props;
7
- return { x };
8
-}
9
-
10
-```
11
-
12
-## Code
13
-
14
-```javascript
15
-import { unstable_useMemoCache as useMemoCache } from "react";
16
-function useFoo(props) {
17
- const $ = useMemoCache(1);
18
- let t0;
19
- if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
20
- t0 = { x };
21
- $[0] = t0;
22
- } else {
23
- t0 = $[0];
24
- }
25
- return t0;
26
-}
27
-
28
-```
29
-
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/_bug.destructure-to-local-global-variables.expect.md
deleted
-35
@@ -1,35 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function Component(props) {
6
- let a;
7
- [a, b] = props.value;
8
-
9
- return [a, b];
10
-}
11
-
12
-```
13
-
14
-## Code
15
-
16
-```javascript
17
-import { unstable_useMemoCache as useMemoCache } from "react";
18
-function Component(props) {
19
- const $ = useMemoCache(2);
20
-
21
- const [a] = props.value;
22
- const c_0 = $[0] !== a;
23
- let t0;
24
- if (c_0) {
25
- t0 = [a, b];
26
- $[0] = a;
27
- $[1] = t0;
28
- } else {
29
- t0 = $[1];
30
- }
31
- return t0;
32
-}
33
-
34
-```
35
-
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md
+1
-1
@@ -17,7 +17,7 @@ function component(a) {
17
## Error
18
19
```
20
-[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$6 is undfined (4:4)
20
+[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$6 is undefined
21
```
22
23
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md
new
+25
@@ -0,0 +1,25 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function useFoo(props) {
6
+ [x] = props;
7
+ return { x };
8
+}
9
+
10
+```
11
+
12
+
13
+## Error
14
+
15
+```
16
+[ReactForget] InvalidInputError: (BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported.
17
+ 1 | function useFoo(props) {
18
+> 2 | [x] = props;
19
+ | ^
20
+ 3 | return { x };
21
+ 4 | }
22
+ 5 |
23
+```
24
+
25
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.js
renamed
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md
new
+28
@@ -0,0 +1,28 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ let a;
7
+ [a, b] = props.value;
8
+
9
+ return [a, b];
10
+}
11
+
12
+```
13
+
14
+
15
+## Error
16
+
17
+```
18
+[ReactForget] InvalidInputError: (BuildHIR::lowerAssignment) Assigning to an identifier defined outside the function scope is not supported.
19
+ 1 | function Component(props) {
20
+ 2 | let a;
21
+> 3 | [a, b] = props.value;
22
+ | ^
23
+ 4 |
24
+ 5 | return [a, b];
25
+ 6 | }
26
+```
27
+
28
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.js
renamed
compiler/forget/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md
+1
-1
@@ -19,7 +19,7 @@ function component(a) {
19
## Error
20
21
```
22
-[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$2 is undfined (7:7)
22
+[ReactForget] Invariant: EnterSSA: Expected identifier to be defined before being used. Identifier x$2 is undefined (7:7)
23
```
24
25
\ No newline at end of file