From 75a19725843486f504bcaf7617d22668a22cbf72 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 25 May 2023 14:33:02 -0700 Subject: [PATCH] More tests for effect enforcement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- compiler/forget/src/HIR/HIR.ts | 9 +++++--- .../src/Inference/InferReferenceEffects.ts | 16 +++++++++----- .../ReactiveScopes/PruneNonEscapingScopes.ts | 2 +- ...d-computed-store-to-frozen-value.expect.md | 22 +++++++++++++++++++ ....invalid-computed-store-to-frozen-value.js | 7 ++++++ ...omputed-property-of-frozen-value.expect.md | 22 +++++++++++++++++++ ...elete-computed-property-of-frozen-value.js | 7 ++++++ ...-delete-property-of-frozen-value.expect.md | 22 +++++++++++++++++++ ...invalid-delete-property-of-frozen-value.js | 7 ++++++ ...d-property-store-to-frozen-value.expect.md | 22 +++++++++++++++++++ ....invalid-property-store-to-frozen-value.js | 7 ++++++ 11 files changed, 134 insertions(+), 9 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.js diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index b3bac241b8..80d405268a 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -863,7 +863,10 @@ export enum Effect { Store = "store", } -export function isMutableEffect(effect: Effect): boolean { +export function isMutableEffect( + effect: Effect, + location: SourceLocation +): boolean { switch (effect) { case Effect.Capture: case Effect.Store: @@ -874,11 +877,11 @@ export function isMutableEffect(effect: Effect): boolean { // All conditional mutations should be resolved into some other effect after InferReferenceEffects CompilerError.invariant( "Unexpected conditional mutation effect", - GeneratedSource + location ); } case Effect.Unknown: { - CompilerError.invariant("Unexpected unknown effect", GeneratedSource); + CompilerError.invariant("Unexpected unknown effect", location); } case Effect.Read: case Effect.Freeze: { diff --git a/compiler/forget/src/Inference/InferReferenceEffects.ts b/compiler/forget/src/Inference/InferReferenceEffects.ts index d51ba02c00..700e8c462c 100644 --- a/compiler/forget/src/Inference/InferReferenceEffects.ts +++ b/compiler/forget/src/Inference/InferReferenceEffects.ts @@ -699,7 +699,7 @@ function inferBlock( operand, operand.effect === Effect.Unknown ? Effect.Read : operand.effect ); - hasMutableOperand ||= isMutableEffect(operand.effect); + hasMutableOperand ||= isMutableEffect(operand.effect, operand.loc); } // If a closure did not capture any mutable values, then we can consider it to be // frozen, which allows it to be independently memoized. @@ -808,7 +808,7 @@ function inferBlock( case "PropertyDelete": { // `delete` returns a boolean (immutable) and modifies the object valueKind = ValueKind.Immutable; - effectKind = Effect.ConditionallyMutate; + effectKind = Effect.Mutate; break; } case "PropertyLoad": { @@ -837,7 +837,7 @@ function inferBlock( state.reference(instrValue.object, Effect.Mutate); state.reference(instrValue.property, Effect.Read); state.initialize(instrValue, ValueKind.Immutable); - state.reference(instr.lvalue, Effect.ConditionallyMutate); + state.reference(instr.lvalue, Effect.Mutate); continue; } case "ComputedLoad": { @@ -927,7 +927,10 @@ function inferBlock( state.alias(lvalue, instrValue.value); lvalue.effect = Effect.Store; state.alias(instrValue.lvalue.place, instrValue.value); - // state.reference(instrValue.lvalue.place, Effect.Store); + // NOTE: *not* using state.reference since this is an assignment. + // reference() checks if the effect is valid given the value kind, + // but here the previous value kind doesn't matter since we are + // replacing it instrValue.lvalue.place.effect = Effect.Store; continue; } @@ -958,7 +961,10 @@ function inferBlock( lvalue.effect = Effect.Store; for (const place of eachPatternOperand(instrValue.lvalue.pattern)) { state.alias(place, instrValue.value); - // state.reference(place, Effect.Store); + // NOTE: *not* using state.reference since this is an assignment. + // reference() checks if the effect is valid given the value kind, + // but here the previous value kind doesn't matter since we are + // replacing it place.effect = Effect.Store; } continue; diff --git a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts index 0db85aa470..18bc3c9104 100644 --- a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -594,7 +594,7 @@ function computeMemoizationInputs( // reachable from a return value. Any mutable rvalue may alias any other rvalue const operands = [...eachReactiveValueOperand(value)]; const lvalues = operands - .filter((operand) => isMutableEffect(operand.effect)) + .filter((operand) => isMutableEffect(operand.effect, operand.loc)) .map((place) => ({ place, level: MemoizationLevel.Memoized })); if (lvalue !== null) { lvalues.push({ place: lvalue, level: MemoizationLevel.Memoized }); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.expect.md new file mode 100644 index 0000000000..66d0839c20 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.expect.md @@ -0,0 +1,22 @@ + +## Input + +```javascript +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + x[0] = true; + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $22 (frozen) (5:5) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.js new file mode 100644 index 0000000000..a0741c5a58 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-computed-store-to-frozen-value.js @@ -0,0 +1,7 @@ +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + x[0] = true; + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.expect.md new file mode 100644 index 0000000000..6d0acd75d8 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.expect.md @@ -0,0 +1,22 @@ + +## Input + +```javascript +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + delete x[y]; + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $20 (frozen) (5:5) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.js new file mode 100644 index 0000000000..d9b47447d3 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-computed-property-of-frozen-value.js @@ -0,0 +1,7 @@ +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + delete x[y]; + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.expect.md new file mode 100644 index 0000000000..973d80490a --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.expect.md @@ -0,0 +1,22 @@ + +## Input + +```javascript +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + delete x.y; + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $19 (frozen) (5:5) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.js new file mode 100644 index 0000000000..f4239d7f8a --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-delete-property-of-frozen-value.js @@ -0,0 +1,7 @@ +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + delete x.y; + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.expect.md new file mode 100644 index 0000000000..efb0e09e58 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.expect.md @@ -0,0 +1,22 @@ + +## Input + +```javascript +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + x.y = true; + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $21 (frozen) (5:5) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.js new file mode 100644 index 0000000000..ff54bb789a --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-property-store-to-frozen-value.js @@ -0,0 +1,7 @@ +function Component(props) { + const x = makeObject(); + // freeze +
{x}
; + x.y = true; + return x; +}