Limit mutation of container-typed fields to the scope of the composite

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

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

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

runtime/runtime_test.go

@@ -1670,7 +1670,7 @@ func TestRuntimeStorageMultipleTransactionsResourceWithArray(t *testing.T) {
 
 	container := []byte(`
       pub resource Container {
-        pub let values: [Int]
+        pub(set) var values: [Int]
 
         init() {
           self.values = []

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
 	}

runtime/sema/check_member_expression.go

@@ -163,6 +163,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
@@ -312,6 +326,14 @@ 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.isWriteableAccess(member.Access) ||
+		checker.containerTypes[member.ContainerType]
+}
+
 // containingContractKindedType returns the containing contract-kinded type
 // of the given type, if any.
 //

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