More tests for effect enforcement
Adds test cases to ensure we're correctly inferring mutative builtin operations — property store, computed property store, property deletion, and computed property deletion — as definite mutation and that we're rejecting inputs where these operations are used on immutable/frozen values.
Joe Savona committed
May 25, 2023 at 14:33 UTC
75a19725843486f504bcaf7617d22668a22cbf72
11 files changed
+134
-9
compiler/forget/src/HIR/HIR.ts
+6
-3
@@ -863,7 +863,10 @@ export enum Effect {
863
Store = "store",
864
}
865
866
-export function isMutableEffect(effect: Effect): boolean {
866
+export function isMutableEffect(
867
+ effect: Effect,
868
+ location: SourceLocation
869
+): boolean {
870
switch (effect) {
871
case Effect.Capture:
872
case Effect.Store:
@@ -874,11 +877,11 @@ export function isMutableEffect(effect: Effect): boolean {
877
// All conditional mutations should be resolved into some other effect after InferReferenceEffects
878
CompilerError.invariant(
879
"Unexpected conditional mutation effect",
877
- GeneratedSource
880
+ location
881
);
882
}
883
case Effect.Unknown: {
881
- CompilerError.invariant("Unexpected unknown effect", GeneratedSource);
884
+ CompilerError.invariant("Unexpected unknown effect", location);
885
}
886
case Effect.Read:
887
case Effect.Freeze: {
compiler/forget/src/Inference/InferReferenceEffects.ts
+11
-5
@@ -699,7 +699,7 @@ function inferBlock(
699
operand,
700
operand.effect === Effect.Unknown ? Effect.Read : operand.effect
701
);
702
- hasMutableOperand ||= isMutableEffect(operand.effect);
702
+ hasMutableOperand ||= isMutableEffect(operand.effect, operand.loc);
703
}
704
// If a closure did not capture any mutable values, then we can consider it to be
705
// frozen, which allows it to be independently memoized.
@@ -808,7 +808,7 @@ function inferBlock(
808
case "PropertyDelete": {
809
// `delete` returns a boolean (immutable) and modifies the object
810
valueKind = ValueKind.Immutable;
811
- effectKind = Effect.ConditionallyMutate;
811
+ effectKind = Effect.Mutate;
812
break;
813
}
814
case "PropertyLoad": {
@@ -837,7 +837,7 @@ function inferBlock(
837
state.reference(instrValue.object, Effect.Mutate);
838
state.reference(instrValue.property, Effect.Read);
839
state.initialize(instrValue, ValueKind.Immutable);
840
- state.reference(instr.lvalue, Effect.ConditionallyMutate);
840
+ state.reference(instr.lvalue, Effect.Mutate);
841
continue;
842
}
843
case "ComputedLoad": {
@@ -927,7 +927,10 @@ function inferBlock(
927
state.alias(lvalue, instrValue.value);
928
lvalue.effect = Effect.Store;
929
state.alias(instrValue.lvalue.place, instrValue.value);
930
- // state.reference(instrValue.lvalue.place, Effect.Store);
930
+ // NOTE: *not* using state.reference since this is an assignment.
931
+ // reference() checks if the effect is valid given the value kind,
932
+ // but here the previous value kind doesn't matter since we are
933
+ // replacing it
934
instrValue.lvalue.place.effect = Effect.Store;
935
continue;
936
}
@@ -958,7 +961,10 @@ function inferBlock(
961
lvalue.effect = Effect.Store;
962
for (const place of eachPatternOperand(instrValue.lvalue.pattern)) {
963
state.alias(place, instrValue.value);
961
- // state.reference(place, Effect.Store);
964
+ // NOTE: *not* using state.reference since this is an assignment.
965
+ // reference() checks if the effect is valid given the value kind,
966
+ // but here the previous value kind doesn't matter since we are
967
+ // replacing it
968
place.effect = Effect.Store;
969
}
970
continue;
compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts
+1
-1
@@ -594,7 +594,7 @@ function computeMemoizationInputs(
594
// reachable from a return value. Any mutable rvalue may alias any other rvalue
595
const operands = [...eachReactiveValueOperand(value)];
596
const lvalues = operands
597
- .filter((operand) => isMutableEffect(operand.effect))
597
+ .filter((operand) => isMutableEffect(operand.effect, operand.loc))
598
.map((place) => ({ place, level: MemoizationLevel.Memoized }));
599
if (lvalue !== null) {
600
lvalues.push({ place: lvalue, level: MemoizationLevel.Memoized });
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.expect.md
new
+22
@@ -0,0 +1,22 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const x = makeObject();
7
+ // freeze
8
+ <div>{x}</div>;
9
+ x[0] = true;
10
+ return x;
11
+}
12
+
13
+```
14
+
15
+
16
+## Error
17
+
18
+```
19
+[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $22 (frozen) (5:5)
20
+```
21
+
22
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.js
new
+7
@@ -0,0 +1,7 @@
1
+function Component(props) {
2
+ const x = makeObject();
3
+ // freeze
4
+ <div>{x}</div>;
5
+ x[0] = true;
6
+ return x;
7
+}
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.expect.md
new
+22
@@ -0,0 +1,22 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const x = makeObject();
7
+ // freeze
8
+ <div>{x}</div>;
9
+ delete x[y];
10
+ return x;
11
+}
12
+
13
+```
14
+
15
+
16
+## Error
17
+
18
+```
19
+[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $20 (frozen) (5:5)
20
+```
21
+
22
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.js
new
+7
@@ -0,0 +1,7 @@
1
+function Component(props) {
2
+ const x = makeObject();
3
+ // freeze
4
+ <div>{x}</div>;
5
+ delete x[y];
6
+ return x;
7
+}
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.expect.md
new
+22
@@ -0,0 +1,22 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const x = makeObject();
7
+ // freeze
8
+ <div>{x}</div>;
9
+ delete x.y;
10
+ return x;
11
+}
12
+
13
+```
14
+
15
+
16
+## Error
17
+
18
+```
19
+[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $19 (frozen) (5:5)
20
+```
21
+
22
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.js
new
+7
@@ -0,0 +1,7 @@
1
+function Component(props) {
2
+ const x = makeObject();
3
+ // freeze
4
+ <div>{x}</div>;
5
+ delete x.y;
6
+ return x;
7
+}
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.expect.md
new
+22
@@ -0,0 +1,22 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const x = makeObject();
7
+ // freeze
8
+ <div>{x}</div>;
9
+ x.y = true;
10
+ return x;
11
+}
12
+
13
+```
14
+
15
+
16
+## Error
17
+
18
+```
19
+[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $21 (frozen) (5:5)
20
+```
21
+
22
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.js
new
+7
@@ -0,0 +1,7 @@
1
+function Component(props) {
2
+ const x = makeObject();
3
+ // freeze
4
+ <div>{x}</div>;
5
+ x.y = true;
6
+ return x;
7
+}