Skip to content

Commit a0dd6cf

Browse files
dsainati1dsainati1
authored
Merge pull request #3176 from onflow/sainati/second-value-transfer
Proper type checking for resource use after validation on two-value transfers
2 parents c574c0a + 9bff990 commit a0dd6cf

File tree

5 files changed

+213
-11
lines changed

5 files changed

+213
-11
lines changed

‎runtime/sema/check_variable_declaration.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,32 @@ func (checker *Checker) visitVariableDeclarationValues(declaration *ast.Variable
156156
)
157157
}
158158

159+
// Given a variable declaration with a second value transfer of the form
160+
// `let x <- e1 <- e2`
161+
//
162+
// the interpreter will evaluate this in the following order:
163+
// - evaluating `e1` to some value `v1` (which is necessarily either a variable, an index expression, or a member expression)
164+
// - moving the value stored in `v1` into the newly declared variable `x`
165+
// - evaluating `e2` to some value `v2`
166+
// - evaluate `e1` again to some value `v1'` (as `e1` may produce a different value after `e2`'s execution)
167+
// - lastly moving the value `v2` into the "variable" denoted by `v1'`
168+
//
169+
// This means that while at the end of the execution of the entire statement,
170+
// `v1` is still a valid resource (having had the result of `e2` moved into it),
171+
// specifically during the execution of `e2` it is invalid.
172+
// In order to reflect this, we temporarily invalidate `v1` before evaluating `e2` and checking the assignment,
173+
// and then re-validate `v1` afterwards.
174+
// We also re-check `e1` again as well, since it is evaluated a second time by the interpreter.
175+
// Note that while typechecking the same expression twice is not safe in general,
176+
// in this case it is permissible for a specific reason:
177+
// `e1` is necessarily an identifier, index or member expression, which may not have side effects
178+
179+
recordedResourceInvalidation := checker.recordResourceInvalidation(
180+
declaration.Value,
181+
declarationType,
182+
ResourceInvalidationKindMoveTemporary,
183+
)
184+
159185
// Check the assignment of the second value to the first expression
160186

161187
// The check of the assignment of the second value to the first also:
@@ -173,6 +199,11 @@ func (checker *Checker) visitVariableDeclarationValues(declaration *ast.Variable
173199
declaration.SecondTransfer,
174200
true,
175201
)
202+
203+
if recordedResourceInvalidation != nil {
204+
checker.resources.RemoveTemporaryMoveInvalidation(recordedResourceInvalidation.resource, recordedResourceInvalidation.invalidation)
205+
}
206+
checker.VisitExpression(declaration.Value, expectedValueType)
176207
}
177208
}
178209

‎runtime/sema/checker.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1556,19 +1556,27 @@ func (checker *Checker) recordResourceInvalidation(
15561556

15571557
accessedSelfMember := checker.accessedSelfMember(expression)
15581558

1559+
// Normally a field members cannot be invalidated,
1560+
// and this check typically prevents invalidations of this kind.
1561+
// However, during second value transfers we would like to be able to invalidate member and index
1562+
// expressions purely for the duration of checking the second value expression in the transfer.
1563+
// To enable this, nested move errors are not reported when the move kind of the invalidation is `Temporary`.
15591564
switch expression.(type) {
15601565
case *ast.MemberExpression:
15611566

1562-
if accessedSelfMember == nil ||
1563-
!checker.allowSelfResourceFieldInvalidation {
1567+
if (accessedSelfMember == nil ||
1568+
!checker.allowSelfResourceFieldInvalidation) &&
1569+
invalidationKind != ResourceInvalidationKindMoveTemporary {
15641570

15651571
reportInvalidNestedMove()
15661572
return nil
15671573
}
15681574

15691575
case *ast.IndexExpression:
1570-
reportInvalidNestedMove()
1571-
return nil
1576+
if invalidationKind != ResourceInvalidationKindMoveTemporary {
1577+
reportInvalidNestedMove()
1578+
return nil
1579+
}
15721580
}
15731581

15741582
invalidation := ResourceInvalidation{

‎runtime/tests/checker/access_test.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1849,7 +1849,7 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi
18491849
},
18501850
)
18511851

1852-
errs := RequireCheckerErrors(t, err, 5)
1852+
errs := RequireCheckerErrors(t, err, 7)
18531853

18541854
require.IsType(t, &sema.InvalidAccessError{}, errs[0])
18551855
assert.Equal(t,
@@ -1867,11 +1867,15 @@ func TestCheckAccessImportGlobalValueVariableDeclarationWithSecondValue(t *testi
18671867

18681868
require.IsType(t, &sema.ResourceCapturingError{}, errs[3])
18691869

1870-
require.IsType(t, &sema.AssignmentToConstantError{}, errs[4])
1870+
require.IsType(t, &sema.ResourceCapturingError{}, errs[4])
1871+
1872+
require.IsType(t, &sema.AssignmentToConstantError{}, errs[5])
18711873
assert.Equal(t,
18721874
"y",
1873-
errs[4].(*sema.AssignmentToConstantError).Name,
1875+
errs[5].(*sema.AssignmentToConstantError).Name,
18741876
)
1877+
1878+
require.IsType(t, &sema.ResourceCapturingError{}, errs[6])
18751879
}
18761880

18771881
func TestCheckContractNestedDeclarationPrivateAccess(t *testing.T) {

‎runtime/tests/checker/resources_test.go‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9859,3 +9859,150 @@ func TestCheckOptionalBindingElseBranch(t *testing.T) {
98599859
errs := RequireCheckerErrors(t, err, 1)
98609860
assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
98619861
}
9862+
9863+
func TestCheckResourceSecondValueTransfer(t *testing.T) {
9864+
9865+
t.Parallel()
9866+
9867+
t.Run("basic array invalid", func(t *testing.T) {
9868+
t.Parallel()
9869+
9870+
_, err := ParseAndCheck(t, `
9871+
resource R {}
9872+
9873+
access(all) fun main() {
9874+
let vaults: @[AnyResource] <- [<-[]]
9875+
let old <- vaults[0] <- vaults
9876+
destroy old
9877+
}
9878+
`)
9879+
9880+
errs := RequireCheckerErrors(t, err, 1)
9881+
assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
9882+
})
9883+
9884+
t.Run("basic array", func(t *testing.T) {
9885+
t.Parallel()
9886+
9887+
_, err := ParseAndCheck(t, `
9888+
resource R {}
9889+
9890+
access(all) fun main() {
9891+
let vaults: @[AnyResource] <- [<-[]]
9892+
let bar <- create R()
9893+
let old <- vaults[0] <- bar
9894+
destroy old
9895+
destroy vaults
9896+
}
9897+
`)
9898+
9899+
require.NoError(t, err)
9900+
})
9901+
9902+
t.Run("basic function call invalid", func(t *testing.T) {
9903+
t.Parallel()
9904+
9905+
_, err := ParseAndCheck(t, `
9906+
resource R {}
9907+
9908+
access(all) fun foo(_ r: @R): @R {
9909+
return <-r
9910+
}
9911+
9912+
access(all) fun main() {
9913+
var r <- create R()
9914+
let r2 <- r <- foo(<-r)
9915+
destroy r2
9916+
// note that r is still "valid" after the two value transfer so we must destroy it
9917+
destroy r
9918+
}
9919+
`)
9920+
9921+
errs := RequireCheckerErrors(t, err, 1)
9922+
assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
9923+
})
9924+
9925+
t.Run("basic function call valid", func(t *testing.T) {
9926+
t.Parallel()
9927+
9928+
_, err := ParseAndCheck(t, `
9929+
resource R {}
9930+
9931+
access(all) fun foo(_ r: @R): @R {
9932+
return <-r
9933+
}
9934+
9935+
access(all) fun main() {
9936+
var r <- create R()
9937+
var bar <- create R()
9938+
let r2 <- r <- foo(<-bar)
9939+
destroy r2
9940+
destroy r
9941+
}
9942+
`)
9943+
9944+
require.NoError(t, err)
9945+
})
9946+
9947+
t.Run("invalid", func(t *testing.T) {
9948+
t.Parallel()
9949+
9950+
_, err := ParseAndCheck(t, `
9951+
access(all) resource R{}
9952+
9953+
access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
9954+
arrRef.append(<- copy2!)
9955+
return <- create R()
9956+
}
9957+
9958+
access(all) fun main() {
9959+
var victim: @R? <- create R()
9960+
var arr: @[R] <- []
9961+
9962+
// In the optional binding below, the 'victim' must be invalidated
9963+
// before evaluation of the collect() call
9964+
let copy1 <- victim <- collect(copy2: <- victim, &arr as auth(Mutate) &[R])
9965+
9966+
// Panics when trying to destroy
9967+
destroy copy1
9968+
destroy arr
9969+
destroy victim
9970+
}
9971+
`)
9972+
9973+
errs := RequireCheckerErrors(t, err, 1)
9974+
assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
9975+
})
9976+
9977+
t.Run("regression", func(t *testing.T) {
9978+
t.Parallel()
9979+
9980+
_, err := ParseAndCheck(t, `
9981+
access(all) resource R{}
9982+
9983+
access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
9984+
arrRef.append(<- copy2!)
9985+
return <- create R()
9986+
}
9987+
9988+
access(all) fun main() {
9989+
var victim: @R? <- create R()
9990+
var arr: @[R] <- []
9991+
9992+
if let copy1 <- victim <- collect(copy2: <- victim, &arr as auth(Mutate) &[R]) {
9993+
var ignore = &victim as &R?
9994+
arr.append(<- copy1)
9995+
destroy victim
9996+
} else {
9997+
destroy victim // Never executed
9998+
}
9999+
10000+
destroy arr
10001+
}
10002+
`)
10003+
10004+
errs := RequireCheckerErrors(t, err, 1)
10005+
// we'd like to only report one error here (i.e. in `copy2: <- victim`, not `&victim as &R?`)
10006+
assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
10007+
})
10008+
}

‎runtime/tests/interpreter/resources_test.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2736,7 +2736,7 @@ func TestInterpretMovedResourceInOptionalBinding(t *testing.T) {
27362736

27372737
t.Parallel()
27382738

2739-
inter, _, err := parseCheckAndInterpretWithLogs(t, `
2739+
inter, err := parseCheckAndInterpretWithOptions(t, `
27402740
access(all) resource R{}
27412741
27422742
access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
@@ -2755,8 +2755,14 @@ func TestInterpretMovedResourceInOptionalBinding(t *testing.T) {
27552755
}
27562756
27572757
destroy arr // This crashes
2758+
destroy victim
27582759
}
2759-
`)
2760+
`, ParseCheckAndInterpretOptions{
2761+
HandleCheckerError: func(err error) {
2762+
errs := checker.RequireCheckerErrors(t, err, 1)
2763+
assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
2764+
},
2765+
})
27602766
require.NoError(t, err)
27612767

27622768
_, err = inter.Invoke("main")
@@ -2774,7 +2780,7 @@ func TestInterpretMovedResourceInSecondValue(t *testing.T) {
27742780

27752781
t.Parallel()
27762782

2777-
inter, _, err := parseCheckAndInterpretWithLogs(t, `
2783+
inter, err := parseCheckAndInterpretWithOptions(t, `
27782784
access(all) resource R{}
27792785
27802786
access(all) fun collect(copy2: @R?, _ arrRef: auth(Mutate) &[R]): @R {
@@ -2792,8 +2798,14 @@ func TestInterpretMovedResourceInSecondValue(t *testing.T) {
27922798
27932799
destroy copy1
27942800
destroy arr
2801+
destroy victim
27952802
}
2796-
`)
2803+
`, ParseCheckAndInterpretOptions{
2804+
HandleCheckerError: func(err error) {
2805+
errs := checker.RequireCheckerErrors(t, err, 1)
2806+
assert.IsType(t, &sema.ResourceUseAfterInvalidationError{}, errs[0])
2807+
},
2808+
})
27972809
require.NoError(t, err)
27982810

27992811
_, err = inter.Invoke("main")

0 commit comments

Comments
 (0)