Merge pull request #3176 from onflow/sainati/second-value-transfer

Proper type checking for resource use after validation on two-value transfers

runtime/sema/check_variable_declaration.go

@@ -156,6 +156,32 @@ func (checker *Checker) visitVariableDeclarationValues(declaration *ast.Variable
 				)
 			}
 
+			// Given a variable declaration with a second value transfer of the form
+			// 				`let x <- e1 <- e2`
+			//
+			// the interpreter will evaluate this in the following order:
+			//  - evaluating `e1` to some value `v1` (which is necessarily either a variable, an index expression, or a member expression)
+			//  - moving the value stored in `v1` into the newly declared variable `x`
+			//  - evaluating `e2` to some value `v2`
+			//  - evaluate `e1` again to some value `v1'` (as `e1` may produce a different value after `e2`'s execution)
+			//  - lastly moving the value `v2` into the "variable" denoted by `v1'`
+			//
+			// This means that while at the end of the execution of the entire statement,
+			// `v1` is still a valid resource (having had the result of `e2` moved into it),
+			// specifically during the execution of `e2` it is invalid.
+			// In order to reflect this, we temporarily invalidate `v1` before evaluating `e2` and checking the assignment,
+			// and then re-validate `v1` afterwards.
+			// We also re-check `e1` again as well, since it is evaluated a second time by the interpreter.
+			// Note that while typechecking the same expression twice is not safe in general,
+			// in this case it is permissible for a specific reason:
+			// `e1` is necessarily an identifier, index or member expression, which may not have side effects
+
+			recordedResourceInvalidation := checker.recordResourceInvalidation(
+				declaration.Value,
+				declarationType,
+				ResourceInvalidationKindMoveTemporary,
+			)
+
 			// Check the assignment of the second value to the first expression
 
 			// The check of the assignment of the second value to the first also:
@@ -173,6 +199,11 @@ func (checker *Checker) visitVariableDeclarationValues(declaration *ast.Variable
 				declaration.SecondTransfer,
 				true,
 			)
+
+			if recordedResourceInvalidation != nil {
+				checker.resources.RemoveTemporaryMoveInvalidation(recordedResourceInvalidation.resource, recordedResourceInvalidation.invalidation)
+			}
+			checker.VisitExpression(declaration.Value, expectedValueType)
 		}
 	}
 

runtime/sema/checker.go

@@ -1556,19 +1556,27 @@ func (checker *Checker) recordResourceInvalidation(
 
 	accessedSelfMember := checker.accessedSelfMember(expression)
 
+	// Normally a field members cannot be invalidated,
+	// and this check typically prevents invalidations of this kind.
+	// However, during second value transfers we would like to be able to invalidate member and index
+	// expressions purely for the duration of checking the second value expression in the transfer.
+	// To enable this, nested move errors are not reported when the move kind of the invalidation is `Temporary`.
 	switch expression.(type) {
 	case *ast.MemberExpression:
 
-		if accessedSelfMember == nil ||
-			!checker.allowSelfResourceFieldInvalidation {
+		if (accessedSelfMember == nil ||
+			!checker.allowSelfResourceFieldInvalidation) &&
+			invalidationKind != ResourceInvalidationKindMoveTemporary {
 
 			reportInvalidNestedMove()
 			return nil
 		}
 
 	case *ast.IndexExpression:
-		reportInvalidNestedMove()
-		return nil
+		if invalidationKind != ResourceInvalidationKindMoveTemporary {
+			reportInvalidNestedMove()
+			return nil
+		}
 	}
 
 	invalidation := ResourceInvalidation{

runtime/tests/checker/access_test.go

@@ -1849,7 +1849,7 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi
 		},
 	)
 
-	errs := RequireCheckerErrors(t, err, 5)
+	errs := RequireCheckerErrors(t, err, 7)
 
 	require.IsType(t, &sema.InvalidAccessError{}, errs[0])
 	assert.Equal(t,
@@ -1867,11 +1867,15 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi
 
 	require.IsType(t, &sema.ResourceCapturingError{}, errs[3])
 
-	require.IsType(t, &sema.AssignmentToConstantError{}, errs[4])
+	require.IsType(t, &sema.ResourceCapturingError{}, errs[4])
+
+	require.IsType(t, &sema.AssignmentToConstantError{}, errs[5])
 	assert.Equal(t,
 		"y",
-		errs[4].(*sema.AssignmentToConstantError).Name,
+		errs[5].(*sema.AssignmentToConstantError).Name,
 	)
+
+	require.IsType(t, &sema.ResourceCapturingError{}, errs[6])
 }
 
 func TestCheckContractNestedDeclarationPrivateAccess(t *testing.T) {

runtime/tests/checker/resources_test.go

@@ -9859,3 +9859,150 @@ func TestCheckOptionalBindingElseBranch(t *testing.T) {
 	errs := RequireCheckerErrors(t, err, 1)
 	assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
 }
+
+func TestCheckResourceSecondValueTransfer(t *testing.T) {
+
+	t.Parallel()
+
+	t.Run("basic array invalid", func(t *testing.T) {
+		t.Parallel()
+
+		_, err := ParseAndCheck(t, `
+            resource R {}
+
+            access(all) fun main() {
+                let vaults: @[AnyResource] <- [<-[]]
+                let old <- vaults[0] <- vaults
+                destroy old
+            }
+        `)
+
+		errs := RequireCheckerErrors(t, err, 1)
+		assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
+	})
+
+	t.Run("basic array", func(t *testing.T) {
+		t.Parallel()
+
+		_, err := ParseAndCheck(t, `
+            resource R {}
+
+            access(all) fun main() {
+                let vaults: @[AnyResource] <- [<-[]]
+                let bar <- create R()
+                let old <- vaults[0] <- bar
+                destroy old
+                destroy vaults
+            }
+        `)
+
+		require.NoError(t, err)
+	})
+
+	t.Run("basic function call invalid", func(t *testing.T) {
+		t.Parallel()
+
+		_, err := ParseAndCheck(t, `
+            resource R {}
+
+            access(all) fun foo(_ r: @R): @R {
+                return <-r 
+            }
+
+            access(all) fun main() {
+                var r <- create R()
+                let r2 <- r <- foo(<-r)
+                destroy r2
+                // note that r is still "valid" after the two value transfer so we must destroy it
+                destroy r
+            }
+        `)
+
+		errs := RequireCheckerErrors(t, err, 1)
+		assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
+	})
+
+	t.Run("basic function call valid", func(t *testing.T) {
+		t.Parallel()
+
+		_, err := ParseAndCheck(t, `
+            resource R {}
+
+            access(all) fun foo(_ r: @R): @R {
+                return <-r 
+            }
+
+            access(all) fun main() {
+                var r <- create R()
+                var bar <- create R()
+                let r2 <- r <- foo(<-bar)
+                destroy r2
+                destroy r
+            }
+        `)
+
+		require.NoError(t, err)
+	})
+
+	t.Run("invalid", func(t *testing.T) {
+		t.Parallel()
+
+		_, err := ParseAndCheck(t, `
+            access(all) resource R{}
+
+            access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
+                arrRef.append(<- copy2!)
+                return <- create R()
+            }
+            
+            access(all) fun main() {
+                var victim: @R? <- create R()
+                var arr: @[R] <- []
+            
+                // In the optional binding below, the 'victim' must be invalidated
+                // before evaluation of the collect() call
+                let copy1 <- victim <- collect(copy2: <- victim, &arr as auth(Mutate) &[R])
+            
+                // Panics when trying to destroy
+                destroy copy1
+                destroy arr    
+                destroy victim
+            }
+        `)
+
+		errs := RequireCheckerErrors(t, err, 1)
+		assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
+	})
+
+	t.Run("regression", func(t *testing.T) {
+		t.Parallel()
+
+		_, err := ParseAndCheck(t, `
+            access(all) resource R{}
+
+            access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
+                arrRef.append(<- copy2!)
+                return <- create R()
+            }
+            
+            access(all) fun main() {
+                var victim: @R? <- create R()
+                var arr: @[R] <- []
+            
+                if let copy1 <- victim <- collect(copy2: <- victim, &arr as auth(Mutate) &[R]) {
+                    var ignore = &victim as &R?
+                    arr.append(<- copy1)
+                    destroy victim
+                } else {
+                    destroy victim // Never executed
+                }
+                
+                destroy arr
+            }
+        `)
+
+		errs := RequireCheckerErrors(t, err, 1)
+		// we'd like to only report one error here (i.e. in `copy2: <- victim`, not `&victim as &R?`)
+		assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
+	})
+}

runtime/tests/interpreter/resources_test.go

@@ -2736,7 +2736,7 @@ func TestInterpretMovedResourceInOptionalBinding(t *testing.T) {
 
 	t.Parallel()
 
-	inter, _, err := parseCheckAndInterpretWithLogs(t, `
+	inter, err := parseCheckAndInterpretWithOptions(t, `
         access(all) resource R{}
 
         access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
@@ -2755,8 +2755,14 @@ func TestInterpretMovedResourceInOptionalBinding(t *testing.T) {
             }
 
             destroy arr // This crashes
+			destroy victim
         }
-    `)
+    `, ParseCheckAndInterpretOptions{
+		HandleCheckerError: func(err error) {
+			errs := checker.RequireCheckerErrors(t, err, 1)
+			assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
+		},
+	})
 	require.NoError(t, err)
 
 	_, err = inter.Invoke("main")
@@ -2774,7 +2780,7 @@ func TestInterpretMovedResourceInSecondValue(t *testing.T) {
 
 	t.Parallel()
 
-	inter, _, err := parseCheckAndInterpretWithLogs(t, `
+	inter, err := parseCheckAndInterpretWithOptions(t, `
         access(all) resource R{}
 
         access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
@@ -2792,8 +2798,14 @@ func TestInterpretMovedResourceInSecondValue(t *testing.T) {
 
             destroy copy1
             destroy arr
+			destroy victim
         }
-    `)
+    `, ParseCheckAndInterpretOptions{
+		HandleCheckerError: func(err error) {
+			errs := checker.RequireCheckerErrors(t, err, 1)
+			assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
+		},
+	})
 	require.NoError(t, err)
 
 	_, err = inter.Invoke("main")