Improve static validation of contract/transaction moves

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)
 }

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()

runtime/sema/check_array_expression.go

@@ -73,7 +73,6 @@ func (checker *Checker) VisitArrayExpression(arrayExpression *ast.ArrayExpressio
 
 			argumentTypes[i] = valueType
 
-			checker.checkVariableMove(element)
 			checker.checkResourceMoveOperation(element, valueType)
 		}
 	}

runtime/sema/check_assignment.go

@@ -110,7 +110,6 @@ func (checker *Checker) checkAssignment(
 	}
 
 	checker.enforceViewAssignment(assignment, target)
-	checker.checkVariableMove(value)
 
 	checker.recordResourceInvalidation(
 		value,

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,

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{

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 {

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
 }

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

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(

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() {

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) {