From a5d2f8eba177a626092bdaea28156749330c1a38 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Wed, 6 Dec 2023 14:30:25 -0800 Subject: [PATCH] Update reference tracking to match stable-cadence --- runtime/attachments_test.go | 2 +- runtime/convertValues_test.go | 2 +- runtime/interpreter/interpreter.go | 50 +-------------- runtime/interpreter/interpreter_expression.go | 44 ++++++++++--- runtime/interpreter/value.go | 13 +--- runtime/interpreter/value_function.go | 2 +- runtime/interpreter/value_test.go | 1 + runtime/runtime_test.go | 2 +- runtime/storage_test.go | 15 ++--- runtime/tests/interpreter/attachments_test.go | 4 +- .../interpreter/container_mutation_test.go | 4 +- runtime/tests/interpreter/interpreter_test.go | 1 + runtime/tests/interpreter/reference_test.go | 13 +++- runtime/tests/interpreter/resources_test.go | 64 ++++--------------- 14 files changed, 78 insertions(+), 139 deletions(-) diff --git a/runtime/attachments_test.go b/runtime/attachments_test.go index 9d186fdfb7..f512cbfeab 100644 --- a/runtime/attachments_test.go +++ b/runtime/attachments_test.go @@ -230,7 +230,7 @@ func TestRuntimeAccountAttachmentExportFailure(t *testing.T) { }, ) require.Error(t, err) - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) } func TestRuntimeAccountAttachmentExport(t *testing.T) { diff --git a/runtime/convertValues_test.go b/runtime/convertValues_test.go index a86bcaedff..e0635104f7 100644 --- a/runtime/convertValues_test.go +++ b/runtime/convertValues_test.go @@ -5286,7 +5286,7 @@ func TestRuntimeDestroyedResourceReferenceExport(t *testing.T) { }, ) require.Error(t, err) - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) } func TestRuntimeDeploymentResultValueImportExport(t *testing.T) { diff --git a/runtime/interpreter/interpreter.go b/runtime/interpreter/interpreter.go index ac094232cf..0073ac2720 100644 --- a/runtime/interpreter/interpreter.go +++ b/runtime/interpreter/interpreter.go @@ -4979,41 +4979,6 @@ func (interpreter *Interpreter) checkContainerMutation( } } -func (interpreter *Interpreter) checkReferencedResourceNotDestroyed(value Value, locationRange LocationRange) { - resourceKindedValue, ok := value.(ResourceKindedValue) - if !ok || !resourceKindedValue.IsDestroyed() { - return - } - - panic(DestroyedResourceError{ - LocationRange: locationRange, - }) -} - -func (interpreter *Interpreter) checkReferencedResourceNotMovedOrDestroyed( - referencedValue Value, - locationRange LocationRange, -) { - - // First check if the referencedValue is a resource. - // This is to handle optionals, since optionals does not - // belong to `ReferenceTrackedResourceKindedValue` - - resourceKindedValue, ok := referencedValue.(ResourceKindedValue) - if ok && resourceKindedValue.IsDestroyed() { - panic(DestroyedResourceError{ - LocationRange: locationRange, - }) - } - - referenceTrackedResourceKindedValue, ok := referencedValue.(ReferenceTrackedResourceKindedValue) - if ok && referenceTrackedResourceKindedValue.IsStaleResource(interpreter) { - panic(InvalidatedResourceReferenceError{ - LocationRange: locationRange, - }) - } -} - func (interpreter *Interpreter) RemoveReferencedSlab(storable atree.Storable) { storageIDStorable, ok := storable.(atree.StorageIDStorable) if !ok { @@ -5192,6 +5157,7 @@ func (interpreter *Interpreter) trackReferencedResourceKindedValue( values[value] = struct{}{} } +// TODO: Remove the `destroyed` flag func (interpreter *Interpreter) invalidateReferencedResources(value Value, destroyed bool) { // skip non-resource typed values if !value.IsResourceKinded(interpreter) { @@ -5234,19 +5200,7 @@ func (interpreter *Interpreter) invalidateReferencedResources(value Value, destr } for value := range values { //nolint:maprange - switch value := value.Value.(type) { - case *CompositeValue: - value.dictionary = nil - value.isDestroyed = destroyed - case *DictionaryValue: - value.dictionary = nil - value.isDestroyed = destroyed - case *ArrayValue: - value.array = nil - value.isDestroyed = destroyed - default: - panic(errors.NewUnreachableError()) - } + value.Value = nil } // The old resource instances are already cleared/invalidated above. diff --git a/runtime/interpreter/interpreter_expression.go b/runtime/interpreter/interpreter_expression.go index 5f04bdaefc..ee05fce8e7 100644 --- a/runtime/interpreter/interpreter_expression.go +++ b/runtime/interpreter/interpreter_expression.go @@ -358,18 +358,44 @@ func (interpreter *Interpreter) VisitIdentifierExpression(expression *ast.Identi func (interpreter *Interpreter) evalExpression(expression ast.Expression) Value { result := ast.AcceptExpression[Value](expression, interpreter) + interpreter.checkInvalidatedResourceOrResourceReference(result, expression) + return result +} - resourceKindedValue, ok := result.(ResourceKindedValue) - if ok && resourceKindedValue.isInvalidatedResource(interpreter) { - panic(InvalidatedResourceError{ - LocationRange: LocationRange{ - Location: interpreter.Location, - HasPosition: expression, - }, - }) +func (interpreter *Interpreter) checkInvalidatedResourceOrResourceReference(value Value, hasPosition ast.HasPosition) { + // Unwrap SomeValue, to access references wrapped inside optionals. + someValue, isSomeValue := value.(*SomeValue) + for isSomeValue && someValue.value != nil { + value = someValue.value + someValue, isSomeValue = value.(*SomeValue) } - return result + switch value := value.(type) { + case ResourceKindedValue: + if value.isInvalidatedResource(interpreter) { + panic(InvalidatedResourceError{ + LocationRange: LocationRange{ + Location: interpreter.Location, + HasPosition: hasPosition, + }, + }) + } + case *EphemeralReferenceValue: + if value.Value == nil { + panic(InvalidatedResourceReferenceError{ + LocationRange: LocationRange{ + Location: interpreter.Location, + HasPosition: hasPosition, + }, + }) + } else { + // If the value is there, check is the referenced value is an invalidated one. + // This step is not really needed, since reference tracking is supposed to clear the + // `value.Value` if the referenced-value was moved/deleted. + // However, have this as a second layer of defensive. + interpreter.checkInvalidatedResourceOrResourceReference(value.Value, hasPosition) + } + } } func (interpreter *Interpreter) VisitBinaryExpression(expression *ast.BinaryExpression) Value { diff --git a/runtime/interpreter/value.go b/runtime/interpreter/value.go index 0d3eb6bf4f..3e63b788c8 100644 --- a/runtime/interpreter/value.go +++ b/runtime/interpreter/value.go @@ -19845,11 +19845,7 @@ func (v *StorageReferenceValue) mustReferencedValue( }) } - self := *referencedValue - - interpreter.checkReferencedResourceNotDestroyed(self, locationRange) - - return self + return *referencedValue } func (v *StorageReferenceValue) GetMember( @@ -20245,10 +20241,7 @@ func (v *EphemeralReferenceValue) MustReferencedValue( }) } - self := *referencedValue - - interpreter.checkReferencedResourceNotMovedOrDestroyed(self, locationRange) - return self + return *referencedValue } func (v *EphemeralReferenceValue) GetMember( @@ -20401,8 +20394,6 @@ func (v *EphemeralReferenceValue) ConformsToStaticType( return false } - interpreter.checkReferencedResourceNotMovedOrDestroyed(*referencedValue, locationRange) - self := *referencedValue staticType := self.StaticType(interpreter) diff --git a/runtime/interpreter/value_function.go b/runtime/interpreter/value_function.go index 29ab68205c..e4ce9f64fe 100644 --- a/runtime/interpreter/value_function.go +++ b/runtime/interpreter/value_function.go @@ -406,7 +406,7 @@ func (f BoundFunctionValue) invoke(invocation Invocation) Value { invocation.BoundAuthorization = f.BoundAuthorization // Check if the 'self' is not invalidated. - _ = f.selfRef.MustReferencedValue(invocation.Interpreter, invocation.LocationRange) + invocation.Interpreter.checkInvalidatedResourceOrResourceReference(f.selfRef, invocation.LocationRange) return f.Function.invoke(invocation) } diff --git a/runtime/interpreter/value_test.go b/runtime/interpreter/value_test.go index af63987a73..a3b00b8985 100644 --- a/runtime/interpreter/value_test.go +++ b/runtime/interpreter/value_test.go @@ -1118,6 +1118,7 @@ func TestStringer(t *testing.T) { common.ZeroAddress, ) arrayRef := NewUnmeteredEphemeralReferenceValue( + inter, UnauthorizedAccess, array, &sema.VariableSizedType{ diff --git a/runtime/runtime_test.go b/runtime/runtime_test.go index f0a305e81f..253dbd1826 100644 --- a/runtime/runtime_test.go +++ b/runtime/runtime_test.go @@ -8270,7 +8270,7 @@ func TestRuntimeReturnDestroyedOptional(t *testing.T) { ) RequireError(t, err) - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) } func TestRuntimeComputationMeteringError(t *testing.T) { diff --git a/runtime/storage_test.go b/runtime/storage_test.go index 17af33df06..131d1e395d 100644 --- a/runtime/storage_test.go +++ b/runtime/storage_test.go @@ -5611,19 +5611,14 @@ func TestRuntimeStorageReferenceBoundFunction(t *testing.T) { import Test from 0x42 transaction { - prepare(signer: AuthAccount) { - signer.save(<-Test.createR(), to: /storage/r) - - signer.link<&Test.R>( - /public/r, - target: /storage/r - ) + prepare(signer: auth(Storage) &Account) { + signer.storage.save(<-Test.createR(), to: /storage/r) - let ref = signer.getCapability<&Test.R>(/public/r).borrow()! + let ref = signer.storage.borrow<&Test.R>(from: /storage/r)! var func = ref.foo - let r <- signer.load<@Test.R>(from: /storage/r)! + let r <- signer.storage.load<@Test.R>(from: /storage/r)! // Should be OK func() @@ -5647,5 +5642,5 @@ func TestRuntimeStorageReferenceBoundFunction(t *testing.T) { ) RequireError(t, err) - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) } diff --git a/runtime/tests/interpreter/attachments_test.go b/runtime/tests/interpreter/attachments_test.go index 9426619add..9795b95c7c 100644 --- a/runtime/tests/interpreter/attachments_test.go +++ b/runtime/tests/interpreter/attachments_test.go @@ -1618,7 +1618,7 @@ func TestInterpretAttachmentResourceReferenceInvalidation(t *testing.T) { }) _, err := inter.Invoke("test") - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) }) t.Run("nested", func(t *testing.T) { @@ -1747,7 +1747,7 @@ func TestInterpretAttachmentResourceReferenceInvalidation(t *testing.T) { }) _, err := inter.Invoke("test") - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) }) t.Run("self reference", func(t *testing.T) { diff --git a/runtime/tests/interpreter/container_mutation_test.go b/runtime/tests/interpreter/container_mutation_test.go index 0753d31ab6..2f34f42fae 100644 --- a/runtime/tests/interpreter/container_mutation_test.go +++ b/runtime/tests/interpreter/container_mutation_test.go @@ -1004,7 +1004,7 @@ func TestInterpretContainerMutationWhileIterating(t *testing.T) { interpreter.NewArrayValue( inter, interpreter.EmptyLocationRange, - interpreter.VariableSizedStaticType{ + &interpreter.VariableSizedStaticType{ Type: interpreter.PrimitiveStaticTypeString, }, common.ZeroAddress, @@ -1144,7 +1144,7 @@ func TestInterpretContainerMutationWhileIterating(t *testing.T) { fun test(): @{String: Foo} { let dictionary: @{String: Foo} <- {"a": <- create Foo(), "b": <- create Foo(), "c": <- create Foo()} - var dictionaryRef = &dictionary as &{String: Foo} + var dictionaryRef = &dictionary as auth(Mutate) &{String: Foo} var i = 0 dictionary.forEachKey(fun (key: String): Bool { diff --git a/runtime/tests/interpreter/interpreter_test.go b/runtime/tests/interpreter/interpreter_test.go index 394565ec8f..f189dcd5ad 100644 --- a/runtime/tests/interpreter/interpreter_test.go +++ b/runtime/tests/interpreter/interpreter_test.go @@ -7305,6 +7305,7 @@ func TestInterpretReferenceEventParameter(t *testing.T) { ) ref := interpreter.NewUnmeteredEphemeralReferenceValue( + inter, interpreter.UnauthorizedAccess, arrayValue, inter.MustConvertStaticToSemaType(arrayStaticType), diff --git a/runtime/tests/interpreter/reference_test.go b/runtime/tests/interpreter/reference_test.go index e988d01f58..ff600433b5 100644 --- a/runtime/tests/interpreter/reference_test.go +++ b/runtime/tests/interpreter/reference_test.go @@ -648,6 +648,7 @@ func TestInterpretResourceReferenceInvalidationOnMove(t *testing.T) { ) arrayRef := interpreter.NewUnmeteredEphemeralReferenceValue( + inter, interpreter.NewEntitlementSetAuthorization( nil, func() []common.TypeID { return []common.TypeID{"Mutate"} }, @@ -754,6 +755,7 @@ func TestInterpretResourceReferenceInvalidationOnMove(t *testing.T) { ) arrayRef1 := interpreter.NewUnmeteredEphemeralReferenceValue( + inter, interpreter.NewEntitlementSetAuthorization( nil, func() []common.TypeID { return []common.TypeID{"Mutate"} }, @@ -779,6 +781,7 @@ func TestInterpretResourceReferenceInvalidationOnMove(t *testing.T) { ) arrayRef2 := interpreter.NewUnmeteredEphemeralReferenceValue( + inter, interpreter.NewEntitlementSetAuthorization( nil, func() []common.TypeID { return []common.TypeID{"Mutate"} }, @@ -851,6 +854,7 @@ func TestInterpretResourceReferenceInvalidationOnMove(t *testing.T) { ) arrayRef := interpreter.NewUnmeteredEphemeralReferenceValue( + inter, interpreter.NewEntitlementSetAuthorization( nil, func() []common.TypeID { return []common.TypeID{"Mutate"} }, @@ -976,6 +980,7 @@ func TestInterpretResourceReferenceInvalidationOnMove(t *testing.T) { ) arrayRef := interpreter.NewUnmeteredEphemeralReferenceValue( + inter, interpreter.NewEntitlementSetAuthorization( nil, func() []common.TypeID { return []common.TypeID{"Mutate"} }, @@ -1555,7 +1560,7 @@ func TestInterpretResourceReferenceInvalidationOnDestroy(t *testing.T) { _, err := inter.Invoke("test") RequireError(t, err) - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) }) t.Run("ref source is field", func(t *testing.T) { @@ -1598,7 +1603,7 @@ func TestInterpretResourceReferenceInvalidationOnDestroy(t *testing.T) { _, err := inter.Invoke("test") RequireError(t, err) - require.ErrorAs(t, err, &interpreter.DestroyedResourceError{}) + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) }) } @@ -1722,7 +1727,9 @@ func TestInterpretInvalidatedReferenceToOptional(t *testing.T) { `) _, err := inter.Invoke("main") - require.NoError(t, err) + RequireError(t, err) + + require.ErrorAs(t, err, &interpreter.InvalidatedResourceReferenceError{}) } func TestInterpretReferenceToReference(t *testing.T) { diff --git a/runtime/tests/interpreter/resources_test.go b/runtime/tests/interpreter/resources_test.go index 57b9565ff0..79b89f5efc 100644 --- a/runtime/tests/interpreter/resources_test.go +++ b/runtime/tests/interpreter/resources_test.go @@ -1990,7 +1990,7 @@ func TestInterpreterResourceDoubleWrappedPreCondition(t *testing.T) { } struct interface B { - pub fun deposit(from: @S) { + access(all) fun deposit(from: @S) { pre { from != nil: "" } @@ -1998,7 +1998,7 @@ func TestInterpreterResourceDoubleWrappedPreCondition(t *testing.T) { } struct Vault: A, B { - pub fun deposit(from: @S) { + access(all) fun deposit(from: @S) { pre { from != nil: "" } @@ -2023,7 +2023,7 @@ func TestInterpreterResourceDoubleWrappedPostCondition(t *testing.T) { resource S {} struct interface A { - pub fun deposit(from: @S) { + access(all) fun deposit(from: @S) { post { from != nil: "" } @@ -2693,69 +2693,31 @@ func TestInterpretMovedResourceInSecondValue(t *testing.T) { assert.Equal(t, 53, errorStartPos.Column) } -func TestInterpretOptionalBindingElseBranch(t *testing.T) { - - t.Parallel() - - inter := parseCheckAndInterpret(t, ` - access(all) resource Victim {} - - access(all) fun main() { - - var r: @Victim? <- nil - var r2: @Victim? <- create Victim() - - if let dummy <- r <- r2 { - // unreachable token destroys to please checker - destroy dummy - destroy r - } else { - // checker failed to notice that r2 is invalid here - var ref = &r as &Victim? - var arr: @[Victim?]<- [ - <- r, - <- r2 - ] - destroy arr - } - } - `) - - _, err := inter.Invoke("main") - RequireError(t, err) - - var invalidatedResourceErr interpreter.InvalidatedResourceError - require.ErrorAs(t, err, &invalidatedResourceErr) - - // Error must be thrown at `<-r2` in the array literal - assert.Equal(t, 18, invalidatedResourceErr.StartPosition().Line) -} - func TestInterpretPreConditionResourceMove(t *testing.T) { t.Parallel() inter, err := parseCheckAndInterpretWithOptions(t, ` - pub resource Vault { } - pub resource interface Interface { - pub fun foo(_ r: @AnyResource) { + access(all) resource Vault { } + access(all) resource interface Interface { + access(all) fun foo(_ r: @AnyResource) { pre { consume(&r as &AnyResource, <- r) } } } - pub resource Implementation: Interface { - pub fun foo(_ r: @AnyResource) { + access(all) resource Implementation: Interface { + access(all) fun foo(_ r: @AnyResource) { pre { consume(&r as &AnyResource, <- r) } } } - pub fun consume(_ unusedRef: &AnyResource?, _ r: @AnyResource): Bool { + access(all) fun consume(_ unusedRef: &AnyResource?, _ r: @AnyResource): Bool { destroy r return true } - pub fun main() { + access(all) fun main() { let a <- create Implementation() let b <- create Vault() a.foo(<-b) @@ -2763,8 +2725,10 @@ func TestInterpretPreConditionResourceMove(t *testing.T) { }`, ParseCheckAndInterpretOptions{ HandleCheckerError: func(err error) { - checkerErrors := checker.RequireCheckerErrors(t, err, 1) - require.IsType(t, &sema.InvalidInterfaceConditionResourceInvalidationError{}, checkerErrors[0]) + checkerErrors := checker.RequireCheckerErrors(t, err, 3) + require.IsType(t, &sema.PurityError{}, checkerErrors[0]) + require.IsType(t, &sema.InvalidInterfaceConditionResourceInvalidationError{}, checkerErrors[1]) + require.IsType(t, &sema.PurityError{}, checkerErrors[2]) }, }, )