From 1dbd11497b459cfb2dd2a8f73e9c914bfc840fa4 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Mon, 22 Nov 2021 15:50:16 -0500 Subject: [PATCH 01/26] demonstration of external mutation error --- runtime/sema/check_assignment.go | 16 ++++++++++++++ runtime/sema/check_member_expression.go | 7 +++++++ runtime/sema/errors.go | 28 +++++++++++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/runtime/sema/check_assignment.go b/runtime/sema/check_assignment.go index 6058bccb4d..05810154b1 100644 --- a/runtime/sema/check_assignment.go +++ b/runtime/sema/check_assignment.go @@ -212,6 +212,22 @@ func (checker *Checker) visitIndexExpressionAssignment( elementType = checker.visitIndexExpression(target, true) + targetExpression := target.TargetExpression + switch targetExpression := targetExpression.(type) { + case *ast.MemberExpression: + _, member, _ := checker.visitMember(targetExpression) + if !checker.isMutatableMember(member) { + checker.report( + &ExternalMutationError{ + Name: member.Identifier.Identifier, + DeclarationKind: member.DeclarationKind, + Range: ast.NewRangeFromPositioned(targetExpression.Identifier), + EnclosingType: member.ContainerType, + }, + ) + } + } + if elementType == nil { return InvalidType } diff --git a/runtime/sema/check_member_expression.go b/runtime/sema/check_member_expression.go index 3140792533..233ef43799 100644 --- a/runtime/sema/check_member_expression.go +++ b/runtime/sema/check_member_expression.go @@ -329,6 +329,13 @@ func (checker *Checker) isWriteableMember(member *Member) bool { checker.containerTypes[member.ContainerType] } +// isMutatableMember returns true if the given member can be mutated +// in the current location of the checker +// +func (checker *Checker) isMutatableMember(member *Member) bool { + return checker.containerTypes[member.ContainerType] +} + // containingContractKindedType returns the containing contract-kinded type // of the given type, if any. // diff --git a/runtime/sema/errors.go b/runtime/sema/errors.go index 200c583243..f29cd07afb 100644 --- a/runtime/sema/errors.go +++ b/runtime/sema/errors.go @@ -2966,3 +2966,31 @@ func (e *InvalidEntryPointTypeError) Error() string { e.Type.QualifiedString(), ) } + +// ImportedProgramError + +type ExternalMutationError struct { + Name string + EnclosingType Type + DeclarationKind common.DeclarationKind + ast.Range +} + +func (e *ExternalMutationError) Error() string { + return fmt.Sprintf( + "cannot mutate `%s`: %s was defined inside `%s`", + e.Name, + e.DeclarationKind.Name(), + e.EnclosingType.QualifiedString(), + ) +} + +func (e *ExternalMutationError) SecondaryError() string { + return fmt.Sprintf( + "Consider adding a setter for `%s` to `%s`", + e.Name, + e.EnclosingType.QualifiedString(), + ) +} + +func (*ExternalMutationError) isSemanticError() {} From e30e92256c2c153a6fde9aa8fd10545f90fb74f1 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Mon, 22 Nov 2021 16:04:04 -0500 Subject: [PATCH 02/26] add tests --- .../tests/checker/external_mutation_test.go | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 runtime/tests/checker/external_mutation_test.go diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go new file mode 100644 index 0000000000..f58a1e944e --- /dev/null +++ b/runtime/tests/checker/external_mutation_test.go @@ -0,0 +1,269 @@ +/* + * Cadence - The resource-oriented smart contract programming language + * + * Copyright 2019-2021 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 ( + "fmt" + "testing" + + "github.com/onflow/cadence/runtime/sema" + "github.com/stretchr/testify/require" +) + +func TestArrayUpdate(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + } + + declarationKinds := []string{ + "let", + "var", + } + + valueKinds := []string{ + "struct", + "resource", + } + + runTest := func(access string, declaration string, valueKind string) { + testName := fmt.Sprintf("%s %s %s", access, valueKind, declaration) + + assignmentOp := "=" + var destroyStatement string + if valueKind == "resource" { + assignmentOp = "<- create" + destroyStatement = "destroy foo" + } + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub %s Foo { + %s %s x : [Int] + + init() { + self.x = [3]; + } + } + + pub fun bar() { + let foo %s Foo() + foo.x[0] = 3 + %s + } + `, valueKind, access, declaration, assignmentOp, destroyStatement), + ParseAndCheckOptions{}, + ) + + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + for _, value := range valueKinds { + runTest(access, kind, value) + } + } + } + +} + +func TestDictionaryUpdate(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + } + + declarationKinds := []string{ + "let", + "var", + } + + valueKinds := []string{ + "struct", + "resource", + } + + runTest := func(access string, declaration string, valueKind string) { + testName := fmt.Sprintf("%s %s %s", access, valueKind, declaration) + + assignmentOp := "=" + var destroyStatement string + if valueKind == "resource" { + assignmentOp = "<- create" + destroyStatement = "destroy foo" + } + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub %s Foo { + %s %s x : {Int: Int} + + init() { + self.x = {0:3}; + } + } + + pub fun bar() { + let foo %s Foo() + foo.x[0] = 3 + %s + } + `, valueKind, access, declaration, assignmentOp, destroyStatement), + ParseAndCheckOptions{}, + ) + + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + for _, value := range valueKinds { + runTest(access, kind, value) + } + } + } + +} + +func TestNestedArrayUpdate(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + } + + declarationKinds := []string{ + "let", + "var", + } + + runTest := func(access string, declaration string) { + testName := fmt.Sprintf("%s struct %s", access, declaration) + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } + } + + pub struct Foo { + %s %s x : [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } + `, access, declaration), + ParseAndCheckOptions{}, + ) + + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + runTest(access, kind) + } + } +} + +func TestNestedDictionaryUpdate(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + } + + declarationKinds := []string{ + "let", + "var", + } + + runTest := func(access string, declaration string) { + testName := fmt.Sprintf("%s struct %s", access, declaration) + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } + } + + pub struct Foo { + %s %s x : {Int: Int} + + init() { + self.x = {3:3} + } + } + + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } + `, access, declaration), + ParseAndCheckOptions{}, + ) + + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + runTest(access, kind) + } + } +} From 02736cc8ad3f6dc178075fe29e560476254c1211 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 23 Nov 2021 11:43:20 -0500 Subject: [PATCH 03/26] improve tests to include acceess(contract) --- .../tests/checker/external_mutation_test.go | 108 ++++++++++-------- 1 file changed, 60 insertions(+), 48 deletions(-) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index f58a1e944e..478cc9a9df 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -33,6 +33,7 @@ func TestArrayUpdate(t *testing.T) { accessModifiers := []string{ "pub", "access(account)", + "access(contract)", } declarationKinds := []string{ @@ -58,18 +59,20 @@ func TestArrayUpdate(t *testing.T) { t.Run(testName, func(t *testing.T) { _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` - pub %s Foo { - %s %s x : [Int] - - init() { - self.x = [3]; + pub contract C { + pub %s Foo { + %s %s x : [Int] + + init() { + self.x = [3]; + } } - } - pub fun bar() { - let foo %s Foo() - foo.x[0] = 3 - %s + pub fun bar() { + let foo %s Foo() + foo.x[0] = 3 + %s + } } `, valueKind, access, declaration, assignmentOp, destroyStatement), ParseAndCheckOptions{}, @@ -98,6 +101,7 @@ func TestDictionaryUpdate(t *testing.T) { accessModifiers := []string{ "pub", "access(account)", + "access(contract)", } declarationKinds := []string{ @@ -123,18 +127,20 @@ func TestDictionaryUpdate(t *testing.T) { t.Run(testName, func(t *testing.T) { _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` - pub %s Foo { - %s %s x : {Int: Int} - - init() { - self.x = {0:3}; + pub contract C { + pub %s Foo { + %s %s x : {Int: Int} + + init() { + self.x = {0:3}; + } } - } - pub fun bar() { - let foo %s Foo() - foo.x[0] = 3 - %s + pub fun bar() { + let foo %s Foo() + foo.x[0] = 3 + %s + } } `, valueKind, access, declaration, assignmentOp, destroyStatement), ParseAndCheckOptions{}, @@ -163,6 +169,7 @@ func TestNestedArrayUpdate(t *testing.T) { accessModifiers := []string{ "pub", "access(account)", + "access(contract)", } declarationKinds := []string{ @@ -176,24 +183,26 @@ func TestNestedArrayUpdate(t *testing.T) { t.Run(testName, func(t *testing.T) { _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` - pub struct Bar { - pub let foo: Foo - init() { - self.foo = Foo() + pub contract C { + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } } - } - pub struct Foo { - %s %s x : [Int] - - init() { - self.x = [3] + pub struct Foo { + %s %s x : [Int] + + init() { + self.x = [3] + } } - } - pub fun bar() { - let bar = Bar() - bar.foo.x[0] = 3 + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } } `, access, declaration), ParseAndCheckOptions{}, @@ -219,6 +228,7 @@ func TestNestedDictionaryUpdate(t *testing.T) { accessModifiers := []string{ "pub", "access(account)", + "access(contract)", } declarationKinds := []string{ @@ -232,24 +242,26 @@ func TestNestedDictionaryUpdate(t *testing.T) { t.Run(testName, func(t *testing.T) { _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` - pub struct Bar { - pub let foo: Foo - init() { - self.foo = Foo() + pub contract C { + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } } - } - pub struct Foo { - %s %s x : {Int: Int} - - init() { - self.x = {3:3} + pub struct Foo { + %s %s x : {Int: Int} + + init() { + self.x = {3:3} + } } - } - pub fun bar() { - let bar = Bar() - bar.foo.x[0] = 3 + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } } `, access, declaration), ParseAndCheckOptions{}, From 4680fd9e33f295b2707da0b1c84b0a49898d80ea Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 23 Nov 2021 11:48:13 -0500 Subject: [PATCH 04/26] add contract test --- .../tests/checker/external_mutation_test.go | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index 478cc9a9df..30144cd661 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -279,3 +279,61 @@ func TestNestedDictionaryUpdate(t *testing.T) { } } } + +func TestMutateContract(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + "access(contract)", + } + + declarationKinds := []string{ + "let", + "var", + } + + runTest := func(access string, declaration string) { + testName := fmt.Sprintf("%s struct %s", access, declaration) + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub contract Foo { + %s %s x : [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + Foo.x[0] = 1 + } + `, access, declaration), + ParseAndCheckOptions{}, + ) + + expectedErrors := 1 + if access == "access(contract)" { + expectedErrors++ + } + + errs := ExpectCheckerErrors(t, err, expectedErrors) + if expectedErrors > 1 { + var accessError *sema.InvalidAccessError + require.ErrorAs(t, errs[expectedErrors-2], &accessError) + } + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[expectedErrors-1], &externalMutationError) + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + runTest(access, kind) + } + } +} From bc66e505af34252fc016e39be1f3d8c0ee080a4f Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 23 Nov 2021 11:53:02 -0500 Subject: [PATCH 05/26] test for mutating struct field within defining contract --- .../tests/checker/external_mutation_test.go | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index 30144cd661..d735656b2b 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -337,3 +337,121 @@ func TestMutateContract(t *testing.T) { } } } + +func TestContractNestedStruct(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + "access(contract)", + } + + declarationKinds := []string{ + "let", + "var", + } + + runTest := func(access string, declaration string) { + testName := fmt.Sprintf("%s struct %s", access, declaration) + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub contract Foo { + pub let x : S + + pub struct S { + %s %s y : [Int] + init() { + self.y = [3] + } + } + + init() { + self.x = S() + } + } + + pub fun bar() { + Foo.x.y[0] = 1 + } + `, access, declaration), + ParseAndCheckOptions{}, + ) + + expectedErrors := 1 + if access == "access(contract)" { + expectedErrors++ + } + + errs := ExpectCheckerErrors(t, err, expectedErrors) + if expectedErrors > 1 { + var accessError *sema.InvalidAccessError + require.ErrorAs(t, errs[expectedErrors-2], &accessError) + } + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[expectedErrors-1], &externalMutationError) + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + runTest(access, kind) + } + } +} + +func TestContractStructInit(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + "access(contract)", + } + + declarationKinds := []string{ + "let", + "var", + } + + runTest := func(access string, declaration string) { + testName := fmt.Sprintf("%s struct %s", access, declaration) + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub contract Foo { + pub let x : S + + pub struct S { + %s %s y : [Int] + init() { + self.y = [3] + } + } + + init() { + self.x = S() + self.x.y[1] = 2 + } + } + `, access, declaration), + ParseAndCheckOptions{}, + ) + + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + runTest(access, kind) + } + } +} From 8cf2b38597e07e9a19fe6cfa99a6a79647274bfb Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 23 Nov 2021 12:04:37 -0500 Subject: [PATCH 06/26] rename tests --- runtime/tests/checker/external_mutation_test.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index d735656b2b..86da806c8e 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -26,7 +26,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestArrayUpdate(t *testing.T) { +func TestArrayUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -91,10 +91,9 @@ func TestArrayUpdate(t *testing.T) { } } } - } -func TestDictionaryUpdate(t *testing.T) { +func TestDictionaryUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -159,10 +158,9 @@ func TestDictionaryUpdate(t *testing.T) { } } } - } -func TestNestedArrayUpdate(t *testing.T) { +func TestNestedArrayUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -221,7 +219,7 @@ func TestNestedArrayUpdate(t *testing.T) { } } -func TestNestedDictionaryUpdate(t *testing.T) { +func TestNestedDictionaryUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -280,7 +278,7 @@ func TestNestedDictionaryUpdate(t *testing.T) { } } -func TestMutateContract(t *testing.T) { +func TestMutateContractIndexAccess(t *testing.T) { t.Parallel() @@ -338,7 +336,7 @@ func TestMutateContract(t *testing.T) { } } -func TestContractNestedStruct(t *testing.T) { +func TestContractNestedStructIndexAccess(t *testing.T) { t.Parallel() @@ -403,7 +401,7 @@ func TestContractNestedStruct(t *testing.T) { } } -func TestContractStructInit(t *testing.T) { +func TestContractStructInitIndexAccess(t *testing.T) { t.Parallel() From 348d3473c167b76638bfb5e8a6d708e04e769eb0 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 23 Nov 2021 12:47:56 -0500 Subject: [PATCH 07/26] add external mutation error when calling mutative methods on arrays and dictionaries --- runtime/sema/check_member_expression.go | 14 +++ runtime/sema/type.go | 29 ++++-- .../tests/checker/external_mutation_test.go | 91 +++++++++++++++++++ 3 files changed, 124 insertions(+), 10 deletions(-) diff --git a/runtime/sema/check_member_expression.go b/runtime/sema/check_member_expression.go index 233ef43799..af73ebf402 100644 --- a/runtime/sema/check_member_expression.go +++ b/runtime/sema/check_member_expression.go @@ -180,6 +180,20 @@ func (checker *Checker) visitMember(expression *ast.MemberExpression) (accessedT } targetRange := ast.NewRangeFromPositioned(expression.Expression) member = resolver.Resolve(identifier, targetRange, checker.report) + switch targetExpression := accessedExpression.(type) { + case *ast.MemberExpression: + _, m, _ := checker.visitMember(targetExpression) + if !checker.isMutatableMember(m) && resolver.Mutating { + checker.report( + &ExternalMutationError{ + Name: m.Identifier.Identifier, + DeclarationKind: m.DeclarationKind, + Range: ast.NewRangeFromPositioned(targetRange), + EnclosingType: m.ContainerType, + }, + ) + } + } } // Get the member from the accessed value based diff --git a/runtime/sema/type.go b/runtime/sema/type.go index 15f449d95a..1b165545b0 100644 --- a/runtime/sema/type.go +++ b/runtime/sema/type.go @@ -182,8 +182,9 @@ type ValueIndexableType interface { } type MemberResolver struct { - Kind common.DeclarationKind - Resolve func(identifier string, targetRange ast.Range, report func(error)) *Member + Kind common.DeclarationKind + Mutating bool + Resolve func(identifier string, targetRange ast.Range, report func(error)) *Member } // ContainedType is a type which might have a container type @@ -1620,7 +1621,8 @@ func getArrayMembers(arrayType ArrayType) map[string]MemberResolver { if _, ok := arrayType.(*VariableSizedType); ok { members["append"] = MemberResolver{ - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, targetRange ast.Range, report func(error)) *Member { elementType := arrayType.ElementType(false) return NewPublicFunctionMember( @@ -1633,7 +1635,8 @@ func getArrayMembers(arrayType ArrayType) map[string]MemberResolver { } members["appendAll"] = MemberResolver{ - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, targetRange ast.Range, report func(error)) *Member { elementType := arrayType.ElementType(false) @@ -1685,7 +1688,8 @@ func getArrayMembers(arrayType ArrayType) map[string]MemberResolver { } members["insert"] = MemberResolver{ - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, _ ast.Range, _ func(error)) *Member { elementType := arrayType.ElementType(false) @@ -1700,7 +1704,8 @@ func getArrayMembers(arrayType ArrayType) map[string]MemberResolver { } members["remove"] = MemberResolver{ - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, _ ast.Range, _ func(error)) *Member { elementType := arrayType.ElementType(false) @@ -1715,7 +1720,8 @@ func getArrayMembers(arrayType ArrayType) map[string]MemberResolver { } members["removeFirst"] = MemberResolver{ - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, _ ast.Range, _ func(error)) *Member { elementType := arrayType.ElementType(false) @@ -1731,7 +1737,8 @@ func getArrayMembers(arrayType ArrayType) map[string]MemberResolver { } members["removeLast"] = MemberResolver{ - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, _ ast.Range, _ func(error)) *Member { elementType := arrayType.ElementType(false) @@ -4324,7 +4331,8 @@ func (t *DictionaryType) initializeMemberResolvers() { }, }, "insert": { - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, _ ast.Range, _ func(error)) *Member { return NewPublicFunctionMember(t, identifier, @@ -4334,7 +4342,8 @@ func (t *DictionaryType) initializeMemberResolvers() { }, }, "remove": { - Kind: common.DeclarationKindFunction, + Kind: common.DeclarationKindFunction, + Mutating: true, Resolve: func(identifier string, _ ast.Range, _ func(error)) *Member { return NewPublicFunctionMember(t, identifier, diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index 86da806c8e..fdaa0bf7af 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -453,3 +453,94 @@ func TestContractStructInitIndexAccess(t *testing.T) { } } } + +func TestArrayUpdateMethodCall(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + "access(contract)", + } + + declarationKinds := []string{ + "let", + "var", + } + + valueKinds := []string{ + "struct", + "resource", + } + + type MethodCall = struct { + Mutating bool + Code string + Name string + } + + memberExpressions := []MethodCall{ + {Mutating: true, Code: ".append(3)", Name: "append"}, + {Mutating: false, Code: ".length", Name: "length"}, + {Mutating: false, Code: ".concat([3])", Name: "concat"}, + {Mutating: false, Code: ".contains(3)", Name: "contains"}, + {Mutating: true, Code: ".appendAll([3])", Name: "appendAll"}, + {Mutating: true, Code: ".insert(at: 0, 3)", Name: "insert"}, + {Mutating: true, Code: ".remove(at: 0)", Name: "remove"}, + {Mutating: true, Code: ".removeFirst()", Name: "removeFirst"}, + {Mutating: true, Code: ".removeLast()", Name: "removeLast"}, + } + + runTest := func(access string, declaration string, valueKind string, member MethodCall) { + testName := fmt.Sprintf("%s %s %s %s", access, valueKind, declaration, member.Name) + + assignmentOp := "=" + var destroyStatement string + if valueKind == "resource" { + assignmentOp = "<- create" + destroyStatement = "destroy foo" + } + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub contract C { + pub %s Foo { + %s %s x : [Int] + + init() { + self.x = [3]; + } + } + + pub fun bar() { + let foo %s Foo() + foo.x%s + %s + } + } + `, valueKind, access, declaration, assignmentOp, member.Code, destroyStatement), + ParseAndCheckOptions{}, + ) + + if member.Mutating { + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + } else { + require.NoError(t, err) + } + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + for _, value := range valueKinds { + for _, member := range memberExpressions { + runTest(access, kind, value, member) + } + } + } + } +} From 7d09f1e3d5d69bd137869ffb77a56a1a126e0887 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 23 Nov 2021 14:39:59 -0500 Subject: [PATCH 08/26] pub(set) access mode allows mutation of fields anywhere --- runtime/account_test.go | 89 ++++++++--- runtime/resourcedictionary_test.go | 8 +- runtime/runtime_test.go | 2 +- runtime/sema/check_member_expression.go | 3 +- .../tests/checker/external_mutation_test.go | 150 +++++++++++++++++- runtime/tests/interpreter/resources_test.go | 2 +- 6 files changed, 222 insertions(+), 32 deletions(-) diff --git a/runtime/account_test.go b/runtime/account_test.go index 69bbb028fe..ff02178b95 100644 --- a/runtime/account_test.go +++ b/runtime/account_test.go @@ -1289,24 +1289,16 @@ func TestRuntimePublicKey(t *testing.T) { storage: storage, } - value, err := executeScript(script, runtimeInterface) - require.NoError(t, err) - - expected := cadence.Struct{ - StructType: PublicKeyType, - Fields: []cadence.Value{ - // Public key (bytes) - newBytesValue([]byte{1, 2}), + _, err := executeScript(script, runtimeInterface) - // Signature Algo - newSignAlgoValue(sema.SignatureAlgorithmECDSA_P256), + require.Error(t, err) - // valid - cadence.Bool(false), - }, - } + var checkerErr *sema.CheckerError + require.ErrorAs(t, err, &checkerErr) + errs := checkerErr.Errors + require.Len(t, errs, 1) - assert.Equal(t, expected, value) + assert.IsType(t, &sema.ExternalMutationError{}, errs[0]) }) } @@ -1368,7 +1360,6 @@ func TestAuthAccountContracts(t *testing.T) { transaction { prepare(signer: AuthAccount) { signer.contracts.names[0] = "baz" - assert(signer.contracts.names[0] == "foo") } } `) @@ -1393,8 +1384,14 @@ func TestAuthAccountContracts(t *testing.T) { Location: nextTransactionLocation(), }, ) + require.Error(t, err) - require.NoError(t, err) + var checkerErr *sema.CheckerError + require.ErrorAs(t, err, &checkerErr) + errs := checkerErr.Errors + require.Len(t, errs, 1) + + assert.IsType(t, &sema.ExternalMutationError{}, errs[0]) }) } @@ -1571,7 +1568,7 @@ func TestPublicAccountContracts(t *testing.T) { nextTransactionLocation := newTransactionLocationGenerator() - result, err := rt.ExecuteScript( + _, err := rt.ExecuteScript( Script{ Source: script, }, @@ -1581,14 +1578,58 @@ func TestPublicAccountContracts(t *testing.T) { }, ) - require.NoError(t, err) + require.Error(t, err) - require.IsType(t, cadence.Array{}, result) - array := result.(cadence.Array) + var checkerErr *sema.CheckerError + require.ErrorAs(t, err, &checkerErr) + errs := checkerErr.Errors + require.Len(t, errs, 1) - require.Len(t, array.Values, 2) - assert.Equal(t, cadence.String("foo"), array.Values[0]) - assert.Equal(t, cadence.String("bar"), array.Values[1]) + assert.IsType(t, &sema.ExternalMutationError{}, errs[0]) + }) + + t.Run("append names", func(t *testing.T) { + t.Parallel() + + rt := newTestInterpreterRuntime() + + script := []byte(` + pub fun main(): [String] { + let acc = getAccount(0x02) + acc.contracts.names.append("baz") + return acc.contracts.names + } + `) + + runtimeInterface := &testRuntimeInterface{ + getSigningAccounts: func() ([]Address, error) { + return []Address{{42}}, nil + }, + getAccountContractNames: func(_ Address) ([]string, error) { + return []string{"foo", "bar"}, nil + }, + } + + nextTransactionLocation := newTransactionLocationGenerator() + + _, err := rt.ExecuteScript( + Script{ + Source: script, + }, + Context{ + Interface: runtimeInterface, + Location: nextTransactionLocation(), + }, + ) + + require.Error(t, err) + + var checkerErr *sema.CheckerError + require.ErrorAs(t, err, &checkerErr) + errs := checkerErr.Errors + require.Len(t, errs, 1) + + assert.IsType(t, &sema.ExternalMutationError{}, errs[0]) }) } diff --git a/runtime/resourcedictionary_test.go b/runtime/resourcedictionary_test.go index 2efcaffdfe..5873c3e2d2 100644 --- a/runtime/resourcedictionary_test.go +++ b/runtime/resourcedictionary_test.go @@ -58,7 +58,7 @@ const resourceDictionaryContract = ` pub resource C { - pub let rs: @{String: R} + pub(set) var rs: @{String: R} init() { self.rs <- {} @@ -410,7 +410,7 @@ func TestRuntimeResourceDictionaryValues_Nested(t *testing.T) { pub resource C2 { - pub let rs: @{String: R} + pub(set) var rs: @{String: R} init() { self.rs <- {} @@ -431,7 +431,7 @@ func TestRuntimeResourceDictionaryValues_Nested(t *testing.T) { pub resource C { - pub let c2s: @{String: C2} + pub(set) var c2s: @{String: C2} init() { self.c2s <- {} @@ -602,7 +602,7 @@ func TestRuntimeResourceDictionaryValues_DictionaryTransfer(t *testing.T) { pub resource C { - pub let rs: @{String: R} + pub(set) var rs: @{String: R} init() { self.rs <- {} diff --git a/runtime/runtime_test.go b/runtime/runtime_test.go index 061fa9e19c..9b3aa4a1e6 100644 --- a/runtime/runtime_test.go +++ b/runtime/runtime_test.go @@ -1686,7 +1686,7 @@ func TestRuntimeStorageMultipleTransactionsResourceWithArray(t *testing.T) { container := []byte(` pub resource Container { - pub let values: [Int] + pub(set) var values: [Int] init() { self.values = [] diff --git a/runtime/sema/check_member_expression.go b/runtime/sema/check_member_expression.go index af73ebf402..b4d90b0fba 100644 --- a/runtime/sema/check_member_expression.go +++ b/runtime/sema/check_member_expression.go @@ -347,7 +347,8 @@ func (checker *Checker) isWriteableMember(member *Member) bool { // in the current location of the checker // func (checker *Checker) isMutatableMember(member *Member) bool { - return checker.containerTypes[member.ContainerType] + return checker.isWriteableAccess(member.Access) || + checker.containerTypes[member.ContainerType] } // containingContractKindedType returns the containing contract-kinded type diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index fdaa0bf7af..7598b00090 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -22,8 +22,9 @@ import ( "fmt" "testing" - "github.com/onflow/cadence/runtime/sema" "github.com/stretchr/testify/require" + + "github.com/onflow/cadence/runtime/sema" ) func TestArrayUpdateIndexAccess(t *testing.T) { @@ -544,3 +545,150 @@ func TestArrayUpdateMethodCall(t *testing.T) { } } } + +func TestDictionaryUpdateMethodCall(t *testing.T) { + + t.Parallel() + + accessModifiers := []string{ + "pub", + "access(account)", + "access(contract)", + } + + declarationKinds := []string{ + "let", + "var", + } + + valueKinds := []string{ + "struct", + "resource", + } + + type MethodCall = struct { + Mutating bool + Code string + Name string + } + + memberExpressions := []MethodCall{ + {Mutating: true, Code: ".insert(key:3, 3)", Name: "insert"}, + {Mutating: false, Code: ".length", Name: "length"}, + {Mutating: false, Code: ".keys", Name: "keys"}, + {Mutating: false, Code: ".values", Name: "values"}, + {Mutating: false, Code: ".containsKey(3)", Name: "containsKey"}, + {Mutating: true, Code: ".remove(key: 0)", Name: "remove"}, + } + + runTest := func(access string, declaration string, valueKind string, member MethodCall) { + testName := fmt.Sprintf("%s %s %s %s", access, valueKind, declaration, member.Name) + + assignmentOp := "=" + var destroyStatement string + if valueKind == "resource" { + assignmentOp = "<- create" + destroyStatement = "destroy foo" + } + + t.Run(testName, func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + fmt.Sprintf(` + pub contract C { + pub %s Foo { + %s %s x : {Int: Int} + + init() { + self.x = {3:3}; + } + } + + pub fun bar() { + let foo %s Foo() + foo.x%s + %s + } + } + `, valueKind, access, declaration, assignmentOp, member.Code, destroyStatement), + ParseAndCheckOptions{}, + ) + + if member.Mutating { + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + } else { + require.NoError(t, err) + } + }) + } + + for _, access := range accessModifiers { + for _, kind := range declarationKinds { + for _, value := range valueKinds { + for _, member := range memberExpressions { + runTest(access, kind, value, member) + } + } + } + } +} + +func TestPubSetAccessModifier(t *testing.T) { + t.Run("pub set dict", func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + ` + pub contract C { + pub struct Foo { + pub(set) var x: {Int: Int} + + init() { + self.x = {3:3}; + } + } + + pub fun bar() { + let foo = Foo() + foo.x[0] = 3 + } + } + `, + ParseAndCheckOptions{}, + ) + require.NoError(t, err) + + }) +} + +func TestPubSetNestedAccessModifier(t *testing.T) { + t.Run("pub set nested", func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + ` + pub contract C { + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } + } + + pub struct Foo { + pub(set) var x : [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } + } + `, + ParseAndCheckOptions{}, + ) + require.NoError(t, err) + + }) +} diff --git a/runtime/tests/interpreter/resources_test.go b/runtime/tests/interpreter/resources_test.go index 7b49097dfd..b695dafff9 100644 --- a/runtime/tests/interpreter/resources_test.go +++ b/runtime/tests/interpreter/resources_test.go @@ -395,7 +395,7 @@ func TestInterpretImplicitResourceRemovalFromContainer(t *testing.T) { } resource R1 { - var r2s: @{Int: R2} + pub(set) var r2s: @{Int: R2} init() { self.r2s <- {} From f9c6c3c3bc73e213e2b1caefb0dcd895740b2a39 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Wed, 24 Nov 2021 11:25:27 -0500 Subject: [PATCH 09/26] add test for structure created in its own scope --- .../tests/checker/external_mutation_test.go | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index 7598b00090..554d86089c 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -692,3 +692,29 @@ func TestPubSetNestedAccessModifier(t *testing.T) { }) } + +func TestSelfContainingStruct(t *testing.T) { + t.Run("pub set dict", func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + ` + pub contract C { + pub struct Foo { + pub let x: {Int: Int} + + init() { + self.x = {3:3}; + } + + pub fun bar() { + let foo = Foo() + foo.x[0] = 3 + } + } + } + `, + ParseAndCheckOptions{}, + ) + require.NoError(t, err) + + }) +} From c4b0ef3ff5ff1030acf9dcfdc7b2096ef01b6eec Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Wed, 24 Nov 2021 12:01:19 -0500 Subject: [PATCH 10/26] test for mutating value through a reference --- .../tests/checker/external_mutation_test.go | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index 554d86089c..b4b88da296 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -694,7 +694,7 @@ func TestPubSetNestedAccessModifier(t *testing.T) { } func TestSelfContainingStruct(t *testing.T) { - t.Run("pub set dict", func(t *testing.T) { + t.Run("pub let", func(t *testing.T) { _, err := ParseAndCheckWithOptions(t, ` pub contract C { @@ -718,3 +718,34 @@ func TestSelfContainingStruct(t *testing.T) { }) } + +func TestMutationThroughReference(t *testing.T) { + t.Run("pub let", func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + ` + pub fun main() { + let foo = Foo() + foo.ref.arr.append("y") + } + + pub struct Foo { + pub let ref: &Bar + init() { + self.ref = &Bar() as &Bar + } + } + + pub struct Bar { + pub let arr: [String] + init() { + self.arr = ["x"] + } + } + `, + ParseAndCheckOptions{}, + ) + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + }) +} From 8bcf40eb1bbb36a122c0a4375cb95add6cc28edc Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Wed, 24 Nov 2021 16:15:18 -0500 Subject: [PATCH 11/26] add rfc for mutability feature --- rfcs/0003-limited-mutability.md | 181 ++++++++++++++++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 rfcs/0003-limited-mutability.md diff --git a/rfcs/0003-limited-mutability.md b/rfcs/0003-limited-mutability.md new file mode 100644 index 0000000000..6e3c9f0eec --- /dev/null +++ b/rfcs/0003-limited-mutability.md @@ -0,0 +1,181 @@ +# Feature name + +- Proposal: RFC-0003 +- Authors: @dsainati1 +- Status: Awaiting implementation +- Issues: [#1260](https://github.com/onflow/cadence/issues/1260) + +## Summary + +[summary]: #summary + +This proposed change would limit the scopes in which the fields of composite types +like contracts, structs, and resources can be mutated. Instead of allowing array +and dictionary fields to be modified in any scope where the field can be read, instead +Cadence would issue a type error. These fields would instead be only modifiable +in the current declaration scope, as well as inner scopes of that scope. + +## Motivation + +[motivation]: #motivation + +Accidentally exposing a mutable variable to external consumers of a contract is currently a +large potential footgun standing in the way of a release of a stable, trustless version of +Cadence. Developers may declare a "constant" field on their contract with `pub let`, intending +that the field only be readable to transactions and other contracts, and unintentially allow +other code to add or remove elements from a dictionary or array stored in that field. Consider this code: + +``` +pub contract Foo { + pub let x : [Int] + + init() { + self.x = [] + } +} + +// in some external code importing Foo +pub fun bar() { + Foo.x.append(1) +} +``` + +Currently Cadence does not warn against this, or prevent a developer from writing this code, even +though, depending on what is stored in `x`, this could be unsafe. + +## Explanation + +[explanation]: #explanation + +Cadence controls where and to what extent variables can be read and written using a combination of +access modifiers and declaration kinds, as described [here](https://docs.onflow.org/cadence/language/access-control/). +Of note is that the `let` kind does not allow fields to be written to in any scope, whereas `var` allows them +to be written in the "Current and Inner" scopes; that is, the scope in which the field was declared, and any scopes +contained within that scope. + +However, simply writing to a field directly is not the only way in which one can modify a value. Consider the following +example: + +``` +pub struct Foo { + pub let x : [Int] + + init() { + self.x = [3]; + } +} + +pub fun bar() { + let foo = Foo() + foo.x = [0] // writes to x, not allowed + foo.x[0] = 0; // does not write to x, also not allowed +} +``` + +Cadence also restricts the scopes in which an array or dictionary field can be modified (or "mutated"). Examples of +mutating operations include an indexed assignment, as in the above example, as well as calls to the `append` or `remove`, +methods of arrays, or the `insert` or `remove` methods of dictionaries. These operations can only be performed on a field +in the current and inner scopes, the same contexts in which the field could be written to if it were a `var`. So the following +would typecheck: + +``` +pub struct Foo { + pub let x : [Int] + + init() { + self.x = [3]; + } + + pub fun addToX(i: Int) { + self.x.append(i) + } +} +``` + +while the following would not: + +``` +pub struct Foo { + pub let y : [Int] + + init() { + self.y = [3]; + } +} + +pub fun addToY(foo: Foo, i: Int) { + foo.y.append(i) +} +``` + +This prevents external code from mutating the values of fields it can read from your contract. Consumers +of your contract may read the values in a `pub let` or `pub var` field, but cannot change them in any way. + +If you wish to allow other code to update or modify a field in your contract, you may expose a method +to do so, like in the example above with `addToX`, or you may use the `pub(set)` access mode, which +allows any code to mutate or write to the field it applies to. + + +## Detailed design + +[detailed-design]: #detailed-design + +This change adds a new error, the `ExternalMutationError`, which is raised when a field +is mutated outside of the context in which it was defined. The error message will also +suggest that the user instead use a setter or modifier method for that field. + +Specifically, the error is emitted whenever a user attempts to perform an +index assignment on a member that is not either declared with the `pub(set)` +access mode, or is defined in the current enclosing scope. This check is the +same one performed for writing to fields, with the difference that mutation +is allowed on both `let` and `var` fields, while only the latter can be written to. + +Additionally, array and dictionary methods now track an additional bit of information +indicating whether they mutate their receiver. Mutating methods may not be called on +members that are not declared with the `pub(set)` access mode, or defined in the current +enclosing scope. + +The array methods that are considered mutating are `append`, `appendAll`, `remove`, +`insert`, `removeFirst` and `removeLast`. The dictionary methods that are considered +mutating are `insert` and `remove`. + +The limitations on mutation are designed to closely mirror the limitations on writes, +so that they can be easily explained to and understood by the user in terms of +language principles with which they are already familiar. Similarly, the suggested +workaround of adding a setter or modifier method to the composite type is designed +to be immediately recognizable to any developer familiar with object-oriented +design principles. + +## Drawbacks + +[drawbacks]: #drawbacks + +Why should we *not* do this? + +## Alternatives + +[alternatives]: #alternatives + +What other designs have been considered and what is the rationale for not choosing them? + +## Prior art + +[prior-art]: #prior-art + +Does this feature exist in other programming languages and what experience have their community had? + +This section is intended to encourage you as an author to think about the lessons from other languages, provide readers of your RFC with a fuller picture. + +If there is no prior art, that is fine - your ideas are interesting to us whether they are brand new or if it is an adaptation from other languages. + +## Unresolved questions + +[unresolved-questions]: #unresolved-questions + +What parts of the design are or do you expect to be still resolved? + +## Related + +[related]: #related + +What related issues do you consider out of scope for this RFC that could be addressed in the future independently of the solution that comes out of this RFC? From d8143c6454ba243a5fbc1013cd3a4a99491cdf4d Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Fri, 26 Nov 2021 17:02:04 -0500 Subject: [PATCH 12/26] iterate on rfc, add another test for code included there --- rfcs/0003-limited-mutability.md | 216 +++++++++++++++++- .../tests/checker/external_mutation_test.go | 36 +++ 2 files changed, 247 insertions(+), 5 deletions(-) diff --git a/rfcs/0003-limited-mutability.md b/rfcs/0003-limited-mutability.md index 6e3c9f0eec..cef92ffca3 100644 --- a/rfcs/0003-limited-mutability.md +++ b/rfcs/0003-limited-mutability.md @@ -53,8 +53,7 @@ Of note is that the `let` kind does not allow fields to be written to in any sco to be written in the "Current and Inner" scopes; that is, the scope in which the field was declared, and any scopes contained within that scope. -However, simply writing to a field directly is not the only way in which one can modify a value. Consider the following -example: +However, simply writing to a field directly is not the only way in which one can modify a value. Consider the following example: ``` pub struct Foo { @@ -115,7 +114,114 @@ If you wish to allow other code to update or modify a field in your contract, yo to do so, like in the example above with `addToX`, or you may use the `pub(set)` access mode, which allows any code to mutate or write to the field it applies to. +Some examples of code that produces a type error as a result of this restriction: +``` +pub resource Foo { + pub let x : {Int: Int} + + init() { + self.x = {0:3}; + } +} + +pub fun bar() { + let foo <- create Foo() + foo.x[0] = 3 // cannot mutate `x`, field was defined in `Foo` + destroy foo +} +``` + +``` +pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } +} + +pub struct Foo { + pub let x : [Int] + + init() { + self.x = [3] + } +} + +pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 // cannot mutate `x`, field was defined in `Foo` +} +``` + +``` +pub contract Foo { + pub let x : S + + pub struct S { + pub let y : [Int] + init() { + self.y = [3] + } + } + + init() { + self.x = S() + } +} + +pub fun bar() { + Foo.x.y.remove(at: 0) // cannot mutate `y`, field was defined in `S` +} +``` +``` +pub contract Foo { + pub let x : S + + pub struct S { + pub let y : [Int] + init() { + self.y = [3] + } + } + + init() { + self.x = S() + self.x.y.append(2) // cannot mutate `y`, field was defined in `S` + } +} +``` + +while the following are allowed: + +``` +pub struct Foo { + pub let x: {Int: Int} + + init() { + self.x = {3:3}; + } + + pub fun bar() { + let foo = Foo() + foo.x[0] = 3 // ok, mutation occurs inside defining struct Foo + } +} +``` +``` +pub struct Foo { + pub(set) var x: {Int: Int} + + init() { + self.x = {3:3}; + } +} + +pub fun bar() { + let foo = Foo() + foo.x.insert(key: 0, 3) // ok, pub(set) access modifier allows mutation +} +``` ## Detailed design [detailed-design]: #detailed-design @@ -150,13 +256,76 @@ design principles. [drawbacks]: #drawbacks -Why should we *not* do this? +This has the potential to break a number of existing contracts by restricting +code that was previously legal. This change would require a migration path for +these contracts in order for developers to update their contracts to satisfy +Cadence's new restrictions. + +This also removes a small amount of expressivity from the language. Previously, +the `pub let` declaration was a way to create a field that could be read and +mutated in all contexts, but not written, while `pub(set) var` created +a field that could be read, written and mutated in all contexts. After this change, +it would not be possible to describe a field that can be read and mutated but not set, +as `pub let` would only allow reads in arbitrary contexts. ## Alternatives [alternatives]: #alternatives -What other designs have been considered and what is the rationale for not choosing them? +One possible approach would be to add mutability modifiers to field or variable declarations, +like the `readonly` or `mut` tags found in languages like Rust, C# and TypeScript. This would make +all fields either mutable or immutable by default, requiring users to supply the appropriate tag +to make their behavior be whichever is not the default. + +This approach has the unfortunate side effect of increasing the surface area of the language, while +not also adding immediate benefit; the `readonly` and `mut` distinction may be redundant when +`let` and `var` already exist in the language. To justify this addition, we would need to identify +a use case for allowing field to be written to but not mutated (`readonly var`), or for restricting +the mutation of fields even within their enclosing type. + +Another approach would be to make `let` a true constant declaration, and forbid writing or mutating it in any context, +while allowing `var` to be mutated in all the contexts it can be written. This, however, is likely too restrictive; +a complex initialization for a dictionary or array value may benefit from the ability to iteratively mutate its contents, +up to the point at which its value is finalized, and can thus be exported. As such we would like to still allow users +to perform mutations in an limited context. + +A third approach would be to ban contract-level public field, with the idea that users cannot accidentally shoot +themselves in the foot by exporting a `pub let` field from their contract and having it be externally mutated +if `pub let` fields cannot exist on contracts in the first place. However, this has a number of downsides. The +obvious one is that all data that one might wish to expose to be read from a contract would need an explicit getter +method. The second, less obvious but also more concerning downside, is that it would be necessary to +ban `pub let` fields on structs and resources as well. Consider the case where a user exports a getter method +to read a struct or resource used by their contract. Any `pub let` fields on this struct or resource +would also be mutable, and thus are subject to the same risks they would be if they were exported +directly from the contract. + +Consider: + +``` +pub contract C { + pub struct Foo { + pub let arr : [Int] + init() { + self.arr = [3] + } + } + + priv let foo : Foo + + init() { + self.foo = Foo() + } + + pub fun getFoo(): Foo { + return self.foo + } +} + +pub fun main() { + let a = C.getFoo() + a.arr.append(0) // a.arr is now [3, 0] +} +``` ## Prior art @@ -172,7 +341,44 @@ If there is no prior art, that is fine - your ideas are interesting to us whethe [unresolved-questions]: #unresolved-questions -What parts of the design are or do you expect to be still resolved? +Do we wish to handle aliased references? Consider the following example (courtesy of @SupunS): +``` +pub fun main() { + let foo <- create Foo() + + log(foo.immutable) + + foo.mutable.content = "updated" // mutating the 'immutable' via 'mutable' + + log(foo.immutable) + + destroy foo +} + +pub resource Foo { + pub(set) var mutable: &Bar + + pub let immutable: &Bar + + init() { + self.mutable = &Bar() as &Bar + + self.immutable = self.mutable. // immutable holds a direct/indirect reference to a mutable + } +} + + +pub struct Bar { + pub(set) var content: String + init() { + self.content = "original" + } +} +``` + +This is one potential way to get around mutability restrictions on `pub let` fields, by aliasing such +a field with a `pub(set) var`. Is preventing this kind of issue within the scope of this change? How +would we begin to approach such a restriction? ## Related diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index b4b88da296..fc88510a50 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -749,3 +749,39 @@ func TestMutationThroughReference(t *testing.T) { require.ErrorAs(t, errs[0], &externalMutationError) }) } + +func TestMutationThroughAccess(t *testing.T) { + t.Run("pub let", func(t *testing.T) { + _, err := ParseAndCheckWithOptions(t, + ` + pub contract C { + pub struct Foo { + pub let arr : [Int] + init() { + self.arr = [3] + } + } + + priv let foo : Foo + + init() { + self.foo = Foo() + } + + pub fun getFoo(): Foo { + return self.foo + } + } + + pub fun main() { + let a = C.getFoo() + a.arr.append(0) // a.arr is now [3, 0] + } + `, + ParseAndCheckOptions{}, + ) + errs := ExpectCheckerErrors(t, err, 1) + var externalMutationError *sema.ExternalMutationError + require.ErrorAs(t, errs[0], &externalMutationError) + }) +} From fb7cfacdd8bc116264f126fd9ad35d72ab259cdd Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Fri, 26 Nov 2021 17:45:18 -0500 Subject: [PATCH 13/26] add prior art section for Swift --- rfcs/0003-limited-mutability.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/rfcs/0003-limited-mutability.md b/rfcs/0003-limited-mutability.md index cef92ffca3..e3eda46151 100644 --- a/rfcs/0003-limited-mutability.md +++ b/rfcs/0003-limited-mutability.md @@ -327,15 +327,21 @@ pub fun main() { } ``` +Another approach would be to have fields use a different type depending on the scope in which they are +viewed. Within a struct, a `pub let` array field would have the type `[Int]`, permitting all array +operations on it, including mutating ones. Outside the struct, the same field might have the type +`[Int]{ReadonlyArray}`, in effect a restricted type that only permits the non-mutating +operations to be performed on that array. This, however, is overly complex, and introduces +a new type to accomplish the same purpose as what this FLIP proposes. + ## Prior art [prior-art]: #prior-art -Does this feature exist in other programming languages and what experience have their community had? - -This section is intended to encourage you as an author to think about the lessons from other languages, provide readers of your RFC with a fuller picture. - -If there is no prior art, that is fine - your ideas are interesting to us whether they are brand new or if it is an adaptation from other languages. +In Swift (where Cadence's access modifiers already draw inspiration from), one can define a field +with a `public private(set) var` access modifier. A field defined this way can be read in a public context, +but can only be set privately. In effect, the change described in this FLIP is changing the default behavior +of fields to have what Swift would consider a private setter. ## Unresolved questions From 0825e3fe265e22d490593fa6236fc107ef2d9973 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Mon, 29 Nov 2021 14:21:03 -0500 Subject: [PATCH 14/26] remove rfc, as it is a FLIP now --- rfcs/0003-limited-mutability.md | 393 -------------------------------- 1 file changed, 393 deletions(-) delete mode 100644 rfcs/0003-limited-mutability.md diff --git a/rfcs/0003-limited-mutability.md b/rfcs/0003-limited-mutability.md deleted file mode 100644 index e3eda46151..0000000000 --- a/rfcs/0003-limited-mutability.md +++ /dev/null @@ -1,393 +0,0 @@ -# Feature name - -- Proposal: RFC-0003 -- Authors: @dsainati1 -- Status: Awaiting implementation -- Issues: [#1260](https://github.com/onflow/cadence/issues/1260) - -## Summary - -[summary]: #summary - -This proposed change would limit the scopes in which the fields of composite types -like contracts, structs, and resources can be mutated. Instead of allowing array -and dictionary fields to be modified in any scope where the field can be read, instead -Cadence would issue a type error. These fields would instead be only modifiable -in the current declaration scope, as well as inner scopes of that scope. - -## Motivation - -[motivation]: #motivation - -Accidentally exposing a mutable variable to external consumers of a contract is currently a -large potential footgun standing in the way of a release of a stable, trustless version of -Cadence. Developers may declare a "constant" field on their contract with `pub let`, intending -that the field only be readable to transactions and other contracts, and unintentially allow -other code to add or remove elements from a dictionary or array stored in that field. Consider this code: - -``` -pub contract Foo { - pub let x : [Int] - - init() { - self.x = [] - } -} - -// in some external code importing Foo -pub fun bar() { - Foo.x.append(1) -} -``` - -Currently Cadence does not warn against this, or prevent a developer from writing this code, even -though, depending on what is stored in `x`, this could be unsafe. - -## Explanation - -[explanation]: #explanation - -Cadence controls where and to what extent variables can be read and written using a combination of -access modifiers and declaration kinds, as described [here](https://docs.onflow.org/cadence/language/access-control/). -Of note is that the `let` kind does not allow fields to be written to in any scope, whereas `var` allows them -to be written in the "Current and Inner" scopes; that is, the scope in which the field was declared, and any scopes -contained within that scope. - -However, simply writing to a field directly is not the only way in which one can modify a value. Consider the following example: - -``` -pub struct Foo { - pub let x : [Int] - - init() { - self.x = [3]; - } -} - -pub fun bar() { - let foo = Foo() - foo.x = [0] // writes to x, not allowed - foo.x[0] = 0; // does not write to x, also not allowed -} -``` - -Cadence also restricts the scopes in which an array or dictionary field can be modified (or "mutated"). Examples of -mutating operations include an indexed assignment, as in the above example, as well as calls to the `append` or `remove`, -methods of arrays, or the `insert` or `remove` methods of dictionaries. These operations can only be performed on a field -in the current and inner scopes, the same contexts in which the field could be written to if it were a `var`. So the following -would typecheck: - -``` -pub struct Foo { - pub let x : [Int] - - init() { - self.x = [3]; - } - - pub fun addToX(i: Int) { - self.x.append(i) - } -} -``` - -while the following would not: - -``` -pub struct Foo { - pub let y : [Int] - - init() { - self.y = [3]; - } -} - -pub fun addToY(foo: Foo, i: Int) { - foo.y.append(i) -} -``` - -This prevents external code from mutating the values of fields it can read from your contract. Consumers -of your contract may read the values in a `pub let` or `pub var` field, but cannot change them in any way. - -If you wish to allow other code to update or modify a field in your contract, you may expose a method -to do so, like in the example above with `addToX`, or you may use the `pub(set)` access mode, which -allows any code to mutate or write to the field it applies to. - -Some examples of code that produces a type error as a result of this restriction: - -``` -pub resource Foo { - pub let x : {Int: Int} - - init() { - self.x = {0:3}; - } -} - -pub fun bar() { - let foo <- create Foo() - foo.x[0] = 3 // cannot mutate `x`, field was defined in `Foo` - destroy foo -} -``` - -``` -pub struct Bar { - pub let foo: Foo - init() { - self.foo = Foo() - } -} - -pub struct Foo { - pub let x : [Int] - - init() { - self.x = [3] - } -} - -pub fun bar() { - let bar = Bar() - bar.foo.x[0] = 3 // cannot mutate `x`, field was defined in `Foo` -} -``` - -``` -pub contract Foo { - pub let x : S - - pub struct S { - pub let y : [Int] - init() { - self.y = [3] - } - } - - init() { - self.x = S() - } -} - -pub fun bar() { - Foo.x.y.remove(at: 0) // cannot mutate `y`, field was defined in `S` -} -``` -``` -pub contract Foo { - pub let x : S - - pub struct S { - pub let y : [Int] - init() { - self.y = [3] - } - } - - init() { - self.x = S() - self.x.y.append(2) // cannot mutate `y`, field was defined in `S` - } -} -``` - -while the following are allowed: - -``` -pub struct Foo { - pub let x: {Int: Int} - - init() { - self.x = {3:3}; - } - - pub fun bar() { - let foo = Foo() - foo.x[0] = 3 // ok, mutation occurs inside defining struct Foo - } -} -``` -``` -pub struct Foo { - pub(set) var x: {Int: Int} - - init() { - self.x = {3:3}; - } -} - -pub fun bar() { - let foo = Foo() - foo.x.insert(key: 0, 3) // ok, pub(set) access modifier allows mutation -} -``` -## Detailed design - -[detailed-design]: #detailed-design - -This change adds a new error, the `ExternalMutationError`, which is raised when a field -is mutated outside of the context in which it was defined. The error message will also -suggest that the user instead use a setter or modifier method for that field. - -Specifically, the error is emitted whenever a user attempts to perform an -index assignment on a member that is not either declared with the `pub(set)` -access mode, or is defined in the current enclosing scope. This check is the -same one performed for writing to fields, with the difference that mutation -is allowed on both `let` and `var` fields, while only the latter can be written to. - -Additionally, array and dictionary methods now track an additional bit of information -indicating whether they mutate their receiver. Mutating methods may not be called on -members that are not declared with the `pub(set)` access mode, or defined in the current -enclosing scope. - -The array methods that are considered mutating are `append`, `appendAll`, `remove`, -`insert`, `removeFirst` and `removeLast`. The dictionary methods that are considered -mutating are `insert` and `remove`. - -The limitations on mutation are designed to closely mirror the limitations on writes, -so that they can be easily explained to and understood by the user in terms of -language principles with which they are already familiar. Similarly, the suggested -workaround of adding a setter or modifier method to the composite type is designed -to be immediately recognizable to any developer familiar with object-oriented -design principles. - -## Drawbacks - -[drawbacks]: #drawbacks - -This has the potential to break a number of existing contracts by restricting -code that was previously legal. This change would require a migration path for -these contracts in order for developers to update their contracts to satisfy -Cadence's new restrictions. - -This also removes a small amount of expressivity from the language. Previously, -the `pub let` declaration was a way to create a field that could be read and -mutated in all contexts, but not written, while `pub(set) var` created -a field that could be read, written and mutated in all contexts. After this change, -it would not be possible to describe a field that can be read and mutated but not set, -as `pub let` would only allow reads in arbitrary contexts. - -## Alternatives - -[alternatives]: #alternatives - -One possible approach would be to add mutability modifiers to field or variable declarations, -like the `readonly` or `mut` tags found in languages like Rust, C# and TypeScript. This would make -all fields either mutable or immutable by default, requiring users to supply the appropriate tag -to make their behavior be whichever is not the default. - -This approach has the unfortunate side effect of increasing the surface area of the language, while -not also adding immediate benefit; the `readonly` and `mut` distinction may be redundant when -`let` and `var` already exist in the language. To justify this addition, we would need to identify -a use case for allowing field to be written to but not mutated (`readonly var`), or for restricting -the mutation of fields even within their enclosing type. - -Another approach would be to make `let` a true constant declaration, and forbid writing or mutating it in any context, -while allowing `var` to be mutated in all the contexts it can be written. This, however, is likely too restrictive; -a complex initialization for a dictionary or array value may benefit from the ability to iteratively mutate its contents, -up to the point at which its value is finalized, and can thus be exported. As such we would like to still allow users -to perform mutations in an limited context. - -A third approach would be to ban contract-level public field, with the idea that users cannot accidentally shoot -themselves in the foot by exporting a `pub let` field from their contract and having it be externally mutated -if `pub let` fields cannot exist on contracts in the first place. However, this has a number of downsides. The -obvious one is that all data that one might wish to expose to be read from a contract would need an explicit getter -method. The second, less obvious but also more concerning downside, is that it would be necessary to -ban `pub let` fields on structs and resources as well. Consider the case where a user exports a getter method -to read a struct or resource used by their contract. Any `pub let` fields on this struct or resource -would also be mutable, and thus are subject to the same risks they would be if they were exported -directly from the contract. - -Consider: - -``` -pub contract C { - pub struct Foo { - pub let arr : [Int] - init() { - self.arr = [3] - } - } - - priv let foo : Foo - - init() { - self.foo = Foo() - } - - pub fun getFoo(): Foo { - return self.foo - } -} - -pub fun main() { - let a = C.getFoo() - a.arr.append(0) // a.arr is now [3, 0] -} -``` - -Another approach would be to have fields use a different type depending on the scope in which they are -viewed. Within a struct, a `pub let` array field would have the type `[Int]`, permitting all array -operations on it, including mutating ones. Outside the struct, the same field might have the type -`[Int]{ReadonlyArray}`, in effect a restricted type that only permits the non-mutating -operations to be performed on that array. This, however, is overly complex, and introduces -a new type to accomplish the same purpose as what this FLIP proposes. - -## Prior art - -[prior-art]: #prior-art - -In Swift (where Cadence's access modifiers already draw inspiration from), one can define a field -with a `public private(set) var` access modifier. A field defined this way can be read in a public context, -but can only be set privately. In effect, the change described in this FLIP is changing the default behavior -of fields to have what Swift would consider a private setter. - -## Unresolved questions - -[unresolved-questions]: #unresolved-questions - -Do we wish to handle aliased references? Consider the following example (courtesy of @SupunS): -``` -pub fun main() { - let foo <- create Foo() - - log(foo.immutable) - - foo.mutable.content = "updated" // mutating the 'immutable' via 'mutable' - - log(foo.immutable) - - destroy foo -} - -pub resource Foo { - pub(set) var mutable: &Bar - - pub let immutable: &Bar - - init() { - self.mutable = &Bar() as &Bar - - self.immutable = self.mutable. // immutable holds a direct/indirect reference to a mutable - } -} - - -pub struct Bar { - pub(set) var content: String - init() { - self.content = "original" - } -} -``` - -This is one potential way to get around mutability restrictions on `pub let` fields, by aliasing such -a field with a `pub(set) var`. Is preventing this kind of issue within the scope of this change? How -would we begin to approach such a restriction? - -## Related - -[related]: #related - -What related issues do you consider out of scope for this RFC that could be addressed in the future independently of the solution that comes out of this RFC? From 91a4decca53096b2ebe7e077f9de3129b413c72e Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Thu, 9 Dec 2021 15:07:32 -0500 Subject: [PATCH 15/26] update docs --- docs/language/access-control.md | 41 ++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/docs/language/access-control.md b/docs/language/access-control.md index ab19bc895b..ac266a74fc 100644 --- a/docs/language/access-control.md +++ b/docs/language/access-control.md @@ -30,7 +30,7 @@ will be covered in a later section. Top-level declarations (variables, constants, functions, structures, resources, interfaces) and fields (in structures, and resources) are always only able to be written -to in the scope where it is defined (self). +to and mutated in the scope where it is defined (self). There are four levels of access control defined in the code that specify where a declaration can be accessed or called. @@ -74,21 +74,21 @@ a declaration can be accessed or called. **Access level must be specified for each declaration** -The `(set)` suffix can be used to make variables also publicly writable. +The `(set)` suffix can be used to make variables also publicly writable and mutable. To summarize the behavior for variable declarations, constant declarations, and fields: -| Declaration kind | Access modifier | Read scope | Write scope | -|:-----------------|:-------------------------|:-----------------------------------------------------|:------------------| -| `let` | `priv` / `access(self)` | Current and inner | *None* | -| `let` | `access(contract)` | Current, inner, and containing contract | *None* | -| `let` | `access(account)` | Current, inner, and other contracts in same account | *None* | -| `let` | `pub`,`access(all)` | **All** | *None* | -| `var` | `access(self)` | Current and inner | Current and inner | -| `var` | `access(contract)` | Current, inner, and containing contract | Current and inner | -| `var` | `access(account)` | Current, inner, and other contracts in same account | Current and inner | -| `var` | `pub` / `access(all)` | **All** | Current and inner | -| `var` | `pub(set)` | **All** | **All** | +| Declaration kind | Access modifier | Read scope | Write scope | Mutate scope | +|:-----------------|:-------------------------|:-----------------------------------------------------|:------------------|:------------------| +| `let` | `priv` / `access(self)` | Current and inner | *None* | Current and inner | +| `let` | `access(contract)` | Current, inner, and containing contract | *None* | Current and inner | +| `let` | `access(account)` | Current, inner, and other contracts in same account | *None* | Current and inner | +| `let` | `pub`,`access(all)` | **All** | *None* | Current and inner | +| `var` | `access(self)` | Current and inner | Current and inner | Current and inner | +| `var` | `access(contract)` | Current, inner, and containing contract | Current and inner | Current and inner | +| `var` | `access(account)` | Current, inner, and other contracts in same account | Current and inner | Current and inner | +| `var` | `pub` / `access(all)` | **All** | Current and inner | Current and inner | +| `var` | `pub(set)` | **All** | **All** | **All** | To summarize the behavior for functions: @@ -143,6 +143,10 @@ pub struct SomeStruct { // pub(set) var e: Int + // Arrays and dictionaries declared without (set) cannot be + // mutated in external scopes + pub let arr: [Int] + // The initializer is omitted for brevity. // Declare a private function which is only callable @@ -203,4 +207,15 @@ some.e // Valid: can set publicly settable variable field in outer scope. // some.e = 5 + +// Invalid: cannot mutate a public field in outer scope. +// +some.f.append(0) + +// Invalid: cannot mutate a public field in outer scope. +// +some.f[3] = 1 + +// Valid: can call non-mutating methods on a public field in outer scope +some.f.contains(0) ``` From c353c0482ca33dd3acccc6b09dac3f8371cd97a7 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Fri, 10 Dec 2021 09:55:12 -0500 Subject: [PATCH 16/26] clarify meaning of 'mutate' in docs --- docs/language/access-control.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/language/access-control.md b/docs/language/access-control.md index ac266a74fc..9609bdaf9a 100644 --- a/docs/language/access-control.md +++ b/docs/language/access-control.md @@ -30,7 +30,8 @@ will be covered in a later section. Top-level declarations (variables, constants, functions, structures, resources, interfaces) and fields (in structures, and resources) are always only able to be written -to and mutated in the scope where it is defined (self). +to and mutated (modified, such as by indexed assignment or methods like `append`) +in the scope where it is defined (self). There are four levels of access control defined in the code that specify where a declaration can be accessed or called. From c0f5d20e38d85a6c27328f36231579d7337f5b95 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Mon, 24 Jan 2022 11:04:11 -0500 Subject: [PATCH 17/26] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Müller --- runtime/sema/errors.go | 2 +- .../tests/checker/external_mutation_test.go | 41 ++++++++++--------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/runtime/sema/errors.go b/runtime/sema/errors.go index f29cd07afb..e5be83baba 100644 --- a/runtime/sema/errors.go +++ b/runtime/sema/errors.go @@ -2978,7 +2978,7 @@ type ExternalMutationError struct { func (e *ExternalMutationError) Error() string { return fmt.Sprintf( - "cannot mutate `%s`: %s was defined inside `%s`", + "cannot mutate `%s`: %s is only mutable inside `%s`", e.Name, e.DeclarationKind.Name(), e.EnclosingType.QualifiedString(), diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index fc88510a50..b819c139be 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -58,14 +58,17 @@ func TestArrayUpdateIndexAccess(t *testing.T) { } t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` pub contract C { pub %s Foo { - %s %s x : [Int] + %s %s x: [Int] init() { - self.x = [3]; + self.x = [3] } } @@ -129,10 +132,10 @@ func TestDictionaryUpdateIndexAccess(t *testing.T) { fmt.Sprintf(` pub contract C { pub %s Foo { - %s %s x : {Int: Int} + %s %s x: {Int: Int} init() { - self.x = {0:3}; + self.x = {0: 3} } } @@ -250,10 +253,10 @@ func TestNestedDictionaryUpdateIndexAccess(t *testing.T) { } pub struct Foo { - %s %s x : {Int: Int} + %s %s x: {Int: Int} init() { - self.x = {3:3} + self.x = {3: 3} } } @@ -301,7 +304,7 @@ func TestMutateContractIndexAccess(t *testing.T) { _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` pub contract Foo { - %s %s x : [Int] + %s %s x: [Int] init() { self.x = [3] @@ -359,10 +362,10 @@ func TestContractNestedStructIndexAccess(t *testing.T) { _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` pub contract Foo { - pub let x : S + pub let x: S pub struct S { - %s %s y : [Int] + %s %s y: [Int] init() { self.y = [3] } @@ -424,10 +427,10 @@ func TestContractStructInitIndexAccess(t *testing.T) { _, err := ParseAndCheckWithOptions(t, fmt.Sprintf(` pub contract Foo { - pub let x : S + pub let x: S pub struct S { - %s %s y : [Int] + %s %s y: [Int] init() { self.y = [3] } @@ -508,10 +511,10 @@ func TestArrayUpdateMethodCall(t *testing.T) { fmt.Sprintf(` pub contract C { pub %s Foo { - %s %s x : [Int] + %s %s x: [Int] init() { - self.x = [3]; + self.x = [3] } } @@ -596,10 +599,10 @@ func TestDictionaryUpdateMethodCall(t *testing.T) { fmt.Sprintf(` pub contract C { pub %s Foo { - %s %s x : {Int: Int} + %s %s x: {Int: Int} init() { - self.x = {3:3}; + self.x = {3: 3} } } @@ -643,7 +646,7 @@ func TestPubSetAccessModifier(t *testing.T) { pub(set) var x: {Int: Int} init() { - self.x = {3:3}; + self.x = {3: 3} } } @@ -673,7 +676,7 @@ func TestPubSetNestedAccessModifier(t *testing.T) { } pub struct Foo { - pub(set) var x : [Int] + pub(set) var x: [Int] init() { self.x = [3] @@ -702,7 +705,7 @@ func TestSelfContainingStruct(t *testing.T) { pub let x: {Int: Int} init() { - self.x = {3:3}; + self.x = {3: 3} } pub fun bar() { @@ -756,7 +759,7 @@ func TestMutationThroughAccess(t *testing.T) { ` pub contract C { pub struct Foo { - pub let arr : [Int] + pub let arr: [Int] init() { self.arr = [3] } From 14465b1469399b5fcfd0d1d0c710d4c1c883ef1b Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Mon, 24 Jan 2022 11:11:15 -0500 Subject: [PATCH 18/26] update documentation to better point out mutating methods --- docs/language/values-and-types.mdx | 18 +++++++++++++++++- runtime/sema/check_assignment.go | 4 ++-- runtime/sema/check_member_expression.go | 9 ++++----- runtime/sema/errors.go | 6 +++--- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/docs/language/values-and-types.mdx b/docs/language/values-and-types.mdx index b76b560efb..1f0186b0d4 100644 --- a/docs/language/values-and-types.mdx +++ b/docs/language/values-and-types.mdx @@ -1112,7 +1112,9 @@ It is invalid to use one of these functions on a fixed-sized array. Adds the new element `element` of type `T` to the end of the array. - The new element must be the same type as all the other elements in the array. + The new element must be the same type as all the other elements in the array. + + This function [mutates](../access-control) the array. ```cadence // Declare an array of integers. @@ -1133,6 +1135,8 @@ It is invalid to use one of these functions on a fixed-sized array. Both arrays must be the same type `T`. + This function [mutates](../access-control) the array. + ```cadence // Declare an array of integers. let numbers = [42, 23] @@ -1160,6 +1164,8 @@ It is invalid to use one of these functions on a fixed-sized array. All the elements after the new inserted element are shifted to the right by one. + This function [mutates](../access-control) the array. + ```cadence // Declare an array of integers. let numbers = [42, 23, 31, 12] @@ -1179,6 +1185,8 @@ It is invalid to use one of these functions on a fixed-sized array. The `index` must be within the bounds of the array. If the index is outside the bounds, the program aborts. + This function [mutates](../access-control) the array. + ```cadence // Declare an array of integers. let numbers = [42, 23, 31] @@ -1199,6 +1207,8 @@ It is invalid to use one of these functions on a fixed-sized array. The array must not be empty. If the array is empty, the program aborts. + This function [mutates](../access-control) the array. + ```cadence // Declare an array of integers. let numbers = [42, 23] @@ -1224,6 +1234,8 @@ It is invalid to use one of these functions on a fixed-sized array. The array must not be empty. If the array is empty, the program aborts. + This function [mutates](../access-control) the array. + ```cadence // Declare an array of integers. let numbers = [42, 23] @@ -1398,6 +1410,8 @@ booleans[0] = true if the dictionary contained the key, otherwise `nil`. + This function [mutates](../access-control) the dictionary. + ```cadence // Declare a dictionary mapping strings to integers. let numbers = {"twentyThree": 23} @@ -1420,6 +1434,8 @@ booleans[0] = true if the dictionary contained the key, otherwise `nil`. + This function [mutates](../access-control) the dictionary. + ```cadence // Declare a dictionary mapping strings to integers. let numbers = {"fortyTwo": 42, "twentyThree": 23} diff --git a/runtime/sema/check_assignment.go b/runtime/sema/check_assignment.go index ab7fc72c3f..eed603732c 100644 --- a/runtime/sema/check_assignment.go +++ b/runtime/sema/check_assignment.go @@ -221,8 +221,8 @@ func (checker *Checker) visitIndexExpressionAssignment( &ExternalMutationError{ Name: member.Identifier.Identifier, DeclarationKind: member.DeclarationKind, - Range: ast.NewRangeFromPositioned(targetExpression.Identifier), - EnclosingType: member.ContainerType, + Range: ast.NewRangeFromPositioned(targetExpression), + ContainerType: member.ContainerType, }, ) } diff --git a/runtime/sema/check_member_expression.go b/runtime/sema/check_member_expression.go index 5602361094..b19090a61f 100644 --- a/runtime/sema/check_member_expression.go +++ b/runtime/sema/check_member_expression.go @@ -172,7 +172,7 @@ func (checker *Checker) visitMember(expression *ast.MemberExpression) (accessedT Name: m.Identifier.Identifier, DeclarationKind: m.DeclarationKind, Range: ast.NewRangeFromPositioned(targetRange), - EnclosingType: m.ContainerType, + ContainerType: m.ContainerType, }, ) } @@ -327,11 +327,10 @@ func (checker *Checker) isWriteableMember(member *Member) bool { } // isMutatableMember returns true if the given member can be mutated -// in the current location of the checker -// +// in the current location of the checker. Currently equivalent to +// isWriteableMember above, but separate in case this changes func (checker *Checker) isMutatableMember(member *Member) bool { - return checker.isWriteableAccess(member.Access) || - checker.containerTypes[member.ContainerType] + return checker.isWriteableMember(member) } // containingContractKindedType returns the containing contract-kinded type diff --git a/runtime/sema/errors.go b/runtime/sema/errors.go index 1bffbb8f4f..380035d6da 100644 --- a/runtime/sema/errors.go +++ b/runtime/sema/errors.go @@ -2971,7 +2971,7 @@ func (e *InvalidEntryPointTypeError) Error() string { type ExternalMutationError struct { Name string - EnclosingType Type + ContainerType Type DeclarationKind common.DeclarationKind ast.Range } @@ -2981,7 +2981,7 @@ func (e *ExternalMutationError) Error() string { "cannot mutate `%s`: %s is only mutable inside `%s`", e.Name, e.DeclarationKind.Name(), - e.EnclosingType.QualifiedString(), + e.ContainerType.QualifiedString(), ) } @@ -2989,7 +2989,7 @@ func (e *ExternalMutationError) SecondaryError() string { return fmt.Sprintf( "Consider adding a setter for `%s` to `%s`", e.Name, - e.EnclosingType.QualifiedString(), + e.ContainerType.QualifiedString(), ) } From 9fe93479a32d20dff3dd06b53eab67bfab42c0e4 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Mon, 24 Jan 2022 11:25:24 -0500 Subject: [PATCH 19/26] improve tests --- runtime/common/declarationkind.go | 2 +- .../tests/checker/external_mutation_test.go | 743 +++++++++--------- 2 files changed, 366 insertions(+), 379 deletions(-) diff --git a/runtime/common/declarationkind.go b/runtime/common/declarationkind.go index 268e210790..dd3021f849 100644 --- a/runtime/common/declarationkind.go +++ b/runtime/common/declarationkind.go @@ -149,7 +149,7 @@ func (k DeclarationKind) Keywords() string { case DeclarationKindVariable: return "var" case DeclarationKindConstant: - return "const" + return "let" case DeclarationKindStructure: return "struct" case DeclarationKindResource: diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index b819c139be..eea782afcd 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -24,6 +24,8 @@ import ( "github.com/stretchr/testify/require" + "github.com/onflow/cadence/runtime/ast" + "github.com/onflow/cadence/runtime/common" "github.com/onflow/cadence/runtime/sema" ) @@ -31,28 +33,28 @@ func TestArrayUpdateIndexAccess(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - valueKinds := []string{ - "struct", - "resource", + valueKinds := []common.CompositeKind{ + common.CompositeKindStructure, + common.CompositeKindResource, } - runTest := func(access string, declaration string, valueKind string) { - testName := fmt.Sprintf("%s %s %s", access, valueKind, declaration) + runTest := func(access ast.Access, declaration common.DeclarationKind, valueKind common.CompositeKind) { + testName := fmt.Sprintf("%s %s %s", access.Keyword(), valueKind.Keyword(), declaration.Keywords()) assignmentOp := "=" var destroyStatement string - if valueKind == "resource" { + if valueKind == common.CompositeKindResource { assignmentOp = "<- create" destroyStatement = "destroy foo" } @@ -61,25 +63,24 @@ func TestArrayUpdateIndexAccess(t *testing.T) { t.Parallel() - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract C { - pub %s Foo { - %s %s x: [Int] - - init() { - self.x = [3] - } - } - - pub fun bar() { - let foo %s Foo() - foo.x[0] = 3 - %s - } - } - `, valueKind, access, declaration, assignmentOp, destroyStatement), - ParseAndCheckOptions{}, + pub contract C { + pub %s Foo { + %s %s x: [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + let foo %s Foo() + foo.x[0] = 3 + %s + } + } + `, valueKind.Keyword(), access.Keyword(), declaration.Keywords(), assignmentOp, destroyStatement), ) errs := ExpectCheckerErrors(t, err, 1) @@ -101,52 +102,51 @@ func TestDictionaryUpdateIndexAccess(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - valueKinds := []string{ - "struct", - "resource", + valueKinds := []common.CompositeKind{ + common.CompositeKindStructure, + common.CompositeKindResource, } - runTest := func(access string, declaration string, valueKind string) { - testName := fmt.Sprintf("%s %s %s", access, valueKind, declaration) + runTest := func(access ast.Access, declaration common.DeclarationKind, valueKind common.CompositeKind) { + testName := fmt.Sprintf("%s %s %s", access.Keyword(), valueKind.Keyword(), declaration.Keywords()) assignmentOp := "=" var destroyStatement string - if valueKind == "resource" { + if valueKind == common.CompositeKindResource { assignmentOp = "<- create" destroyStatement = "destroy foo" } t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract C { - pub %s Foo { - %s %s x: {Int: Int} - - init() { - self.x = {0: 3} - } - } - - pub fun bar() { - let foo %s Foo() - foo.x[0] = 3 - %s - } - } - `, valueKind, access, declaration, assignmentOp, destroyStatement), - ParseAndCheckOptions{}, + pub contract C { + pub %s Foo { + %s %s x: {Int: Int} + + init() { + self.x = {0: 3} + } + } + + pub fun bar() { + let foo %s Foo() + foo.x[0] = 3 + %s + } + } + `, valueKind.Keyword(), access.Keyword(), declaration.Keywords(), assignmentOp, destroyStatement), ) errs := ExpectCheckerErrors(t, err, 1) @@ -168,46 +168,45 @@ func TestNestedArrayUpdateIndexAccess(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - runTest := func(access string, declaration string) { - testName := fmt.Sprintf("%s struct %s", access, declaration) + runTest := func(access ast.Access, declaration common.DeclarationKind) { + testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract C { - pub struct Bar { - pub let foo: Foo - init() { - self.foo = Foo() - } - } - - pub struct Foo { - %s %s x : [Int] - - init() { - self.x = [3] - } - } - - pub fun bar() { - let bar = Bar() - bar.foo.x[0] = 3 - } - } - `, access, declaration), - ParseAndCheckOptions{}, + pub contract C { + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } + } + + pub struct Foo { + %s %s x : [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } + } + `, access.Keyword(), declaration.Keywords()), ) errs := ExpectCheckerErrors(t, err, 1) @@ -227,46 +226,45 @@ func TestNestedDictionaryUpdateIndexAccess(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - runTest := func(access string, declaration string) { - testName := fmt.Sprintf("%s struct %s", access, declaration) + runTest := func(access ast.Access, declaration common.DeclarationKind) { + testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract C { - pub struct Bar { - pub let foo: Foo - init() { - self.foo = Foo() - } - } - - pub struct Foo { - %s %s x: {Int: Int} - - init() { - self.x = {3: 3} - } - } - - pub fun bar() { - let bar = Bar() - bar.foo.x[0] = 3 - } - } - `, access, declaration), - ParseAndCheckOptions{}, + pub contract C { + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } + } + + pub struct Foo { + %s %s x: {Int: Int} + + init() { + self.x = {3: 3} + } + } + + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } + } + `, access.Keyword(), declaration.Keywords()), ) errs := ExpectCheckerErrors(t, err, 1) @@ -286,40 +284,39 @@ func TestMutateContractIndexAccess(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - runTest := func(access string, declaration string) { - testName := fmt.Sprintf("%s struct %s", access, declaration) + runTest := func(access ast.Access, declaration common.DeclarationKind) { + testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract Foo { - %s %s x: [Int] - - init() { - self.x = [3] - } - } - - pub fun bar() { - Foo.x[0] = 1 - } - `, access, declaration), - ParseAndCheckOptions{}, + pub contract Foo { + %s %s x: [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + Foo.x[0] = 1 + } + `, access.Keyword(), declaration.Keywords()), ) expectedErrors := 1 - if access == "access(contract)" { + if access == ast.AccessContract { expectedErrors++ } @@ -344,47 +341,46 @@ func TestContractNestedStructIndexAccess(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - runTest := func(access string, declaration string) { - testName := fmt.Sprintf("%s struct %s", access, declaration) + runTest := func(access ast.Access, declaration common.DeclarationKind) { + testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract Foo { - pub let x: S - - pub struct S { - %s %s y: [Int] - init() { - self.y = [3] - } - } - - init() { - self.x = S() - } - } - - pub fun bar() { - Foo.x.y[0] = 1 - } - `, access, declaration), - ParseAndCheckOptions{}, + pub contract Foo { + pub let x: S + + pub struct S { + %s %s y: [Int] + init() { + self.y = [3] + } + } + + init() { + self.x = S() + } + } + + pub fun bar() { + Foo.x.y[0] = 1 + } + `, access.Keyword(), declaration.Keywords()), ) expectedErrors := 1 - if access == "access(contract)" { + if access == ast.AccessContract { expectedErrors++ } @@ -409,40 +405,39 @@ func TestContractStructInitIndexAccess(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - runTest := func(access string, declaration string) { - testName := fmt.Sprintf("%s struct %s", access, declaration) + runTest := func(access ast.Access, declaration common.DeclarationKind) { + testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract Foo { - pub let x: S - - pub struct S { - %s %s y: [Int] - init() { - self.y = [3] - } - } - - init() { - self.x = S() - self.x.y[1] = 2 - } - } - `, access, declaration), - ParseAndCheckOptions{}, + pub contract Foo { + pub let x: S + + pub struct S { + %s %s y: [Int] + init() { + self.y = [3] + } + } + + init() { + self.x = S() + self.x.y[1] = 2 + } + } + `, access.Keyword(), declaration.Keywords()), ) errs := ExpectCheckerErrors(t, err, 1) @@ -462,20 +457,20 @@ func TestArrayUpdateMethodCall(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - valueKinds := []string{ - "struct", - "resource", + valueKinds := []common.CompositeKind{ + common.CompositeKindStructure, + common.CompositeKindResource, } type MethodCall = struct { @@ -496,36 +491,35 @@ func TestArrayUpdateMethodCall(t *testing.T) { {Mutating: true, Code: ".removeLast()", Name: "removeLast"}, } - runTest := func(access string, declaration string, valueKind string, member MethodCall) { - testName := fmt.Sprintf("%s %s %s %s", access, valueKind, declaration, member.Name) + runTest := func(access ast.Access, declaration common.DeclarationKind, valueKind common.CompositeKind, member MethodCall) { + testName := fmt.Sprintf("%s %s %s %s", access.Keyword(), valueKind.Keyword(), declaration.Keywords(), member.Name) assignmentOp := "=" var destroyStatement string - if valueKind == "resource" { + if valueKind == common.CompositeKindResource { assignmentOp = "<- create" destroyStatement = "destroy foo" } t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract C { - pub %s Foo { - %s %s x: [Int] - - init() { - self.x = [3] - } - } - - pub fun bar() { - let foo %s Foo() - foo.x%s - %s - } - } - `, valueKind, access, declaration, assignmentOp, member.Code, destroyStatement), - ParseAndCheckOptions{}, + pub contract C { + pub %s Foo { + %s %s x: [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + let foo %s Foo() + foo.x%s + %s + } + } + `, valueKind.Keyword(), access.Keyword(), declaration.Keywords(), assignmentOp, member.Code, destroyStatement), ) if member.Mutating { @@ -553,22 +547,21 @@ func TestDictionaryUpdateMethodCall(t *testing.T) { t.Parallel() - accessModifiers := []string{ - "pub", - "access(account)", - "access(contract)", + accessModifiers := []ast.Access{ + ast.AccessPublic, + ast.AccessAccount, + ast.AccessContract, } - declarationKinds := []string{ - "let", - "var", + declarationKinds := []common.DeclarationKind{ + common.DeclarationKindConstant, + common.DeclarationKindVariable, } - valueKinds := []string{ - "struct", - "resource", + valueKinds := []common.CompositeKind{ + common.CompositeKindStructure, + common.CompositeKindResource, } - type MethodCall = struct { Mutating bool Code string @@ -584,36 +577,35 @@ func TestDictionaryUpdateMethodCall(t *testing.T) { {Mutating: true, Code: ".remove(key: 0)", Name: "remove"}, } - runTest := func(access string, declaration string, valueKind string, member MethodCall) { - testName := fmt.Sprintf("%s %s %s %s", access, valueKind, declaration, member.Name) + runTest := func(access ast.Access, declaration common.DeclarationKind, valueKind common.CompositeKind, member MethodCall) { + testName := fmt.Sprintf("%s %s %s %s", access.Keyword(), valueKind.Keyword(), declaration.Keywords(), member.Name) assignmentOp := "=" var destroyStatement string - if valueKind == "resource" { + if valueKind == common.CompositeKindResource { assignmentOp = "<- create" destroyStatement = "destroy foo" } t.Run(testName, func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, fmt.Sprintf(` - pub contract C { - pub %s Foo { - %s %s x: {Int: Int} - - init() { - self.x = {3: 3} - } - } - - pub fun bar() { - let foo %s Foo() - foo.x%s - %s - } - } - `, valueKind, access, declaration, assignmentOp, member.Code, destroyStatement), - ParseAndCheckOptions{}, + pub contract C { + pub %s Foo { + %s %s x: {Int: Int} + + init() { + self.x = {3: 3} + } + } + + pub fun bar() { + let foo %s Foo() + foo.x%s + %s + } + } + `, valueKind.Keyword(), access.Keyword(), declaration.Keywords(), assignmentOp, member.Code, destroyStatement), ) if member.Mutating { @@ -639,24 +631,23 @@ func TestDictionaryUpdateMethodCall(t *testing.T) { func TestPubSetAccessModifier(t *testing.T) { t.Run("pub set dict", func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, ` - pub contract C { - pub struct Foo { - pub(set) var x: {Int: Int} - - init() { - self.x = {3: 3} - } - } - - pub fun bar() { - let foo = Foo() - foo.x[0] = 3 - } - } - `, - ParseAndCheckOptions{}, + pub contract C { + pub struct Foo { + pub(set) var x: {Int: Int} + + init() { + self.x = {3: 3} + } + } + + pub fun bar() { + let foo = Foo() + foo.x[0] = 3 + } + } + `, ) require.NoError(t, err) @@ -665,31 +656,30 @@ func TestPubSetAccessModifier(t *testing.T) { func TestPubSetNestedAccessModifier(t *testing.T) { t.Run("pub set nested", func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, ` - pub contract C { - pub struct Bar { - pub let foo: Foo - init() { - self.foo = Foo() - } - } - - pub struct Foo { - pub(set) var x: [Int] - - init() { - self.x = [3] - } - } - - pub fun bar() { - let bar = Bar() - bar.foo.x[0] = 3 - } - } - `, - ParseAndCheckOptions{}, + pub contract C { + pub struct Bar { + pub let foo: Foo + init() { + self.foo = Foo() + } + } + + pub struct Foo { + pub(set) var x: [Int] + + init() { + self.x = [3] + } + } + + pub fun bar() { + let bar = Bar() + bar.foo.x[0] = 3 + } + } + `, ) require.NoError(t, err) @@ -698,24 +688,23 @@ func TestPubSetNestedAccessModifier(t *testing.T) { func TestSelfContainingStruct(t *testing.T) { t.Run("pub let", func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, ` - pub contract C { - pub struct Foo { - pub let x: {Int: Int} - - init() { - self.x = {3: 3} - } - - pub fun bar() { - let foo = Foo() - foo.x[0] = 3 - } - } - } - `, - ParseAndCheckOptions{}, + pub contract C { + pub struct Foo { + pub let x: {Int: Int} + + init() { + self.x = {3: 3} + } + + pub fun bar() { + let foo = Foo() + foo.x[0] = 3 + } + } + } + `, ) require.NoError(t, err) @@ -724,28 +713,27 @@ func TestSelfContainingStruct(t *testing.T) { func TestMutationThroughReference(t *testing.T) { t.Run("pub let", func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, ` - pub fun main() { - let foo = Foo() - foo.ref.arr.append("y") - } - - pub struct Foo { - pub let ref: &Bar - init() { - self.ref = &Bar() as &Bar - } - } - - pub struct Bar { - pub let arr: [String] - init() { - self.arr = ["x"] - } - } - `, - ParseAndCheckOptions{}, + pub fun main() { + let foo = Foo() + foo.ref.arr.append("y") + } + + pub struct Foo { + pub let ref: &Bar + init() { + self.ref = &Bar() as &Bar + } + } + + pub struct Bar { + pub let arr: [String] + init() { + self.arr = ["x"] + } + } + `, ) errs := ExpectCheckerErrors(t, err, 1) var externalMutationError *sema.ExternalMutationError @@ -755,33 +743,32 @@ func TestMutationThroughReference(t *testing.T) { func TestMutationThroughAccess(t *testing.T) { t.Run("pub let", func(t *testing.T) { - _, err := ParseAndCheckWithOptions(t, + _, err := ParseAndCheck(t, ` - pub contract C { - pub struct Foo { - pub let arr: [Int] - init() { - self.arr = [3] - } - } - - priv let foo : Foo - - init() { - self.foo = Foo() - } - - pub fun getFoo(): Foo { - return self.foo - } - } - - pub fun main() { - let a = C.getFoo() - a.arr.append(0) // a.arr is now [3, 0] - } - `, - ParseAndCheckOptions{}, + pub contract C { + pub struct Foo { + pub let arr: [Int] + init() { + self.arr = [3] + } + } + + priv let foo : Foo + + init() { + self.foo = Foo() + } + + pub fun getFoo(): Foo { + return self.foo + } + } + + pub fun main() { + let a = C.getFoo() + a.arr.append(0) // a.arr is now [3, 0] + } + `, ) errs := ExpectCheckerErrors(t, err, 1) var externalMutationError *sema.ExternalMutationError From 937eb888c174afbb0d2bb2cbfc6ac4ecc3fd8886 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 25 Jan 2022 10:25:47 -0500 Subject: [PATCH 20/26] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Müller --- .../tests/checker/external_mutation_test.go | 84 +++++++++++++++---- 1 file changed, 68 insertions(+), 16 deletions(-) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index eea782afcd..6f24c9d95f 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -29,7 +29,7 @@ import ( "github.com/onflow/cadence/runtime/sema" ) -func TestArrayUpdateIndexAccess(t *testing.T) { +func TestCheckArrayUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -98,7 +98,7 @@ func TestArrayUpdateIndexAccess(t *testing.T) { } } -func TestDictionaryUpdateIndexAccess(t *testing.T) { +func TestCheckDictionaryUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -129,6 +129,9 @@ func TestDictionaryUpdateIndexAccess(t *testing.T) { } t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract C { @@ -164,7 +167,7 @@ func TestDictionaryUpdateIndexAccess(t *testing.T) { } } -func TestNestedArrayUpdateIndexAccess(t *testing.T) { +func TestCheckNestedArrayUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -183,6 +186,9 @@ func TestNestedArrayUpdateIndexAccess(t *testing.T) { testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract C { @@ -194,7 +200,7 @@ func TestNestedArrayUpdateIndexAccess(t *testing.T) { } pub struct Foo { - %s %s x : [Int] + %s %s x: [Int] init() { self.x = [3] @@ -222,7 +228,7 @@ func TestNestedArrayUpdateIndexAccess(t *testing.T) { } } -func TestNestedDictionaryUpdateIndexAccess(t *testing.T) { +func TestCheckNestedDictionaryUpdateIndexAccess(t *testing.T) { t.Parallel() @@ -241,6 +247,9 @@ func TestNestedDictionaryUpdateIndexAccess(t *testing.T) { testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract C { @@ -280,7 +289,7 @@ func TestNestedDictionaryUpdateIndexAccess(t *testing.T) { } } -func TestMutateContractIndexAccess(t *testing.T) { +func TestCheckMutateContractIndexAccess(t *testing.T) { t.Parallel() @@ -299,6 +308,9 @@ func TestMutateContractIndexAccess(t *testing.T) { testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract Foo { @@ -337,7 +349,7 @@ func TestMutateContractIndexAccess(t *testing.T) { } } -func TestContractNestedStructIndexAccess(t *testing.T) { +func TestCheckContractNestedStructIndexAccess(t *testing.T) { t.Parallel() @@ -356,6 +368,9 @@ func TestContractNestedStructIndexAccess(t *testing.T) { testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract Foo { @@ -401,7 +416,7 @@ func TestContractNestedStructIndexAccess(t *testing.T) { } } -func TestContractStructInitIndexAccess(t *testing.T) { +func TestCheckContractStructInitIndexAccess(t *testing.T) { t.Parallel() @@ -420,6 +435,9 @@ func TestContractStructInitIndexAccess(t *testing.T) { testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract Foo { @@ -453,7 +471,7 @@ func TestContractStructInitIndexAccess(t *testing.T) { } } -func TestArrayUpdateMethodCall(t *testing.T) { +func TestCheckArrayUpdateMethodCall(t *testing.T) { t.Parallel() @@ -502,6 +520,9 @@ func TestArrayUpdateMethodCall(t *testing.T) { } t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract C { @@ -543,7 +564,7 @@ func TestArrayUpdateMethodCall(t *testing.T) { } } -func TestDictionaryUpdateMethodCall(t *testing.T) { +func TestCheckDictionaryUpdateMethodCall(t *testing.T) { t.Parallel() @@ -588,6 +609,9 @@ func TestDictionaryUpdateMethodCall(t *testing.T) { } t.Run(testName, func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, fmt.Sprintf(` pub contract C { @@ -595,7 +619,7 @@ func TestDictionaryUpdateMethodCall(t *testing.T) { %s %s x: {Int: Int} init() { - self.x = {3: 3} + self.x = {3: 3} } } @@ -629,8 +653,13 @@ func TestDictionaryUpdateMethodCall(t *testing.T) { } } -func TestPubSetAccessModifier(t *testing.T) { +func TestCheckPubSetAccessModifier(t *testing.T) { + + t.Parallel() t.Run("pub set dict", func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, ` pub contract C { @@ -654,8 +683,13 @@ func TestPubSetAccessModifier(t *testing.T) { }) } -func TestPubSetNestedAccessModifier(t *testing.T) { +func TestCheckPubSetNestedAccessModifier(t *testing.T) { + + t.Parallel() t.Run("pub set nested", func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, ` pub contract C { @@ -686,8 +720,14 @@ func TestPubSetNestedAccessModifier(t *testing.T) { }) } -func TestSelfContainingStruct(t *testing.T) { +func TestCheckSelfContainingStruct(t *testing.T) { + + t.Parallel() + t.Run("pub let", func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, ` pub contract C { @@ -711,8 +751,14 @@ func TestSelfContainingStruct(t *testing.T) { }) } -func TestMutationThroughReference(t *testing.T) { +func TestCheckMutationThroughReference(t *testing.T) { + + t.Parallel() + t.Run("pub let", func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, ` pub fun main() { @@ -741,8 +787,14 @@ func TestMutationThroughReference(t *testing.T) { }) } -func TestMutationThroughAccess(t *testing.T) { +func TestCheckMutationThroughAccess(t *testing.T) { + + t.Parallel() + t.Run("pub let", func(t *testing.T) { + + t.Parallel() + _, err := ParseAndCheck(t, ` pub contract C { From 4ed47d37b46af31e3c399726c982a3842e8de671 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 25 Jan 2022 10:31:50 -0500 Subject: [PATCH 21/26] fix lint --- runtime/tests/checker/external_mutation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index 6f24c9d95f..1733ca9665 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -368,7 +368,7 @@ func TestCheckContractNestedStructIndexAccess(t *testing.T) { testName := fmt.Sprintf("%s %s", access.Keyword(), declaration.Keywords()) t.Run(testName, func(t *testing.T) { - + t.Parallel() _, err := ParseAndCheck(t, From a424ddadfdeb68e178726d542278e46b780d4959 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 1 Feb 2022 11:38:33 -0500 Subject: [PATCH 22/26] add test --- .../tests/checker/external_mutation_test.go | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/runtime/tests/checker/external_mutation_test.go b/runtime/tests/checker/external_mutation_test.go index 1733ca9665..e3d8073ec3 100644 --- a/runtime/tests/checker/external_mutation_test.go +++ b/runtime/tests/checker/external_mutation_test.go @@ -787,6 +787,41 @@ func TestCheckMutationThroughReference(t *testing.T) { }) } +func TestCheckMutationThroughInnerReference(t *testing.T) { + + t.Parallel() + + t.Run("pub let", func(t *testing.T) { + + t.Parallel() + + _, err := ParseAndCheck(t, + ` + pub fun main() { + let foo = Foo() + var arrayRef = &foo.ref.arr as &[String] + arrayRef[0] = "y" + } + + pub struct Foo { + pub let ref: &Bar + init() { + self.ref = &Bar() as &Bar + } + } + + pub struct Bar { + pub let arr: [String] + init() { + self.arr = ["x"] + } + } + `, + ) + require.NoError(t, err) + }) +} + func TestCheckMutationThroughAccess(t *testing.T) { t.Parallel() From 797ec728f6d38c1ea82cc05792ae5ba2eea12e1b Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 1 Feb 2022 12:54:12 -0500 Subject: [PATCH 23/26] add comments --- runtime/sema/check_assignment.go | 1 + runtime/sema/check_member_expression.go | 1 + 2 files changed, 2 insertions(+) diff --git a/runtime/sema/check_assignment.go b/runtime/sema/check_assignment.go index eed603732c..a4d89d120c 100644 --- a/runtime/sema/check_assignment.go +++ b/runtime/sema/check_assignment.go @@ -215,6 +215,7 @@ func (checker *Checker) visitIndexExpressionAssignment( targetExpression := target.TargetExpression switch targetExpression := targetExpression.(type) { case *ast.MemberExpression: + // calls to this method are cached, so this performs no computation _, member, _ := checker.visitMember(targetExpression) if !checker.isMutatableMember(member) { checker.report( diff --git a/runtime/sema/check_member_expression.go b/runtime/sema/check_member_expression.go index b19090a61f..1448138f9f 100644 --- a/runtime/sema/check_member_expression.go +++ b/runtime/sema/check_member_expression.go @@ -165,6 +165,7 @@ func (checker *Checker) visitMember(expression *ast.MemberExpression) (accessedT member = resolver.Resolve(identifier, targetRange, checker.report) switch targetExpression := accessedExpression.(type) { case *ast.MemberExpression: + // calls to this method are cached, so this performs no computation _, m, _ := checker.visitMember(targetExpression) if !checker.isMutatableMember(m) && resolver.Mutating { checker.report( From 1d6b9655b3f6cead6fde04b8aac4f4119bc4c955 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Wed, 2 Feb 2022 10:13:43 -0500 Subject: [PATCH 24/26] add test for public key immutability --- runtime/account_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/runtime/account_test.go b/runtime/account_test.go index ff02178b95..91a6e4fa35 100644 --- a/runtime/account_test.go +++ b/runtime/account_test.go @@ -1301,6 +1301,44 @@ func TestRuntimePublicKey(t *testing.T) { assert.IsType(t, &sema.ExternalMutationError{}, errs[0]) }) + t.Run("raw-key reference mutability", func(t *testing.T) { + script := ` + pub fun main(): PublicKey { + let publicKey = PublicKey( + publicKey: "0102".decodeHex(), + signatureAlgorithm: SignatureAlgorithm.ECDSA_P256 + ) + + var publickeyRef = &publicKey.publicKey as &[UInt8] + publickeyRef[0] = 3 + + return publicKey + } + ` + + storage := newTestLedger(nil, nil) + + runtimeInterface := &testRuntimeInterface{ + storage: storage, + } + + value, err := executeScript(script, runtimeInterface) + require.NoError(t, err) + + expected := cadence.Struct{ + StructType: PublicKeyType, + Fields: []cadence.Value{ + // Public key (bytes) + newBytesValue([]byte{1, 2}), + // Signature Algo + newSignAlgoValue(sema.SignatureAlgorithmECDSA_P256), + // valid + cadence.Bool(false), + }, + } + assert.Equal(t, expected, value) + }) + } func TestAuthAccountContracts(t *testing.T) { From 32d59bfb12485a65a1fe019f5befb29934f6dfe0 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Wed, 2 Feb 2022 10:19:12 -0500 Subject: [PATCH 25/26] add test for contract names immutability --- runtime/account_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/runtime/account_test.go b/runtime/account_test.go index 91a6e4fa35..0ab2d550b5 100644 --- a/runtime/account_test.go +++ b/runtime/account_test.go @@ -1431,6 +1431,45 @@ func TestAuthAccountContracts(t *testing.T) { assert.IsType(t, &sema.ExternalMutationError{}, errs[0]) }) + + t.Run("update names through reference", func(t *testing.T) { + t.Parallel() + + rt := newTestInterpreterRuntime() + + script := []byte(` + transaction { + prepare(signer: AuthAccount) { + var namesRef = &signer.contracts.names as &[String] + namesRef[0] = "baz" + + assert(signer.contracts.names[0] == "foo") + } + } + `) + + runtimeInterface := &testRuntimeInterface{ + getSigningAccounts: func() ([]Address, error) { + return []Address{{42}}, nil + }, + getAccountContractNames: func(_ Address) ([]string, error) { + return []string{"foo", "bar"}, nil + }, + } + + nextTransactionLocation := newTransactionLocationGenerator() + + err := rt.ExecuteTransaction( + Script{ + Source: script, + }, + Context{ + Interface: runtimeInterface, + Location: nextTransactionLocation(), + }, + ) + require.NoError(t, err) + }) } func TestPublicAccountContracts(t *testing.T) { From 55be0957b159acd625364708738018f69bfde885 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Wed, 2 Feb 2022 10:19:34 -0500 Subject: [PATCH 26/26] fix indentation --- runtime/account_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/runtime/account_test.go b/runtime/account_test.go index 0ab2d550b5..6e11fdcb5b 100644 --- a/runtime/account_test.go +++ b/runtime/account_test.go @@ -1305,14 +1305,14 @@ func TestRuntimePublicKey(t *testing.T) { script := ` pub fun main(): PublicKey { let publicKey = PublicKey( - publicKey: "0102".decodeHex(), - signatureAlgorithm: SignatureAlgorithm.ECDSA_P256 - ) + publicKey: "0102".decodeHex(), + signatureAlgorithm: SignatureAlgorithm.ECDSA_P256 + ) var publickeyRef = &publicKey.publicKey as &[UInt8] publickeyRef[0] = 3 - return publicKey + return publicKey } ` @@ -1441,9 +1441,9 @@ func TestAuthAccountContracts(t *testing.T) { transaction { prepare(signer: AuthAccount) { var namesRef = &signer.contracts.names as &[String] - namesRef[0] = "baz" + namesRef[0] = "baz" - assert(signer.contracts.names[0] == "foo") + assert(signer.contracts.names[0] == "foo") } } `)