From 41f0300b7b32fdc08d65c599f7ac4225c09268a8 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Thu, 18 Apr 2024 14:08:25 -0700 Subject: [PATCH 1/4] Fix resource loss check in dictionary set-key From bb10009c18ab32968c2b0ae54c2ba70b2d250390 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 30 Apr 2024 10:02:48 -0400 Subject: [PATCH 2/4] port fixes to v0.42 --- runtime/sema/check_assignment.go | 86 ++++++++---- runtime/tests/checker/access_test.go | 14 +- runtime/tests/checker/resources_test.go | 144 +++++++++++++++++++++ runtime/tests/checker/transactions_test.go | 31 +++++ 4 files changed, 241 insertions(+), 34 deletions(-) diff --git a/runtime/sema/check_assignment.go b/runtime/sema/check_assignment.go index dedef05e54..1854667788 100644 --- a/runtime/sema/check_assignment.go +++ b/runtime/sema/check_assignment.go @@ -135,6 +135,35 @@ func (checker *Checker) checkAssignment( return } +func (checker *Checker) rootOfAccessChain(target ast.Expression) (baseVariable *Variable, accessChain []Type) { + accessChain = make([]Type, 0) + var inAccessChain = true + + // seek the variable expression (if it exists) at the base of the access chain + for inAccessChain { + switch targetExp := target.(type) { + case *ast.IdentifierExpression: + baseVariable = checker.valueActivations.Find(targetExp.Identifier.Identifier) + if baseVariable != nil { + accessChain = append(accessChain, baseVariable.Type) + } + inAccessChain = false + case *ast.IndexExpression: + target = targetExp.TargetExpression + elementType := checker.Elaboration.IndexExpressionTypes(targetExp).IndexedType.ElementType(true) + accessChain = append(accessChain, elementType) + case *ast.MemberExpression: + target = targetExp.Expression + memberType, _, _, _ := checker.visitMember(targetExp, true) + accessChain = append(accessChain, memberType) + default: + inAccessChain = false + } + } + + return +} + // We have to prevent any writes to references, since we cannot know where the value // pointed to by the reference may have come from. Similarly, we can never safely assign // to a resource; because resources are moved instead of copied, we cannot currently @@ -162,31 +191,7 @@ func (checker *Checker) enforceViewAssignment(assignment ast.Statement, target a return } - var baseVariable *Variable - var accessChain = make([]Type, 0) - var inAccessChain = true - - // seek the variable expression (if it exists) at the base of the access chain - for inAccessChain { - switch targetExp := target.(type) { - case *ast.IdentifierExpression: - baseVariable = checker.valueActivations.Find(targetExp.Identifier.Identifier) - if baseVariable != nil { - accessChain = append(accessChain, baseVariable.Type) - } - inAccessChain = false - case *ast.IndexExpression: - target = targetExp.TargetExpression - elementType := checker.Elaboration.IndexExpressionTypes(targetExp).IndexedType.ElementType(true) - accessChain = append(accessChain, elementType) - case *ast.MemberExpression: - target = targetExp.Expression - memberType, _, _, _ := checker.visitMember(targetExp, true) - accessChain = append(accessChain, memberType) - default: - inAccessChain = false - } - } + baseVariable, accessChain := checker.rootOfAccessChain(target) // if the base of the access chain is not a variable, then we cannot make any static guarantees about // whether or not it is a local struct-kinded variable. E.g. in the case of `(b ? s1 : s2).x`, we can't @@ -312,7 +317,7 @@ func (checker *Checker) visitAssignmentValueType( func (checker *Checker) visitIdentifierExpressionAssignment( target *ast.IdentifierExpression, ) (targetType Type) { - identifier := target.Identifier.Identifier + identifier := target.Identifier // check identifier was declared before variable := checker.findAndCheckValueVariable(target, true) @@ -320,11 +325,15 @@ func (checker *Checker) visitIdentifierExpressionAssignment( return InvalidType } + if variable.Type.IsResourceType() { + checker.checkResourceVariableCapturingInFunction(variable, identifier) + } + // check identifier is not a constant if variable.IsConstant { checker.report( &AssignmentToConstantError{ - Name: identifier, + Name: identifier.Identifier, Range: ast.NewRangeFromPositioned(checker.memoryGauge, target), }, ) @@ -483,7 +492,28 @@ func (checker *Checker) visitMemberExpressionAssignment( reportAssignmentToConstant() } - return memberType + memberType = member.TypeAnnotation.Type + + if !memberType.IsResourceType() { + return + } + + // if the member is a resource, check that it is not captured in a function, + // based off the activation depth of the root of the access chain, i.e. `a` in `a.b.c` + // we only want to make this check for transactions, as they are the only "resource-like" types + // (that can contain resources and must destroy them in their `execute` blocks), that are themselves + // not checked by the capturing logic, since they are not themselves resources. + baseVariable, _ := checker.rootOfAccessChain(target) + + if baseVariable == nil { + return + } + + if _, isTransaction := baseVariable.Type.(*TransactionType); isTransaction { + checker.checkResourceVariableCapturingInFunction(baseVariable, member.Identifier) + } + + return } func IsValidAssignmentTargetExpression(expression ast.Expression) bool { diff --git a/runtime/tests/checker/access_test.go b/runtime/tests/checker/access_test.go index 916a7258fe..c450ded70c 100644 --- a/runtime/tests/checker/access_test.go +++ b/runtime/tests/checker/access_test.go @@ -1859,20 +1859,22 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi require.IsType(t, &sema.ResourceCapturingError{}, errs[1]) - require.IsType(t, &sema.AssignmentToConstantError{}, errs[2]) + require.IsType(t, &sema.ResourceCapturingError{}, errs[2]) + + require.IsType(t, &sema.AssignmentToConstantError{}, errs[3]) assert.Equal(t, "x", - errs[2].(*sema.AssignmentToConstantError).Name, + errs[3].(*sema.AssignmentToConstantError).Name, ) - require.IsType(t, &sema.ResourceCapturingError{}, errs[3]) - require.IsType(t, &sema.ResourceCapturingError{}, errs[4]) - require.IsType(t, &sema.AssignmentToConstantError{}, errs[5]) + require.IsType(t, &sema.ResourceCapturingError{}, errs[5]) + + require.IsType(t, &sema.AssignmentToConstantError{}, errs[6]) assert.Equal(t, "y", - errs[5].(*sema.AssignmentToConstantError).Name, + errs[6].(*sema.AssignmentToConstantError).Name, ) require.IsType(t, &sema.ResourceCapturingError{}, errs[6]) diff --git a/runtime/tests/checker/resources_test.go b/runtime/tests/checker/resources_test.go index 99def38e62..9be6bac225 100644 --- a/runtime/tests/checker/resources_test.go +++ b/runtime/tests/checker/resources_test.go @@ -10055,3 +10055,147 @@ func TestCheckIndexingResourceLoss(t *testing.T) { assert.IsType(t, &sema.InvalidNestedResourceMoveError{}, errs[1]) }) } + +func TestCheckInvalidResourceCaptureOnLeft(t *testing.T) { + + t.Parallel() + + _, err := ParseAndCheck(t, ` + fun test() { + var x: @AnyResource? <- nil + fun () { + x <-! [] + } + destroy x + } + `) + errs := RequireCheckerErrors(t, err, 1) + + assert.IsType(t, &sema.ResourceCapturingError{}, errs[0]) +} + +func TestCheckInvalidNestedResourceCapture(t *testing.T) { + + t.Parallel() + + t.Run("on right", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction { + var x: @AnyResource? + prepare() { + self.x <- nil + } + execute { + fun() { + let y <- self.x + destroy y + } + } + } + `) + require.NoError(t, err) + }) + + t.Run("resource field on right", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + resource R { + var x: @AnyResource? + init() { + self.x <- nil + } + fun foo() { + fun() { + let y <- self.x <- nil + destroy y + } + } + destroy() { + destroy self.x + } + } + `) + errs := RequireCheckerErrors(t, err, 1) + + assert.IsType(t, &sema.ResourceCapturingError{}, errs[0]) + }) + + t.Run("on left", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction { + var x: @AnyResource? + prepare() { + self.x <- nil + } + execute { + fun() { + self.x <-! nil + } + destroy self.x + } + } + `) + errs := RequireCheckerErrors(t, err, 1) + + assert.IsType(t, &sema.ResourceCapturingError{}, errs[0]) + }) + + t.Run("on left method scope", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction { + var x: @AnyResource? + prepare() { + self.x <- nil + } + execute { + self.x <-! nil + destroy self.x + } + } + `) + require.NoError(t, err) + }) + + t.Run("contract self variable on left", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + contract C { + var x: @AnyResource? + init() { + self.x <- nil + } + fun foo() { + fun() { + self.x <-! nil + } + } + } + `) + require.NoError(t, err) + }) + + t.Run("contract self variable on left method scope", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + contract C { + var x: @AnyResource? + init() { + self.x <- nil + } + fun foo() { + self.x <-! nil + } + } + `) + require.NoError(t, err) + }) +} diff --git a/runtime/tests/checker/transactions_test.go b/runtime/tests/checker/transactions_test.go index 7262162c33..f3c512be2d 100644 --- a/runtime/tests/checker/transactions_test.go +++ b/runtime/tests/checker/transactions_test.go @@ -523,3 +523,34 @@ func TestCheckInvalidTransactionSelfMoveIntoDictionaryLiteral(t *testing.T) { assert.IsType(t, &sema.InvalidMoveError{}, errs[0]) } + +func TestCheckInvalidTransactionResourceLoss(t *testing.T) { + + t.Parallel() + + _, err := ParseAndCheck(t, ` + access(all) resource R{} + transaction { + var r: @R? + + prepare() { + self.r <- nil + } + + execute { + let writeback = fun() { + self.r <-! create R() + } + + var x <- self.r + + destroy x + writeback() + } + } + `) + + errs := RequireCheckerErrors(t, err, 1) + + assert.IsType(t, &sema.ResourceCapturingError{}, errs[0]) +} From 63abc0f77f9800011bc66caa93b4965f28b4964b Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 30 Apr 2024 13:29:23 -0400 Subject: [PATCH 3/4] apply suggested fixes --- runtime/sema/check_assignment.go | 29 +++++++++++++--------------- runtime/tests/checker/access_test.go | 1 + 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/runtime/sema/check_assignment.go b/runtime/sema/check_assignment.go index 1854667788..b1b7898ecc 100644 --- a/runtime/sema/check_assignment.go +++ b/runtime/sema/check_assignment.go @@ -136,7 +136,6 @@ func (checker *Checker) checkAssignment( } func (checker *Checker) rootOfAccessChain(target ast.Expression) (baseVariable *Variable, accessChain []Type) { - accessChain = make([]Type, 0) var inAccessChain = true // seek the variable expression (if it exists) at the base of the access chain @@ -494,23 +493,21 @@ func (checker *Checker) visitMemberExpressionAssignment( memberType = member.TypeAnnotation.Type - if !memberType.IsResourceType() { - return - } - - // if the member is a resource, check that it is not captured in a function, - // based off the activation depth of the root of the access chain, i.e. `a` in `a.b.c` - // we only want to make this check for transactions, as they are the only "resource-like" types - // (that can contain resources and must destroy them in their `execute` blocks), that are themselves - // not checked by the capturing logic, since they are not themselves resources. - baseVariable, _ := checker.rootOfAccessChain(target) + if memberType.IsResourceType() { + // if the member is a resource, check that it is not captured in a function, + // based off the activation depth of the root of the access chain, i.e. `a` in `a.b.c` + // we only want to make this check for transactions, as they are the only "resource-like" types + // (that can contain resources and must destroy them in their `execute` blocks), that are themselves + // not checked by the capturing logic, since they are not themselves resources. + baseVariable, _ := checker.rootOfAccessChain(target) - if baseVariable == nil { - return - } + if baseVariable == nil { + return + } - if _, isTransaction := baseVariable.Type.(*TransactionType); isTransaction { - checker.checkResourceVariableCapturingInFunction(baseVariable, member.Identifier) + if _, isTransaction := baseVariable.Type.(*TransactionType); isTransaction { + checker.checkResourceVariableCapturingInFunction(baseVariable, member.Identifier) + } } return diff --git a/runtime/tests/checker/access_test.go b/runtime/tests/checker/access_test.go index c450ded70c..2c604a9df1 100644 --- a/runtime/tests/checker/access_test.go +++ b/runtime/tests/checker/access_test.go @@ -1826,6 +1826,7 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi `) require.NoError(t, err) + // these capture x and y because they are created in a different file _, err = ParseAndCheckWithOptions(t, ` import x, y, createR from "imported" From e0d55364412c6e53d889f18cfb76969ecbba4aad Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Thu, 9 May 2024 10:08:03 -0700 Subject: [PATCH 4/4] Fix merge --- runtime/sema/check_assignment.go | 2 -- runtime/tests/checker/access_test.go | 14 ++++++++++---- runtime/tests/checker/resources_test.go | 3 --- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/runtime/sema/check_assignment.go b/runtime/sema/check_assignment.go index b1b7898ecc..e8a3dd03eb 100644 --- a/runtime/sema/check_assignment.go +++ b/runtime/sema/check_assignment.go @@ -491,8 +491,6 @@ func (checker *Checker) visitMemberExpressionAssignment( reportAssignmentToConstant() } - memberType = member.TypeAnnotation.Type - if memberType.IsResourceType() { // if the member is a resource, check that it is not captured in a function, // based off the activation depth of the root of the access chain, i.e. `a` in `a.b.c` diff --git a/runtime/tests/checker/access_test.go b/runtime/tests/checker/access_test.go index 2c604a9df1..e91c640d3b 100644 --- a/runtime/tests/checker/access_test.go +++ b/runtime/tests/checker/access_test.go @@ -1850,7 +1850,9 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi }, ) - errs := RequireCheckerErrors(t, err, 7) + errs := RequireCheckerErrors(t, err, 9) + + // For `x` require.IsType(t, &sema.InvalidAccessError{}, errs[0]) assert.Equal(t, @@ -1870,15 +1872,19 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi require.IsType(t, &sema.ResourceCapturingError{}, errs[4]) + // For `y` + require.IsType(t, &sema.ResourceCapturingError{}, errs[5]) - require.IsType(t, &sema.AssignmentToConstantError{}, errs[6]) + require.IsType(t, &sema.ResourceCapturingError{}, errs[6]) + + require.IsType(t, &sema.AssignmentToConstantError{}, errs[7]) assert.Equal(t, "y", - errs[6].(*sema.AssignmentToConstantError).Name, + errs[7].(*sema.AssignmentToConstantError).Name, ) - require.IsType(t, &sema.ResourceCapturingError{}, errs[6]) + require.IsType(t, &sema.ResourceCapturingError{}, errs[8]) } func TestCheckContractNestedDeclarationPrivateAccess(t *testing.T) { diff --git a/runtime/tests/checker/resources_test.go b/runtime/tests/checker/resources_test.go index 9be6bac225..db21d40bc1 100644 --- a/runtime/tests/checker/resources_test.go +++ b/runtime/tests/checker/resources_test.go @@ -10113,9 +10113,6 @@ func TestCheckInvalidNestedResourceCapture(t *testing.T) { destroy y } } - destroy() { - destroy self.x - } } `) errs := RequireCheckerErrors(t, err, 1)