From 7abf054ce0c4cadfe092d9736e173e18eadfb87d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Thu, 19 Jan 2023 14:41:56 -0800 Subject: [PATCH 01/12] simplify resource invalidation recording --- runtime/sema/checker.go | 94 +++++++++++++++++++-------------------- runtime/sema/resources.go | 2 +- 2 files changed, 46 insertions(+), 50 deletions(-) diff --git a/runtime/sema/checker.go b/runtime/sema/checker.go index 51fef62030..bb2a2b443d 100644 --- a/runtime/sema/checker.go +++ b/runtime/sema/checker.go @@ -1366,32 +1366,13 @@ func (checker *Checker) recordResourceInvalidation( ) } - accessedSelfMember := checker.accessedSelfMember(expression) - - switch expression.(type) { - case *ast.MemberExpression: - - if accessedSelfMember == nil || - !checker.allowSelfResourceFieldInvalidation { - - reportInvalidNestedMove() - return nil + getRecordedResourceInvalidation := func(res Resource) *recordedResourceInvalidation { + invalidation := ResourceInvalidation{ + Kind: invalidationKind, + StartPos: expression.StartPosition(), + EndPos: expression.EndPosition(checker.memoryGauge), } - case *ast.IndexExpression: - reportInvalidNestedMove() - return nil - } - - invalidation := ResourceInvalidation{ - Kind: invalidationKind, - StartPos: expression.StartPosition(), - EndPos: expression.EndPosition(checker.memoryGauge), - } - - if checker.allowSelfResourceFieldInvalidation && accessedSelfMember != nil { - res := Resource{Member: accessedSelfMember} - checker.maybeAddResourceInvalidation(res, invalidation) return &recordedResourceInvalidation{ @@ -1400,37 +1381,52 @@ func (checker *Checker) recordResourceInvalidation( } } - identifierExpression, ok := expression.(*ast.IdentifierExpression) - if !ok { - return nil - } + switch expression := expression.(type) { + case *ast.IdentifierExpression: + variable := checker.findAndCheckValueVariable(expression, false) + if variable == nil { + return nil + } - variable := checker.findAndCheckValueVariable(identifierExpression, false) - if variable == nil { - return nil - } + if invalidationKind != ResourceInvalidationKindMoveTemporary && + variable.DeclarationKind == common.DeclarationKindSelf { + + checker.report( + &InvalidSelfInvalidationError{ + InvalidationKind: invalidationKind, + Range: ast.NewRangeFromPositioned( + checker.memoryGauge, + expression, + ), + }, + ) + } - if invalidationKind != ResourceInvalidationKindMoveTemporary && - variable.DeclarationKind == common.DeclarationKindSelf { + return getRecordedResourceInvalidation(Resource{ + Variable: variable, + }) - checker.report( - &InvalidSelfInvalidationError{ - InvalidationKind: invalidationKind, - Range: ast.NewRangeFromPositioned( - checker.memoryGauge, - expression, - ), - }, - ) - } + case *ast.MemberExpression: - res := Resource{Variable: variable} + accessedSelfMember := checker.accessedSelfMember(expression) - checker.maybeAddResourceInvalidation(res, invalidation) + if accessedSelfMember == nil || + !checker.allowSelfResourceFieldInvalidation { + + reportInvalidNestedMove() + return nil + } + + return getRecordedResourceInvalidation(Resource{ + Member: accessedSelfMember, + }) - return &recordedResourceInvalidation{ - resource: res, - invalidation: invalidation, + case *ast.IndexExpression: + reportInvalidNestedMove() + return nil + + default: + return nil } } diff --git a/runtime/sema/resources.go b/runtime/sema/resources.go index dead221a2c..f1c1032985 100644 --- a/runtime/sema/resources.go +++ b/runtime/sema/resources.go @@ -57,7 +57,7 @@ import ( */ -// A Resource is a variable or a member +// A Resource is a variable OR a member type Resource struct { Variable *Variable Member *Member From 48247ab22059162d0d97db8c0ca4ce715c2e1a90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Thu, 19 Jan 2023 15:29:48 -0800 Subject: [PATCH 02/12] refactor resource field invalidation check to function --- runtime/sema/check_composite_declaration.go | 27 +++++----- runtime/sema/check_transaction_declaration.go | 11 +++-- runtime/sema/checker.go | 49 ++++++++++--------- 3 files changed, 49 insertions(+), 38 deletions(-) diff --git a/runtime/sema/check_composite_declaration.go b/runtime/sema/check_composite_declaration.go index e612aee0a3..07df5470b9 100644 --- a/runtime/sema/check_composite_declaration.go +++ b/runtime/sema/check_composite_declaration.go @@ -171,17 +171,22 @@ func (checker *Checker) visitCompositeDeclaration(declaration *ast.CompositeDecl // NOTE: check destructors after initializer and functions - checker.withSelfResourceInvalidationAllowed(func() { - checker.checkDestructors( - declaration.Members.Destructors(), - declaration.Members.FieldsByIdentifier(), - compositeType.Members, - compositeType, - declaration.DeclarationKind(), - declaration.DeclarationDocString(), - kind, - ) - }) + checker.withResourceFieldInvalidationAllowed( + func(expression *ast.MemberExpression) *Member { + return checker.accessedSelfMember(expression) + }, + func() { + checker.checkDestructors( + declaration.Members.Destructors(), + declaration.Members.FieldsByIdentifier(), + compositeType.Members, + compositeType, + declaration.DeclarationKind(), + declaration.DeclarationDocString(), + kind, + ) + }, + ) // NOTE: visit interfaces first // DON'T use `nestedDeclarations`, because of non-deterministic order diff --git a/runtime/sema/check_transaction_declaration.go b/runtime/sema/check_transaction_declaration.go index 479c4f4e7b..7dacb6d541 100644 --- a/runtime/sema/check_transaction_declaration.go +++ b/runtime/sema/check_transaction_declaration.go @@ -83,9 +83,14 @@ func (checker *Checker) VisitTransactionDeclaration(declaration *ast.Transaction declaration.PostConditions, VoidType, func() { - checker.withSelfResourceInvalidationAllowed(func() { - checker.visitTransactionExecuteFunction(declaration.Execute, transactionType) - }) + checker.withResourceFieldInvalidationAllowed( + func(expression *ast.MemberExpression) *Member { + return checker.accessedSelfMember(expression) + }, + func() { + checker.visitTransactionExecuteFunction(declaration.Execute, transactionType) + }, + ) }, ) diff --git a/runtime/sema/checker.go b/runtime/sema/checker.go index bb2a2b443d..85f706d7c2 100644 --- a/runtime/sema/checker.go +++ b/runtime/sema/checker.go @@ -104,15 +104,15 @@ type Checker struct { Config *Config Elaboration *Elaboration // initialized lazily. use beforeExtractor() - _beforeExtractor *BeforeExtractor - errors []error - functionActivations *FunctionActivations - inCondition bool - allowSelfResourceFieldInvalidation bool - inAssignment bool - inInvocation bool - inCreate bool - isChecked bool + _beforeExtractor *BeforeExtractor + errors []error + functionActivations *FunctionActivations + inCondition bool + resourceFieldInvalidationAllowed func(*ast.MemberExpression) *Member + inAssignment bool + inInvocation bool + inCreate bool + isChecked bool } var _ ast.DeclarationVisitor[struct{}] = &Checker{} @@ -1407,19 +1407,17 @@ func (checker *Checker) recordResourceInvalidation( }) case *ast.MemberExpression: - - accessedSelfMember := checker.accessedSelfMember(expression) - - if accessedSelfMember == nil || - !checker.allowSelfResourceFieldInvalidation { - - reportInvalidNestedMove() - return nil + if checker.resourceFieldInvalidationAllowed != nil { + member := checker.resourceFieldInvalidationAllowed(expression) + if member != nil { + return getRecordedResourceInvalidation(Resource{ + Member: member, + }) + } } - return getRecordedResourceInvalidation(Resource{ - Member: accessedSelfMember, - }) + reportInvalidNestedMove() + return nil case *ast.IndexExpression: reportInvalidNestedMove() @@ -1794,11 +1792,14 @@ func (checker *Checker) isWriteableAccess(access ast.Access) bool { } } -func (checker *Checker) withSelfResourceInvalidationAllowed(f func()) { - allowSelfResourceFieldInvalidation := checker.allowSelfResourceFieldInvalidation - checker.allowSelfResourceFieldInvalidation = true +func (checker *Checker) withResourceFieldInvalidationAllowed( + invalidationAllowed func(*ast.MemberExpression) *Member, + f func(), +) { + oldInvalidationAllowed := checker.resourceFieldInvalidationAllowed + checker.resourceFieldInvalidationAllowed = invalidationAllowed defer func() { - checker.allowSelfResourceFieldInvalidation = allowSelfResourceFieldInvalidation + checker.resourceFieldInvalidationAllowed = oldInvalidationAllowed }() f() From 4cea08d90e200bfada3b4cd2369b45c5c78375c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Thu, 19 Jan 2023 15:39:26 -0800 Subject: [PATCH 03/12] add support for resource-kinded fields in transaction roles: allow/require invalidation of roles' members --- runtime/sema/check_transaction_declaration.go | 14 ++++- runtime/tests/checker/transactions_test.go | 53 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/runtime/sema/check_transaction_declaration.go b/runtime/sema/check_transaction_declaration.go index 7dacb6d541..f21efad4eb 100644 --- a/runtime/sema/check_transaction_declaration.go +++ b/runtime/sema/check_transaction_declaration.go @@ -85,6 +85,14 @@ func (checker *Checker) VisitTransactionDeclaration(declaration *ast.Transaction func() { checker.withResourceFieldInvalidationAllowed( func(expression *ast.MemberExpression) *Member { + // In addition to the behaviour in composite destructors, + // where a self member may be invalidated, + // also allow invalidation of role members + memberInfo, _ := checker.Elaboration.MemberExpressionMemberInfo(expression) + if _, ok := memberInfo.AccessedType.(*TransactionRoleType); ok { + return memberInfo.Member + } + return checker.accessedSelfMember(expression) }, func() { @@ -94,7 +102,11 @@ func (checker *Checker) VisitTransactionDeclaration(declaration *ast.Transaction }, ) - checker.checkResourceFieldsInvalidated(transactionType, transactionType.Members) + checker.checkResourceFieldsInvalidated(transactionType, members) + + transactionType.Roles.Foreach(func(_ string, roleTye *TransactionRoleType) { + checker.checkResourceFieldsInvalidated(roleTye, roleTye.Members) + }) return } diff --git a/runtime/tests/checker/transactions_test.go b/runtime/tests/checker/transactions_test.go index f3d2c9a78d..193d93b0bc 100644 --- a/runtime/tests/checker/transactions_test.go +++ b/runtime/tests/checker/transactions_test.go @@ -661,6 +661,59 @@ func TestCheckTransactionRoles(t *testing.T) { nil, ) }) + + t.Run("resource field", func(t *testing.T) { + test(t, + ` + resource R {} + + fun absorb(_ r: @R) { + destroy r + } + + transaction { + + role buyer { + var x: @R + + prepare() { + self.x <- create R() + } + } + + execute { + absorb(<-self.buyer.x) + } + } + `, + nil, + ) + }) + + t.Run("invalid resource field loss", func(t *testing.T) { + test(t, + ` + resource R {} + + transaction { + + role buyer { + var x: @R + + prepare() { + self.x <- create R() + } + } + + execute {} + } + `, + []error{ + &sema.ResourceFieldNotInvalidatedError{}, + }, + ) + }) + } func TestCheckTransactionExecuteScope(t *testing.T) { From 2d73b129b7cd978b83edf7279c1b99757a2f7b5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Fri, 20 Jan 2023 15:06:10 -0800 Subject: [PATCH 04/12] improve identifier naming --- runtime/tests/checker/transactions_test.go | 42 +++++++++++----------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/runtime/tests/checker/transactions_test.go b/runtime/tests/checker/transactions_test.go index 193d93b0bc..d4c866888d 100644 --- a/runtime/tests/checker/transactions_test.go +++ b/runtime/tests/checker/transactions_test.go @@ -261,14 +261,14 @@ func TestCheckTransactions(t *testing.T) { transaction { - var x: @R + var r: @R prepare() { - self.x <- create R() + self.r <- create R() } execute { - destroy self.x + destroy self.r } } `, @@ -283,10 +283,10 @@ func TestCheckTransactions(t *testing.T) { transaction { - var x: @R + var r: @R prepare() { - self.x <- create R() + self.r <- create R() } execute {} @@ -555,7 +555,7 @@ func TestCheckTransactionRoles(t *testing.T) { prepare(signer: AuthAccount) {} - role buyer { + role role1 { let foo: Int prepare(foo: Int) { @@ -579,7 +579,7 @@ func TestCheckTransactionRoles(t *testing.T) { prepare(signer: AuthAccount) {} - role buyer { + role role1 { prepare(signer: AuthAccount) {} } } @@ -596,7 +596,7 @@ func TestCheckTransactionRoles(t *testing.T) { prepare(signer: AuthAccount) {} - role buyer {} + role role1 {} } `, []error{ @@ -613,7 +613,7 @@ func TestCheckTransactionRoles(t *testing.T) { prepare(signer: AuthAccount) {} - role buyer { + role role1 { prepare() {} } } @@ -632,7 +632,7 @@ func TestCheckTransactionRoles(t *testing.T) { prepare(signer: AuthAccount) {} - role buyer { + role role1 { prepare(firstSigner: AuthAccount, secondSigner: AuthAccount) {} } } @@ -649,7 +649,7 @@ func TestCheckTransactionRoles(t *testing.T) { ` transaction(foo: Int) { - role buyer { + role role1 { let foo: Int prepare() { @@ -673,16 +673,16 @@ func TestCheckTransactionRoles(t *testing.T) { transaction { - role buyer { - var x: @R + role role1 { + var r: @R prepare() { - self.x <- create R() + self.r <- create R() } } execute { - absorb(<-self.buyer.x) + absorb(<-self.role1.r) } } `, @@ -697,11 +697,11 @@ func TestCheckTransactionRoles(t *testing.T) { transaction { - role buyer { - var x: @R + role role1 { + var r: @R prepare() { - self.x <- create R() + self.r <- create R() } } @@ -857,7 +857,7 @@ func TestCheckInvalidTransactionRoleSelfMove(t *testing.T) { transaction { - role buyer { + role role1 { prepare() { let x = self } @@ -878,7 +878,7 @@ func TestCheckInvalidTransactionRoleSelfMove(t *testing.T) { transaction { - role buyer { + role role1 { prepare() { let txs = [self] } @@ -899,7 +899,7 @@ func TestCheckInvalidTransactionRoleSelfMove(t *testing.T) { transaction { - role buyer { + role role1 { prepare() { let txs = {"self": self} } From c566da8e32be922e9712c8ca47ac15e67fa7fe67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Fri, 20 Jan 2023 15:07:28 -0800 Subject: [PATCH 05/12] check role resource field after invalidation --- runtime/sema/check_member_expression.go | 34 ++++++++++++---------- runtime/sema/checker.go | 5 ++-- runtime/tests/checker/transactions_test.go | 31 ++++++++++++++++++++ 3 files changed, 52 insertions(+), 18 deletions(-) diff --git a/runtime/sema/check_member_expression.go b/runtime/sema/check_member_expression.go index f58379c07f..d92f3dbdab 100644 --- a/runtime/sema/check_member_expression.go +++ b/runtime/sema/check_member_expression.go @@ -110,6 +110,24 @@ func (checker *Checker) visitMember(expression *ast.MemberExpression) (accessedT IsOptional: isOptional, }, ) + + // If the access is to a member which may be invalidated, + // its use must be recorded/checked, so that it isn't used after it was invalidated + + resourceFieldInvalidationAllowed := checker.resourceFieldInvalidationAllowed + if resourceFieldInvalidationAllowed != nil { + member := resourceFieldInvalidationAllowed(expression) + if member != nil && member.TypeAnnotation.Type.IsResourceType() { + + // NOTE: Preventing the capturing of the resource field is already implicitly handled: + // By definition, the resource field can only be nested in a resource, + // so `self` is a resource, and the capture of it is checked separately + + res := Resource{Member: member} + + checker.checkResourceUseAfterInvalidation(res, expression.Identifier) + } + } }() accessedExpression := expression.Expression @@ -133,22 +151,6 @@ func (checker *Checker) visitMember(expression *ast.MemberExpression) (accessedT return accessedType, member, isOptional } - // If the access is to a member of `self` and a resource, - // its use must be recorded/checked, so that it isn't used after it was invalidated - - accessedSelfMember := checker.accessedSelfMember(expression) - if accessedSelfMember != nil && - accessedSelfMember.TypeAnnotation.Type.IsResourceType() { - - // NOTE: Preventing the capturing of the resource field is already implicitly handled: - // By definition, the resource field can only be nested in a resource, - // so `self` is a resource, and the capture of it is checked separately - - res := Resource{Member: accessedSelfMember} - - checker.checkResourceUseAfterInvalidation(res, expression.Identifier) - } - identifier := expression.Identifier.Identifier identifierStartPosition := expression.Identifier.StartPosition() identifierEndPosition := expression.Identifier.EndPosition(checker.memoryGauge) diff --git a/runtime/sema/checker.go b/runtime/sema/checker.go index 85f706d7c2..28f7fe05a1 100644 --- a/runtime/sema/checker.go +++ b/runtime/sema/checker.go @@ -1407,8 +1407,9 @@ func (checker *Checker) recordResourceInvalidation( }) case *ast.MemberExpression: - if checker.resourceFieldInvalidationAllowed != nil { - member := checker.resourceFieldInvalidationAllowed(expression) + resourceFieldInvalidationAllowed := checker.resourceFieldInvalidationAllowed + if resourceFieldInvalidationAllowed != nil { + member := resourceFieldInvalidationAllowed(expression) if member != nil { return getRecordedResourceInvalidation(Resource{ Member: member, diff --git a/runtime/tests/checker/transactions_test.go b/runtime/tests/checker/transactions_test.go index d4c866888d..177e443cb5 100644 --- a/runtime/tests/checker/transactions_test.go +++ b/runtime/tests/checker/transactions_test.go @@ -714,6 +714,37 @@ func TestCheckTransactionRoles(t *testing.T) { ) }) + t.Run("resource field use after invalidation", func(t *testing.T) { + test(t, + ` + resource R {} + + fun absorb(_ r: @R) { + destroy r + } + + transaction { + + role role1 { + var r: @R + + prepare() { + self.r <- create R() + } + } + + execute { + absorb(<-self.role1.r) + absorb(<-self.role1.r) + } + } + `, + []error{ + &sema.ResourceUseAfterInvalidationError{}, + }, + ) + }) + } func TestCheckTransactionExecuteScope(t *testing.T) { From 8a955179e8972d5df05de0cdd4023808e1bde5ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 23 Jan 2023 17:05:25 -0800 Subject: [PATCH 06/12] improve field alignment --- runtime/ast/transaction_declaration.go | 2 +- runtime/interpreter/errors.go | 4 ++-- runtime/sema/checker.go | 28 +++++++++++++------------- runtime/sema/type.go | 2 +- types.go | 4 ++-- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/runtime/ast/transaction_declaration.go b/runtime/ast/transaction_declaration.go index 4362b9e0da..9d5c7e43e9 100644 --- a/runtime/ast/transaction_declaration.go +++ b/runtime/ast/transaction_declaration.go @@ -198,10 +198,10 @@ func (d *TransactionDeclaration) String() string { // TransactionRoleDeclaration type TransactionRoleDeclaration struct { - Identifier Identifier Prepare *SpecialFunctionDeclaration DocString string Fields []*FieldDeclaration + Identifier Identifier Range } diff --git a/runtime/interpreter/errors.go b/runtime/interpreter/errors.go index 06db6fba3d..75f693ae40 100644 --- a/runtime/interpreter/errors.go +++ b/runtime/interpreter/errors.go @@ -239,10 +239,10 @@ func (e RedeclarationError) Error() string { // DereferenceError type DereferenceError struct { - Cause string + LocationRange ExpectedType sema.Type ActualType sema.Type - LocationRange + Cause string } var _ errors.UserError = DereferenceError{} diff --git a/runtime/sema/checker.go b/runtime/sema/checker.go index 28f7fe05a1..8aed019865 100644 --- a/runtime/sema/checker.go +++ b/runtime/sema/checker.go @@ -91,24 +91,24 @@ type ContractValueHandlerFunc func( type Checker struct { // memoryGauge is used for metering memory usage - memoryGauge common.MemoryGauge - Location common.Location - expectedType Type - resources *Resources - valueActivations *VariableActivations - currentMemberExpression *ast.MemberExpression - typeActivations *VariableActivations - containerTypes map[Type]struct{} - Program *ast.Program - PositionInfo *PositionInfo - Config *Config - Elaboration *Elaboration + memoryGauge common.MemoryGauge + Location common.Location + expectedType Type + Config *Config // initialized lazily. use beforeExtractor() _beforeExtractor *BeforeExtractor - errors []error + currentMemberExpression *ast.MemberExpression + typeActivations *VariableActivations + containerTypes map[Type]struct{} + Program *ast.Program + PositionInfo *PositionInfo + resources *Resources + Elaboration *Elaboration + valueActivations *VariableActivations + resourceFieldInvalidationAllowed func(*ast.MemberExpression) *Member functionActivations *FunctionActivations + errors []error inCondition bool - resourceFieldInvalidationAllowed func(*ast.MemberExpression) *Member inAssignment bool inInvocation bool inCreate bool diff --git a/runtime/sema/type.go b/runtime/sema/type.go index f67b7ad3fa..dee2c43c46 100644 --- a/runtime/sema/type.go +++ b/runtime/sema/type.go @@ -5649,10 +5649,10 @@ func IsNilType(ty Type) bool { type TransactionType struct { Members *StringMemberOrderedMap + Roles *orderedmap.OrderedMap[string, *TransactionRoleType] Fields []string PrepareParameters []Parameter Parameters []Parameter - Roles *orderedmap.OrderedMap[string, *TransactionRoleType] } var _ Type = &TransactionType{} diff --git a/types.go b/types.go index b218eee109..397828bdc9 100644 --- a/types.go +++ b/types.go @@ -1769,10 +1769,10 @@ func (t ReferenceType) Equal(other Type) bool { type restrictionSet = map[Type]struct{} type RestrictedType struct { - typeID string Type Type - Restrictions []Type restrictionSet restrictionSet + typeID string + Restrictions []Type restrictionSetOnce sync.Once } From 1a9038051d6850751994804c164c5440c5d58386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Fri, 27 Jan 2023 11:59:59 -0800 Subject: [PATCH 07/12] improve message of invalid nested resource move error: include identifier of member expression --- runtime/sema/checker.go | 7 ++++--- runtime/sema/errors.go | 7 ++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/runtime/sema/checker.go b/runtime/sema/checker.go index 8aed019865..6867ec0251 100644 --- a/runtime/sema/checker.go +++ b/runtime/sema/checker.go @@ -1355,9 +1355,10 @@ func (checker *Checker) recordResourceInvalidation( return nil } - reportInvalidNestedMove := func() { + reportInvalidNestedMove := func(identifier *ast.Identifier) { checker.report( &InvalidNestedResourceMoveError{ + Identifier: identifier, Range: ast.NewRangeFromPositioned( checker.memoryGauge, expression, @@ -1417,11 +1418,11 @@ func (checker *Checker) recordResourceInvalidation( } } - reportInvalidNestedMove() + reportInvalidNestedMove(&expression.Identifier) return nil case *ast.IndexExpression: - reportInvalidNestedMove() + reportInvalidNestedMove(nil) return nil default: diff --git a/runtime/sema/errors.go b/runtime/sema/errors.go index 0b5bbb7c9a..2ff374e6f0 100644 --- a/runtime/sema/errors.go +++ b/runtime/sema/errors.go @@ -1860,6 +1860,7 @@ func (e *MissingResourceAnnotationError) Error() string { // InvalidNestedResourceMoveError type InvalidNestedResourceMoveError struct { + Identifier *ast.Identifier ast.Range } @@ -1871,6 +1872,11 @@ func (*InvalidNestedResourceMoveError) isSemanticError() {} func (*InvalidNestedResourceMoveError) IsUserError() {} func (e *InvalidNestedResourceMoveError) Error() string { + identifier := e.Identifier + if identifier != nil { + return fmt.Sprintf("cannot move nested resource-kinded field `%s`", identifier.Identifier) + } + return "cannot move nested resource" } @@ -2903,7 +2909,6 @@ func (e *ReadOnlyTargetAssignmentError) Error() string { } // MissingPrepareForFieldError -// type MissingPrepareForFieldError struct { FirstFieldName string FirstFieldPos ast.Position From 7fb3e0a5c855dd14facd19b690989ec916abdbc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Fri, 27 Jan 2023 12:02:01 -0800 Subject: [PATCH 08/12] refactor tests --- runtime/tests/checker/transactions_test.go | 465 ++++++++++----------- 1 file changed, 227 insertions(+), 238 deletions(-) diff --git a/runtime/tests/checker/transactions_test.go b/runtime/tests/checker/transactions_test.go index 177e443cb5..2ce9e853d8 100644 --- a/runtime/tests/checker/transactions_test.go +++ b/runtime/tests/checker/transactions_test.go @@ -390,361 +390,350 @@ func TestCheckTransactionRoles(t *testing.T) { t.Parallel() - test := func(t *testing.T, code string, expectedErrors []error) { - t.Parallel() - - _, err := ParseAndCheck(t, code) + t.Run("empty", func(t *testing.T) { - errs := RequireCheckerErrors(t, err, len(expectedErrors)) + t.Parallel() - for i, err := range errs { - if !assert.IsType(t, expectedErrors[i], err) { - t.Log(err) - } - } - } + _, err := ParseAndCheck(t, ` + transaction { - t.Run("empty", func(t *testing.T) { - test( - t, - ` - transaction { + role foo {} + } + `) - role foo {} - } - `, - nil, - ) + require.NoError(t, err) }) t.Run("field, prepare", func(t *testing.T) { - test( - t, - ` - transaction { - role foo { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction { - let bar: Int + role foo { - prepare() { - self.bar = 1 - } + let bar: Int + + prepare() { + self.bar = 1 } } - `, - nil, - ) + } + `) + require.NoError(t, err) }) t.Run("field, missing prepare", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - role foo { + _, err := ParseAndCheck(t, ` + transaction { - let bar: Int - } + role foo { + + let bar: Int } - `, - []error{ - &sema.MissingPrepareForFieldError{}, - }, - ) + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.MissingPrepareForFieldError{}, errs[0]) }) t.Run("field, prepare, missing initialization", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction { - role foo { + role foo { - let bar: Int + let bar: Int - prepare() {} - } + prepare() {} } - `, - []error{ - &sema.FieldUninitializedError{}, - }, - ) + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.FieldUninitializedError{}, errs[0]) }) t.Run("duplicate", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - role foo {} + _, err := ParseAndCheck(t, ` + transaction { - role foo {} - } - `, - []error{ - &sema.DuplicateTransactionRoleError{}, - }, - ) + role foo {} + + role foo {} + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.DuplicateTransactionRoleError{}, errs[0]) }) t.Run("field name conflict", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - let foo: Int + _, err := ParseAndCheck(t, ` + transaction { - prepare() { - self.foo = 1 - } + let foo: Int - role foo {} + prepare() { + self.foo = 1 } - `, - []error{ - &sema.TransactionRoleWithFieldNameError{}, - }, - ) + + role foo {} + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.TransactionRoleWithFieldNameError{}, errs[0]) }) t.Run("multiple roles, one field each, execute", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction { - role foo { + role foo { - let bar: Int + let bar: Int - prepare() { - self.bar = 1 - } + prepare() { + self.bar = 1 } + } - role baz { + role baz { - let blub: String + let blub: String - prepare() { - self.blub = "2" - } + prepare() { + self.blub = "2" } + } - execute { - let bar: Int = self.foo.bar - let blub: String = self.baz.blub - } + execute { + let bar: Int = self.foo.bar + let blub: String = self.baz.blub } - `, - nil, - ) + } + `) + + require.NoError(t, err) }) t.Run("invalid prepare parameter type", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - prepare(signer: AuthAccount) {} + _, err := ParseAndCheck(t, ` + transaction { + + prepare(signer: AuthAccount) {} - role role1 { - let foo: Int + role role1 { + let foo: Int - prepare(foo: Int) { - self.foo = foo - } + prepare(foo: Int) { + self.foo = foo } } - `, - []error{ - &sema.InvalidTransactionPrepareParameterTypeError{}, - &sema.TypeMismatchError{}, - }, - ) + } + `) + + errs := RequireCheckerErrors(t, err, 2) + + require.IsType(t, &sema.InvalidTransactionPrepareParameterTypeError{}, errs[0]) + require.IsType(t, &sema.TypeMismatchError{}, errs[1]) }) t.Run("matching prepare", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - prepare(signer: AuthAccount) {} + _, err := ParseAndCheck(t, ` + transaction { - role role1 { - prepare(signer: AuthAccount) {} - } + prepare(signer: AuthAccount) {} + + role role1 { + prepare(signer: AuthAccount) {} } - `, - nil, - ) + } + `) + + require.NoError(t, err) }) t.Run("missing prepare", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - prepare(signer: AuthAccount) {} + _, err := ParseAndCheck(t, ` + transaction { - role role1 {} - } - `, - []error{ - &sema.MissingRolePrepareError{}, - }, - ) + prepare(signer: AuthAccount) {} + + role role1 {} + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.MissingRolePrepareError{}, errs[0]) }) t.Run("fewer prepare parameters", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - prepare(signer: AuthAccount) {} + _, err := ParseAndCheck(t, ` + transaction { - role role1 { - prepare() {} - } + prepare(signer: AuthAccount) {} + + role role1 { + prepare() {} } - `, - []error{ - &sema.PrepareParameterCountMismatchError{}, - }, - ) + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.PrepareParameterCountMismatchError{}, errs[0]) }) t.Run("more prepare parameters", func(t *testing.T) { - test( - t, - ` - transaction { + t.Parallel() - prepare(signer: AuthAccount) {} + _, err := ParseAndCheck(t, ` + transaction { - role role1 { - prepare(firstSigner: AuthAccount, secondSigner: AuthAccount) {} - } + prepare(signer: AuthAccount) {} + + role role1 { + prepare(firstSigner: AuthAccount, secondSigner: AuthAccount) {} } - `, - []error{ - &sema.PrepareParameterCountMismatchError{}, - }, - ) + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.PrepareParameterCountMismatchError{}, errs[0]) }) t.Run("transaction parameter usage", func(t *testing.T) { - test( - t, - ` - transaction(foo: Int) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction(foo: Int) { - role role1 { - let foo: Int + role role1 { + let foo: Int - prepare() { - self.foo = foo - } + prepare() { + self.foo = foo } } - `, - nil, - ) + } + `) + + require.NoError(t, err) }) t.Run("resource field", func(t *testing.T) { - test(t, - ` - resource R {} + t.Parallel() - fun absorb(_ r: @R) { - destroy r - } + _, err := ParseAndCheck(t, ` + resource R {} - transaction { + fun absorb(_ r: @R) { + destroy r + } - role role1 { - var r: @R + transaction { - prepare() { - self.r <- create R() - } - } + role role1 { + var r: @R - execute { - absorb(<-self.role1.r) + prepare() { + self.r <- create R() } } - `, - nil, - ) + + execute { + absorb(<-self.role1.r) + } + } + `) + + require.NoError(t, err) }) t.Run("invalid resource field loss", func(t *testing.T) { - test(t, - ` - resource R {} + t.Parallel() - transaction { + _, err := ParseAndCheck(t, ` + resource R {} - role role1 { - var r: @R + transaction { - prepare() { - self.r <- create R() - } - } + role role1 { + var r: @R - execute {} + prepare() { + self.r <- create R() + } } - `, - []error{ - &sema.ResourceFieldNotInvalidatedError{}, - }, - ) + + execute {} + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.ResourceFieldNotInvalidatedError{}, errs[0]) }) t.Run("resource field use after invalidation", func(t *testing.T) { - test(t, - ` - resource R {} + t.Parallel() - fun absorb(_ r: @R) { - destroy r - } + _, err := ParseAndCheck(t, ` + resource R {} - transaction { + fun absorb(_ r: @R) { + destroy r + } - role role1 { - var r: @R + transaction { - prepare() { - self.r <- create R() - } - } + role role1 { + var r: @R - execute { - absorb(<-self.role1.r) - absorb(<-self.role1.r) + prepare() { + self.r <- create R() } } - `, - []error{ - &sema.ResourceUseAfterInvalidationError{}, - }, - ) - }) + execute { + absorb(<-self.role1.r) + absorb(<-self.role1.r) + } + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + require.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0]) + }) } func TestCheckTransactionExecuteScope(t *testing.T) { From a91d4063d5eb9282966da44d6a7c737de94ac426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Fri, 27 Jan 2023 12:05:34 -0800 Subject: [PATCH 09/12] test nested resource invalidation for transaction role resource-kinded field --- runtime/tests/checker/transactions_test.go | 87 ++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/runtime/tests/checker/transactions_test.go b/runtime/tests/checker/transactions_test.go index 2ce9e853d8..b687ffc1d8 100644 --- a/runtime/tests/checker/transactions_test.go +++ b/runtime/tests/checker/transactions_test.go @@ -734,6 +734,93 @@ func TestCheckTransactionRoles(t *testing.T) { require.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0]) }) + + t.Run("resource field with nested resource", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + resource R1 { + var r2: @R2 + + init() { + self.r2 <- create R2() + } + + destroy() { + destroy self.r2 + } + } + + resource R2 {} + + fun absorb(_ r: @AnyResource) { + destroy r + } + + transaction { + + role role1 { + var r1: @R1 + + prepare() { + self.r1 <- create R1() + } + } + + execute { + absorb(<-self.role1.r1) + } + } + `) + require.NoError(t, err) + }) + + t.Run("nested resource field", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + resource R1 { + var r2: @R2 + + init() { + self.r2 <- create R2() + } + + destroy() { + destroy self.r2 + } + } + + resource R2 {} + + fun absorb(_ r: @AnyResource) { + destroy r + } + + transaction { + + role role1 { + var r1: @R1 + + prepare() { + self.r1 <- create R1() + } + } + + execute { + absorb(<-self.role1.r1.r2) + } + } + `) + + errs := RequireCheckerErrors(t, err, 2) + + var invalidNestedResourceErr *sema.InvalidNestedResourceMoveError + require.ErrorAs(t, errs[0], &invalidNestedResourceErr) + assert.ErrorContains(t, invalidNestedResourceErr, "field `r2`") + + require.IsType(t, &sema.ResourceFieldNotInvalidatedError{}, errs[1]) + }) } func TestCheckTransactionExecuteScope(t *testing.T) { From 9fa982e8926b63ea923f1927eb3a4ecca99b2dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Fri, 27 Jan 2023 12:05:57 -0800 Subject: [PATCH 10/12] move checker config setup out of checking function --- runtime/tests/checker/utils.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/runtime/tests/checker/utils.go b/runtime/tests/checker/utils.go index 36e7c13a1d..66d51adfc5 100644 --- a/runtime/tests/checker/utils.go +++ b/runtime/tests/checker/utils.go @@ -88,17 +88,17 @@ func ParseAndCheckWithOptionsAndMemoryMetering( return nil, err } - check := func() (*sema.Checker, error) { + config := options.Config + if config == nil { + config = &sema.Config{} + } - config := options.Config - if config == nil { - config = &sema.Config{} - } + if config.AccessCheckMode == sema.AccessCheckModeDefault { + config.AccessCheckMode = sema.AccessCheckModeNotSpecifiedUnrestricted + } + config.ExtendedElaborationEnabled = true - if config.AccessCheckMode == sema.AccessCheckModeDefault { - config.AccessCheckMode = sema.AccessCheckModeNotSpecifiedUnrestricted - } - config.ExtendedElaborationEnabled = true + check := func() (*sema.Checker, error) { checker, err := sema.NewChecker( program, From 27de6f27975dccd54f4ffa2c9fa8f20ac46426ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 30 Jan 2023 14:55:34 -0800 Subject: [PATCH 11/12] fix merge --- runtime/sema/type.go | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/sema/type.go b/runtime/sema/type.go index df0a2867bb..dee2c43c46 100644 --- a/runtime/sema/type.go +++ b/runtime/sema/type.go @@ -28,6 +28,7 @@ import ( "github.com/onflow/cadence/fixedpoint" "github.com/onflow/cadence/runtime/ast" "github.com/onflow/cadence/runtime/common" + "github.com/onflow/cadence/runtime/common/orderedmap" "github.com/onflow/cadence/runtime/errors" ) From 7d32929a5d4b364729fe7d5c8fc751e67c0775da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 30 Jan 2023 15:04:53 -0800 Subject: [PATCH 12/12] roles are not stored in transaction type anymore, iterate over members --- runtime/sema/check_transaction_declaration.go | 11 +++++++++-- runtime/sema/type.go | 2 -- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/runtime/sema/check_transaction_declaration.go b/runtime/sema/check_transaction_declaration.go index 370a76d3c6..94ee72296d 100644 --- a/runtime/sema/check_transaction_declaration.go +++ b/runtime/sema/check_transaction_declaration.go @@ -103,8 +103,15 @@ func (checker *Checker) VisitTransactionDeclaration(declaration *ast.Transaction checker.checkResourceFieldsInvalidated(transactionType, members) - transactionType.Roles.Foreach(func(_ string, roleTye *TransactionRoleType) { - checker.checkResourceFieldsInvalidated(roleTye, roleTye.Members) + transactionType.Members.Foreach(func(_ string, member *Member) { + transactionRoleType, ok := member.TypeAnnotation.Type.(*TransactionRoleType) + if !ok { + return + } + checker.checkResourceFieldsInvalidated( + transactionRoleType, + transactionRoleType.Members, + ) }) return diff --git a/runtime/sema/type.go b/runtime/sema/type.go index dee2c43c46..b75951576a 100644 --- a/runtime/sema/type.go +++ b/runtime/sema/type.go @@ -28,7 +28,6 @@ import ( "github.com/onflow/cadence/fixedpoint" "github.com/onflow/cadence/runtime/ast" "github.com/onflow/cadence/runtime/common" - "github.com/onflow/cadence/runtime/common/orderedmap" "github.com/onflow/cadence/runtime/errors" ) @@ -5649,7 +5648,6 @@ func IsNilType(ty Type) bool { type TransactionType struct { Members *StringMemberOrderedMap - Roles *orderedmap.OrderedMap[string, *TransactionRoleType] Fields []string PrepareParameters []Parameter Parameters []Parameter