From 97d55e558f3d8585ad814deedf49205af495feb1 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Mon, 22 Apr 2024 11:35:56 -0700 Subject: [PATCH 1/4] Improve static validation of contract/transaction moves --- runtime/resource_duplicate_test.go | 6 +- runtime/sema/check_array_expression.go | 1 - runtime/sema/check_assignment.go | 1 - runtime/sema/check_attach_expression.go | 1 - runtime/sema/check_dictionary_expression.go | 2 - runtime/sema/check_expression.go | 41 ++++++++++++ runtime/sema/check_invocation_expression.go | 3 - runtime/sema/check_return_statement.go | 1 - runtime/sema/check_variable_declaration.go | 2 - runtime/sema/checker.go | 34 ---------- runtime/tests/checker/attachments_test.go | 21 ++++-- runtime/tests/checker/composite_test.go | 39 +++++++++-- runtime/tests/checker/contract_test.go | 7 +- runtime/tests/checker/events_test.go | 7 +- runtime/tests/checker/move_test.go | 64 +++++++++++++++++++ runtime/tests/checker/nesting_test.go | 4 +- runtime/tests/checker/operations_test.go | 17 ++++- runtime/tests/checker/optional_test.go | 24 ++++++- runtime/tests/checker/resources_test.go | 64 ++++++++++++++++--- runtime/tests/checker/transactions_test.go | 9 ++- .../tests/interpreter/composite_value_test.go | 12 +++- runtime/tests/interpreter/condition_test.go | 4 +- runtime/tests/interpreter/interpreter_test.go | 4 +- 23 files changed, 284 insertions(+), 84 deletions(-) create mode 100644 runtime/tests/checker/move_test.go diff --git a/runtime/resource_duplicate_test.go b/runtime/resource_duplicate_test.go index 6d43f669f8..4fbd33be2e 100644 --- a/runtime/resource_duplicate_test.go +++ b/runtime/resource_duplicate_test.go @@ -29,7 +29,7 @@ import ( "github.com/onflow/cadence/encoding/json" . "github.com/onflow/cadence/runtime" "github.com/onflow/cadence/runtime/common" - "github.com/onflow/cadence/runtime/interpreter" + "github.com/onflow/cadence/runtime/sema" . "github.com/onflow/cadence/runtime/tests/runtime_utils" . "github.com/onflow/cadence/runtime/tests/utils" ) @@ -204,6 +204,6 @@ func TestRuntimeResourceDuplicationWithContractTransfer(t *testing.T) { ) RequireError(t, err) - var nonTransferableValueError interpreter.NonTransferableValueError - require.ErrorAs(t, err, &nonTransferableValueError) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, err, &invalidMoveError) } diff --git a/runtime/sema/check_array_expression.go b/runtime/sema/check_array_expression.go index f957796c02..6007822ffc 100644 --- a/runtime/sema/check_array_expression.go +++ b/runtime/sema/check_array_expression.go @@ -73,7 +73,6 @@ func (checker *Checker) VisitArrayExpression(expression *ast.ArrayExpression) Ty argumentTypes[i] = valueType - checker.checkVariableMove(value) checker.checkResourceMoveOperation(value, valueType) } } diff --git a/runtime/sema/check_assignment.go b/runtime/sema/check_assignment.go index b81372eb6b..56f03290b3 100644 --- a/runtime/sema/check_assignment.go +++ b/runtime/sema/check_assignment.go @@ -110,7 +110,6 @@ func (checker *Checker) checkAssignment( } checker.enforceViewAssignment(assignment, target) - checker.checkVariableMove(value) checker.recordResourceInvalidation( value, diff --git a/runtime/sema/check_attach_expression.go b/runtime/sema/check_attach_expression.go index 3e4c35096f..d06681ed98 100644 --- a/runtime/sema/check_attach_expression.go +++ b/runtime/sema/check_attach_expression.go @@ -41,7 +41,6 @@ func (checker *Checker) VisitAttachExpression(expression *ast.AttachExpression) return InvalidType } - checker.checkVariableMove(baseExpression) checker.checkResourceMoveOperation(baseExpression, attachmentType) // check that the attachment type is a valid attachment, diff --git a/runtime/sema/check_dictionary_expression.go b/runtime/sema/check_dictionary_expression.go index 327c1d209d..bcbb92eb7f 100644 --- a/runtime/sema/check_dictionary_expression.go +++ b/runtime/sema/check_dictionary_expression.go @@ -51,11 +51,9 @@ func (checker *Checker) VisitDictionaryExpression(expression *ast.DictionaryExpr // not combined after both type checks! entryKeyType := checker.VisitExpression(entry.Key, expression, keyType) - checker.checkVariableMove(entry.Key) checker.checkResourceMoveOperation(entry.Key, entryKeyType) entryValueType := checker.VisitExpression(entry.Value, expression, valueType) - checker.checkVariableMove(entry.Value) checker.checkResourceMoveOperation(entry.Value, entryValueType) entryTypes[i] = DictionaryEntryType{ diff --git a/runtime/sema/check_expression.go b/runtime/sema/check_expression.go index 4dc71ac4bd..f6a9b6dad0 100644 --- a/runtime/sema/check_expression.go +++ b/runtime/sema/check_expression.go @@ -46,9 +46,50 @@ func (checker *Checker) VisitIdentifierExpression(expression *ast.IdentifierExpr checker.Elaboration.SetIdentifierInInvocationType(expression, valueType) } + checker.checkVariableMove(expression, variable) + return valueType } +func (checker *Checker) checkVariableMove(IdentifierExpression *ast.IdentifierExpression, variable *Variable) { + + reportMaybeInvalidMove := func(declarationKind common.DeclarationKind) { + // If the parent is member-access or index-access, then it's OK. + // e.g: `v.foo`, `v.bar()`, `v[T]` are OK. + switch parent := checker.parent.(type) { + case *ast.MemberExpression: + // TODO: No need for below check? i.e: should always be true + if parent.Expression == IdentifierExpression { + return + } + case *ast.IndexExpression: + // Only `v[foo]` is OK, `foo[v]` is not. + if parent.TargetExpression == IdentifierExpression { + return + } + } + + checker.report( + &InvalidMoveError{ + Name: variable.Identifier, + DeclarationKind: declarationKind, + Pos: IdentifierExpression.StartPosition(), + }, + ) + } + + switch ty := variable.Type.(type) { + case *TransactionType: + reportMaybeInvalidMove(common.DeclarationKindTransaction) + + case CompositeKindedType: + kind := ty.GetCompositeKind() + if kind == common.CompositeKindContract { + reportMaybeInvalidMove(common.DeclarationKindContract) + } + } +} + func (checker *Checker) checkReferenceValidity(variable *Variable, hasPosition ast.HasPosition) { typ := UnwrapOptionalType(variable.Type) if _, ok := typ.(*ReferenceType); !ok { diff --git a/runtime/sema/check_invocation_expression.go b/runtime/sema/check_invocation_expression.go index fa07694d2b..c118850336 100644 --- a/runtime/sema/check_invocation_expression.go +++ b/runtime/sema/check_invocation_expression.go @@ -807,9 +807,6 @@ func (checker *Checker) checkInvocationArgumentParameterTypeCompatibility( } func (checker *Checker) checkInvocationArgumentMove(argument ast.Expression, argumentType Type) Type { - - checker.checkVariableMove(argument) checker.checkResourceMoveOperation(argument, argumentType) - return argumentType } diff --git a/runtime/sema/check_return_statement.go b/runtime/sema/check_return_statement.go index a4b144ce0c..ad98d274fd 100644 --- a/runtime/sema/check_return_statement.go +++ b/runtime/sema/check_return_statement.go @@ -74,7 +74,6 @@ func (checker *Checker) VisitReturnStatement(statement *ast.ReturnStatement) (_ return } - checker.checkVariableMove(statement.Expression) checker.checkResourceMoveOperation(statement.Expression, valueType) return diff --git a/runtime/sema/check_variable_declaration.go b/runtime/sema/check_variable_declaration.go index 66afecc444..cc9a642fa0 100644 --- a/runtime/sema/check_variable_declaration.go +++ b/runtime/sema/check_variable_declaration.go @@ -113,8 +113,6 @@ func (checker *Checker) visitVariableDeclarationValues(declaration *ast.Variable panic(errors.NewUnreachableError()) } - checker.checkVariableMove(declaration.Value) - // If only one value expression is provided, it is invalidated (if it has a resource type) checker.recordResourceInvalidation( diff --git a/runtime/sema/checker.go b/runtime/sema/checker.go index e62b5599a7..7846ed8c38 100644 --- a/runtime/sema/checker.go +++ b/runtime/sema/checker.go @@ -2300,40 +2300,6 @@ func (checker *Checker) predeclaredMembers(containerType Type) []*Member { return predeclaredMembers } -func (checker *Checker) checkVariableMove(expression ast.Expression) { - - identifierExpression, ok := expression.(*ast.IdentifierExpression) - if !ok { - return - } - - variable := checker.valueActivations.Find(identifierExpression.Identifier.Identifier) - if variable == nil { - return - } - - reportInvalidMove := func(declarationKind common.DeclarationKind) { - checker.report( - &InvalidMoveError{ - Name: variable.Identifier, - DeclarationKind: declarationKind, - Pos: identifierExpression.StartPosition(), - }, - ) - } - - switch ty := variable.Type.(type) { - case *TransactionType: - reportInvalidMove(common.DeclarationKindTransaction) - - case CompositeKindedType: - kind := ty.GetCompositeKind() - if kind == common.CompositeKindContract { - reportInvalidMove(common.DeclarationKindContract) - } - } -} - func (checker *Checker) checkTypeAnnotation(typeAnnotation TypeAnnotation, pos ast.HasPosition) { switch typeAnnotation.TypeAnnotationState() { diff --git a/runtime/tests/checker/attachments_test.go b/runtime/tests/checker/attachments_test.go index 9294ed964a..c49b7e839f 100644 --- a/runtime/tests/checker/attachments_test.go +++ b/runtime/tests/checker/attachments_test.go @@ -1540,9 +1540,12 @@ func TestCheckAttachmentIllegalInit(t *testing.T) { let t = optContractRef?.Test() `) - errs := RequireCheckerErrors(t, err, 1) + errs := RequireCheckerErrors(t, err, 2) - assert.IsType(t, &sema.InvalidAttachmentUsageError{}, errs[0]) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + + assert.IsType(t, &sema.InvalidAttachmentUsageError{}, errs[1]) }) } @@ -1655,9 +1658,12 @@ func TestCheckAttachmentAttachNonAttachment(t *testing.T) { `, ) - errs := RequireCheckerErrors(t, err, 1) + errs := RequireCheckerErrors(t, err, 2) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) - assert.IsType(t, &sema.NotCallableError{}, errs[0]) + assert.IsType(t, &sema.NotCallableError{}, errs[1]) }) } @@ -1773,9 +1779,12 @@ func TestCheckAttachmentAttachToNonComposite(t *testing.T) { `, ) - errs := RequireCheckerErrors(t, err, 1) + errs := RequireCheckerErrors(t, err, 2) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) - assert.IsType(t, &sema.NotCallableError{}, errs[0]) + assert.IsType(t, &sema.NotCallableError{}, errs[1]) }) t.Run("attachment", func(t *testing.T) { diff --git a/runtime/tests/checker/composite_test.go b/runtime/tests/checker/composite_test.go index a57ac1445c..b577207ec0 100644 --- a/runtime/tests/checker/composite_test.go +++ b/runtime/tests/checker/composite_test.go @@ -633,9 +633,14 @@ func TestCheckCompositeInitializerSelfUse(t *testing.T) { ) switch kind { - case common.CompositeKindStructure, common.CompositeKindContract, common.CompositeKindAttachment: + case common.CompositeKindStructure, common.CompositeKindAttachment: require.NoError(t, err) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 1) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + case common.CompositeKindResource: errs := RequireCheckerErrors(t, err, 1) @@ -674,9 +679,14 @@ func TestCheckCompositeFunctionSelfUse(t *testing.T) { ) switch kind { - case common.CompositeKindStructure, common.CompositeKindContract, common.CompositeKindAttachment: + case common.CompositeKindStructure, common.CompositeKindAttachment: require.NoError(t, err) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 1) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + case common.CompositeKindResource: errs := RequireCheckerErrors(t, err, 1) @@ -1981,19 +1991,36 @@ func TestCheckMutualTypeUseTopLevel(t *testing.T) { ) firstBody := "" + firstCallableFunc := "fun foo()" if !firstIsInterface { + usage := "b" + if secondKind == common.CompositeKindContract { + usage = usage + ".foo()" + } + firstBody = fmt.Sprintf( - "{ %s b }", + "{ %s %s }", secondKind.DestructionKeyword(), + usage, ) + + firstCallableFunc = firstCallableFunc + " {}" } secondBody := "" + secondCallableFunc := "fun foo()" if !secondIsInterface { + usage := "a" + if firstKind == common.CompositeKindContract { + usage = usage + ".foo()" + } + secondBody = fmt.Sprintf( - "{ %s a }", + "{ %s %s }", firstKind.DestructionKeyword(), + usage, ) + secondCallableFunc = secondCallableFunc + " {}" } t.Run(testName, func(t *testing.T) { @@ -2002,10 +2029,12 @@ func TestCheckMutualTypeUseTopLevel(t *testing.T) { ` %[1]s %[2]s A { fun use(_ b: %[3]s%[4]s) %[5]s + %[11]s } %[6]s %[7]s B { fun use(_ a: %[8]s%[9]s) %[10]s + %[12]s } `, firstKind.Keyword(), @@ -2018,6 +2047,8 @@ func TestCheckMutualTypeUseTopLevel(t *testing.T) { firstKind.Annotation(), firstTypeAnnotation, secondBody, + firstCallableFunc, + secondCallableFunc, ) _, err := ParseAndCheck(t, code) diff --git a/runtime/tests/checker/contract_test.go b/runtime/tests/checker/contract_test.go index 6f4a4ea847..c56026e0fc 100644 --- a/runtime/tests/checker/contract_test.go +++ b/runtime/tests/checker/contract_test.go @@ -744,7 +744,12 @@ func TestCheckBadContractNesting(t *testing.T) { func TestCheckContractEnumAccessRestricted(t *testing.T) { t.Parallel() - _, err := ParseAndCheckWithOptions(t, "contract foo{}let x = foo!", + _, err := ParseAndCheckWithOptions(t, ` + contract foo { + access(all) fun bar() {} + } + let x = foo.bar()! + `, ParseAndCheckOptions{ Config: &sema.Config{ AccessCheckMode: sema.AccessCheckModeStrict, diff --git a/runtime/tests/checker/events_test.go b/runtime/tests/checker/events_test.go index b5435cf914..076da567e8 100644 --- a/runtime/tests/checker/events_test.go +++ b/runtime/tests/checker/events_test.go @@ -250,9 +250,12 @@ func TestCheckEmitEvent(t *testing.T) { } `) - errs := RequireCheckerErrors(t, err, 1) + errs := RequireCheckerErrors(t, err, 2) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) - assert.IsType(t, &sema.InvalidEventUsageError{}, errs[0]) + assert.IsType(t, &sema.InvalidEventUsageError{}, errs[1]) }) t.Run("emit non-event", func(t *testing.T) { diff --git a/runtime/tests/checker/move_test.go b/runtime/tests/checker/move_test.go new file mode 100644 index 0000000000..da8b314853 --- /dev/null +++ b/runtime/tests/checker/move_test.go @@ -0,0 +1,64 @@ +/* + * Cadence - The resource-oriented smart contract programming language + * + * Copyright Dapper Labs, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package checker + +import ( + "github.com/onflow/cadence/runtime/sema" + "github.com/stretchr/testify/require" + "testing" +) + +func TestCheckInvalidMoves(t *testing.T) { + + t.Parallel() + + t.Run("contract", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + access(all) contract Foo { + access(all) fun moveSelf() { + var x = self! + } + } + `) + + errors := RequireCheckerErrors(t, err, 1) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errors[0], &invalidMoveError) + }) + + t.Run("transaction", func(t *testing.T) { + t.Parallel() + + _, err := ParseAndCheck(t, ` + transaction { + prepare() { + var x = true ? self : self + } + execute {} + } + `) + + errors := RequireCheckerErrors(t, err, 2) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errors[0], &invalidMoveError) + require.ErrorAs(t, errors[1], &invalidMoveError) + }) +} diff --git a/runtime/tests/checker/nesting_test.go b/runtime/tests/checker/nesting_test.go index 47c125c638..c56279b7e8 100644 --- a/runtime/tests/checker/nesting_test.go +++ b/runtime/tests/checker/nesting_test.go @@ -197,9 +197,11 @@ func TestCheckCompositeDeclarationNestedTypeScopingInsideNestedOuter(t *testing. struct X { fun test() { - Test + Test.foo() } } + + fun foo() {} } `) diff --git a/runtime/tests/checker/operations_test.go b/runtime/tests/checker/operations_test.go index 38c02bcc7f..3781d18d4b 100644 --- a/runtime/tests/checker/operations_test.go +++ b/runtime/tests/checker/operations_test.go @@ -584,9 +584,10 @@ func TestCheckInvalidCompositeEquality(t *testing.T) { ), ) - if compositeKind == common.CompositeKindEnum { + switch compositeKind { + case common.CompositeKindEnum: require.NoError(t, err) - } else if compositeKind == common.CompositeKindAttachment { + case common.CompositeKindAttachment: errs := RequireCheckerErrors(t, err, 5) assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[0]) @@ -594,7 +595,17 @@ func TestCheckInvalidCompositeEquality(t *testing.T) { assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[2]) assert.IsType(t, &sema.InvalidAttachmentUsageError{}, errs[3]) assert.IsType(t, &sema.InvalidBinaryOperandsError{}, errs[4]) - } else { + + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 3) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + require.ErrorAs(t, errs[1], &invalidMoveError) + + assert.IsType(t, &sema.InvalidBinaryOperandsError{}, errs[2]) + + default: errs := RequireCheckerErrors(t, err, 1) assert.IsType(t, &sema.InvalidBinaryOperandsError{}, errs[0]) diff --git a/runtime/tests/checker/optional_test.go b/runtime/tests/checker/optional_test.go index 0a40571461..91d3bb9688 100644 --- a/runtime/tests/checker/optional_test.go +++ b/runtime/tests/checker/optional_test.go @@ -264,7 +264,14 @@ func TestCheckCompositeNilEquality(t *testing.T) { ), ) - require.NoError(t, err) + if compositeKind == common.CompositeKindContract { + errs := RequireCheckerErrors(t, err, 2) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + require.ErrorAs(t, errs[1], &invalidMoveError) + } else { + require.NoError(t, err) + } }) } @@ -338,9 +345,20 @@ func TestCheckInvalidCompositeNilEquality(t *testing.T) { ), ) - if compositeKind == common.CompositeKindEnum { + switch compositeKind { + case common.CompositeKindEnum: require.NoError(t, err) - } else { + + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 3) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + require.ErrorAs(t, errs[1], &invalidMoveError) + + assert.IsType(t, &sema.InvalidBinaryOperandsError{}, errs[2]) + + default: errs := RequireCheckerErrors(t, err, 1) assert.IsType(t, &sema.InvalidBinaryOperandsError{}, errs[0]) diff --git a/runtime/tests/checker/resources_test.go b/runtime/tests/checker/resources_test.go index 3823366f6e..4af1b8a434 100644 --- a/runtime/tests/checker/resources_test.go +++ b/runtime/tests/checker/resources_test.go @@ -93,13 +93,19 @@ func TestCheckFailableCastingWithResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidAttachmentUsageError{}, errs[1]) case common.CompositeKindStructure, - common.CompositeKindContract, common.CompositeKindEnum: errs := RequireCheckerErrors(t, err, 1) + assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) + + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 2) assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[1], &invalidMoveError) + case common.CompositeKindEvent: errs := RequireCheckerErrors(t, err, 2) @@ -171,7 +177,6 @@ func TestCheckFunctionDeclarationParameterWithResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[0]) case common.CompositeKindStructure, - common.CompositeKindContract, common.CompositeKindEvent, common.CompositeKindEnum: @@ -179,6 +184,14 @@ func TestCheckFunctionDeclarationParameterWithResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 2) + + assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[1], &invalidMoveError) + default: panic(errors.NewUnreachableError()) } @@ -244,12 +257,16 @@ func TestCheckFunctionDeclarationParameterWithoutResourceAnnotation(t *testing.T assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[0]) case common.CompositeKindStructure, - common.CompositeKindContract, common.CompositeKindEvent, common.CompositeKindEnum: require.NoError(t, err) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 1) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + default: panic(errors.NewUnreachableError()) } @@ -779,7 +796,6 @@ func TestCheckFunctionExpressionParameterWithResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[0]) case common.CompositeKindStructure, - common.CompositeKindContract, common.CompositeKindEvent, common.CompositeKindEnum: @@ -787,6 +803,14 @@ func TestCheckFunctionExpressionParameterWithResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 2) + + assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[1], &invalidMoveError) + default: panic(errors.NewUnreachableError()) } @@ -854,12 +878,17 @@ func TestCheckFunctionExpressionParameterWithoutResourceAnnotation(t *testing.T) assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[0]) case common.CompositeKindStructure, - common.CompositeKindContract, common.CompositeKindEvent, common.CompositeKindEnum: require.NoError(t, err) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 1) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + default: panic(errors.NewUnreachableError()) } @@ -1097,7 +1126,6 @@ func TestCheckFunctionTypeParameterWithResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[0]) case common.CompositeKindStructure, - common.CompositeKindContract, common.CompositeKindEvent, common.CompositeKindEnum: @@ -1106,6 +1134,15 @@ func TestCheckFunctionTypeParameterWithResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[1]) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 3) + + assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[0]) + assert.IsType(t, &sema.InvalidResourceAnnotationError{}, errs[1]) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[2], &invalidMoveError) + default: panic(errors.NewUnreachableError()) } @@ -1175,12 +1212,16 @@ func TestCheckFunctionTypeParameterWithoutResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidAttachmentAnnotationError{}, errs[1]) case common.CompositeKindStructure, - common.CompositeKindContract, common.CompositeKindEvent, common.CompositeKindEnum: require.NoError(t, err) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 1) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + default: panic(errors.NewUnreachableError()) } @@ -1408,11 +1449,14 @@ func TestCheckFailableCastingWithoutResourceAnnotation(t *testing.T) { assert.IsType(t, &sema.InvalidFailableResourceDowncastOutsideOptionalBindingError{}, errs[1]) assert.IsType(t, &sema.InvalidNonIdentifierFailableResourceDowncast{}, errs[2]) - case common.CompositeKindStructure, - common.CompositeKindContract: - + case common.CompositeKindStructure: require.NoError(t, err) + case common.CompositeKindContract: + errs := RequireCheckerErrors(t, err, 1) + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) + case common.CompositeKindEvent: errs := RequireCheckerErrors(t, err, 1) diff --git a/runtime/tests/checker/transactions_test.go b/runtime/tests/checker/transactions_test.go index f3c512be2d..c36b1a560d 100644 --- a/runtime/tests/checker/transactions_test.go +++ b/runtime/tests/checker/transactions_test.go @@ -477,10 +477,13 @@ func TestCheckInvalidTransactionSelfMoveReturnFromFunction(t *testing.T) { } `) - errs := RequireCheckerErrors(t, err, 1) + errs := RequireCheckerErrors(t, err, 2) + + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, errs[0], &invalidMoveError) - require.IsType(t, &sema.TypeMismatchError{}, errs[0]) - typeMismatchErr := errs[0].(*sema.TypeMismatchError) + require.IsType(t, &sema.TypeMismatchError{}, errs[1]) + typeMismatchErr := errs[1].(*sema.TypeMismatchError) assert.Equal(t, sema.VoidType, typeMismatchErr.ExpectedType) assert.IsType(t, &sema.TransactionType{}, typeMismatchErr.ActualType) diff --git a/runtime/tests/interpreter/composite_value_test.go b/runtime/tests/interpreter/composite_value_test.go index 07c1106fae..21ed849bb9 100644 --- a/runtime/tests/interpreter/composite_value_test.go +++ b/runtime/tests/interpreter/composite_value_test.go @@ -194,7 +194,17 @@ func TestInterpretContractTransfer(t *testing.T) { `, value, ) - inter, _ := testAccount(t, address, true, nil, code, sema.Config{}) + inter, _ := testAccountWithErrorHandler( + t, + address, + true, + nil, + code, + sema.Config{}, + func(err error) { + var invalidMoveError *sema.InvalidMoveError + require.ErrorAs(t, err, &invalidMoveError) + }) _, err := inter.Invoke("test") RequireError(t, err) diff --git a/runtime/tests/interpreter/condition_test.go b/runtime/tests/interpreter/condition_test.go index 841b1f0c36..44ddfb75fa 100644 --- a/runtime/tests/interpreter/condition_test.go +++ b/runtime/tests/interpreter/condition_test.go @@ -835,7 +835,7 @@ func TestInterpretInitializerWithInterfacePreCondition(t *testing.T) { // use the contract singleton, so it is loaded testFunction = ` access(all) fun test() { - TestImpl + TestImpl.NoOpFunc() } ` } else { @@ -878,6 +878,8 @@ func TestInterpretInitializerWithInterfacePreCondition(t *testing.T) { emit Foo(x: x) } } + + access(all) fun NoOpFunc() {} } %[2]s diff --git a/runtime/tests/interpreter/interpreter_test.go b/runtime/tests/interpreter/interpreter_test.go index 35eead01e5..b9bc618358 100644 --- a/runtime/tests/interpreter/interpreter_test.go +++ b/runtime/tests/interpreter/interpreter_test.go @@ -3955,7 +3955,9 @@ func TestInterpretCompositeNilEquality(t *testing.T) { for _, compositeKind := range common.AllCompositeKinds { - if compositeKind == common.CompositeKindEvent || compositeKind == common.CompositeKindAttachment { + if compositeKind == common.CompositeKindEvent || + compositeKind == common.CompositeKindAttachment || + compositeKind == common.CompositeKindContract { continue } From 321a305fc69514b36760b6a4f761d3fbbb04ba51 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Tue, 14 May 2024 15:41:49 -0700 Subject: [PATCH 2/4] Refactor --- runtime/sema/check_expression.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/sema/check_expression.go b/runtime/sema/check_expression.go index f6a9b6dad0..007301fd5e 100644 --- a/runtime/sema/check_expression.go +++ b/runtime/sema/check_expression.go @@ -51,7 +51,7 @@ func (checker *Checker) VisitIdentifierExpression(expression *ast.IdentifierExpr return valueType } -func (checker *Checker) checkVariableMove(IdentifierExpression *ast.IdentifierExpression, variable *Variable) { +func (checker *Checker) checkVariableMove(identifierExpression *ast.IdentifierExpression, variable *Variable) { reportMaybeInvalidMove := func(declarationKind common.DeclarationKind) { // If the parent is member-access or index-access, then it's OK. @@ -59,12 +59,12 @@ func (checker *Checker) checkVariableMove(IdentifierExpression *ast.IdentifierEx switch parent := checker.parent.(type) { case *ast.MemberExpression: // TODO: No need for below check? i.e: should always be true - if parent.Expression == IdentifierExpression { + if parent.Expression == identifierExpression { return } case *ast.IndexExpression: // Only `v[foo]` is OK, `foo[v]` is not. - if parent.TargetExpression == IdentifierExpression { + if parent.TargetExpression == identifierExpression { return } } @@ -73,7 +73,7 @@ func (checker *Checker) checkVariableMove(IdentifierExpression *ast.IdentifierEx &InvalidMoveError{ Name: variable.Identifier, DeclarationKind: declarationKind, - Pos: IdentifierExpression.StartPosition(), + Pos: identifierExpression.StartPosition(), }, ) } From b2a8d8e2a7868f9eb1252a0483d8f6315a7f2f8d Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Tue, 14 May 2024 15:45:12 -0700 Subject: [PATCH 3/4] Remove invalid runtime test --- runtime/runtime_test.go | 117 ---------------------------------------- 1 file changed, 117 deletions(-) diff --git a/runtime/runtime_test.go b/runtime/runtime_test.go index 0f78771fbc..adbd5a4614 100644 --- a/runtime/runtime_test.go +++ b/runtime/runtime_test.go @@ -10634,123 +10634,6 @@ func TestRuntimeNonPublicAccessModifierInInterface(t *testing.T) { require.Len(t, conformanceErr.MemberMismatches, 2) } -func TestRuntimeMoveSelfVariable(t *testing.T) { - - t.Parallel() - - t.Run("contract", func(t *testing.T) { - t.Parallel() - - contract := []byte(` - access(all) contract Foo { - - access(all) fun moveSelf() { - var x = self! - } - } - `) - - runtime := NewTestInterpreterRuntimeWithConfig(Config{ - AtreeValidationEnabled: false, - }) - - address := common.MustBytesToAddress([]byte{0x1}) - - var contractCode []byte - - runtimeInterface := &TestRuntimeInterface{ - Storage: NewTestLedger(nil, nil), - OnGetSigningAccounts: func() ([]Address, error) { - return []Address{address}, nil - }, - OnGetAccountContractCode: func(location common.AddressLocation) ([]byte, error) { - return contractCode, nil - }, - OnResolveLocation: NewSingleIdentifierLocationResolver(t), - OnUpdateAccountContractCode: func(_ common.AddressLocation, code []byte) error { - contractCode = code - return nil - }, - OnEmitEvent: func(event cadence.Event) error { - return nil - }, - } - - nextTransactionLocation := NewTransactionLocationGenerator() - - // Deploy - - deploymentTx := DeploymentTransaction("Foo", contract) - err := runtime.ExecuteTransaction( - Script{ - Source: deploymentTx, - }, - Context{ - Interface: runtimeInterface, - Location: nextTransactionLocation(), - }, - ) - require.NoError(t, err) - - // Execute script - - nextScriptLocation := NewScriptLocationGenerator() - - script := []byte(fmt.Sprintf(` - import Foo from %[1]s - - access(all) fun main(): Void { - Foo.moveSelf() - }`, - address.HexWithPrefix(), - )) - - _, err = runtime.ExecuteScript( - Script{ - Source: script, - }, - Context{ - Interface: runtimeInterface, - Location: nextScriptLocation(), - }, - ) - - RequireError(t, err) - require.ErrorAs(t, err, &interpreter.NonTransferableValueError{}) - }) - - t.Run("transaction", func(t *testing.T) { - t.Parallel() - - script := []byte(` - transaction { - prepare() { - var x = true ? self : self - } - execute {} - } - `) - - runtime := NewTestInterpreterRuntime() - runtimeInterface := &TestRuntimeInterface{} - - nextTransactionLocation := NewTransactionLocationGenerator() - - err := runtime.ExecuteTransaction( - Script{ - Source: script, - }, - Context{ - Interface: runtimeInterface, - Location: nextTransactionLocation(), - }, - ) - - RequireError(t, err) - require.ErrorAs(t, err, &interpreter.NonTransferableValueError{}) - }) -} - func TestRuntimeContractWithInvalidCapability(t *testing.T) { t.Parallel() From 07a2143dc02ffa01e12856db0a69d3eab6c79a4b Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Wed, 15 May 2024 06:40:50 -0700 Subject: [PATCH 4/4] Make linter happy --- runtime/tests/checker/composite_test.go | 8 ++++---- runtime/tests/checker/move_test.go | 6 ++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/runtime/tests/checker/composite_test.go b/runtime/tests/checker/composite_test.go index b577207ec0..af440e83e9 100644 --- a/runtime/tests/checker/composite_test.go +++ b/runtime/tests/checker/composite_test.go @@ -1995,7 +1995,7 @@ func TestCheckMutualTypeUseTopLevel(t *testing.T) { if !firstIsInterface { usage := "b" if secondKind == common.CompositeKindContract { - usage = usage + ".foo()" + usage += ".foo()" } firstBody = fmt.Sprintf( @@ -2004,7 +2004,7 @@ func TestCheckMutualTypeUseTopLevel(t *testing.T) { usage, ) - firstCallableFunc = firstCallableFunc + " {}" + firstCallableFunc += " {}" } secondBody := "" @@ -2012,7 +2012,7 @@ func TestCheckMutualTypeUseTopLevel(t *testing.T) { if !secondIsInterface { usage := "a" if firstKind == common.CompositeKindContract { - usage = usage + ".foo()" + usage += ".foo()" } secondBody = fmt.Sprintf( @@ -2020,7 +2020,7 @@ func TestCheckMutualTypeUseTopLevel(t *testing.T) { firstKind.DestructionKeyword(), usage, ) - secondCallableFunc = secondCallableFunc + " {}" + secondCallableFunc += " {}" } t.Run(testName, func(t *testing.T) { diff --git a/runtime/tests/checker/move_test.go b/runtime/tests/checker/move_test.go index da8b314853..ddea3219e0 100644 --- a/runtime/tests/checker/move_test.go +++ b/runtime/tests/checker/move_test.go @@ -19,9 +19,11 @@ package checker import ( - "github.com/onflow/cadence/runtime/sema" - "github.com/stretchr/testify/require" "testing" + + "github.com/stretchr/testify/require" + + "github.com/onflow/cadence/runtime/sema" ) func TestCheckInvalidMoves(t *testing.T) {