From a1458029b11b64188d3c1a34bbe8b606f60e342b Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 14:24:11 -0400 Subject: [PATCH 1/9] add special support for a removedType pragma and prevent it from being removed once added --- runtime/ast/memberindices.go | 11 +++ runtime/ast/members.go | 4 + runtime/contract_update_validation_test.go | 83 ++++++++++++++++++++ runtime/stdlib/contract_update_validation.go | 74 +++++++++++++++++ 4 files changed, 172 insertions(+) diff --git a/runtime/ast/memberindices.go b/runtime/ast/memberindices.go index 93007a67c2..ca21bdba01 100644 --- a/runtime/ast/memberindices.go +++ b/runtime/ast/memberindices.go @@ -63,6 +63,8 @@ type memberIndices struct { _attachments []*AttachmentDeclaration // Use `EnumCases()` instead _enumCases []*EnumCaseDeclaration + // Use `Pragmas()` instead + _pragmas []*PragmaDeclaration } func (i *memberIndices) FieldsByIdentifier(declarations []Declaration) map[string]*FieldDeclaration { @@ -150,6 +152,11 @@ func (i *memberIndices) EnumCases(declarations []Declaration) []*EnumCaseDeclara return i._enumCases } +func (i *memberIndices) Pragmas(declarations []Declaration) []*PragmaDeclaration { + i.once.Do(i.initializer(declarations)) + return i._pragmas +} + func (i *memberIndices) initializer(declarations []Declaration) func() { return func() { i.init(declarations) @@ -184,6 +191,7 @@ func (i *memberIndices) init(declarations []Declaration) { i._entitlementMappingsByIdentifier = make(map[string]*EntitlementMappingDeclaration) i._enumCases = make([]*EnumCaseDeclaration, 0) + i._pragmas = make([]*PragmaDeclaration, 0) for _, declaration := range declarations { switch declaration := declaration.(type) { @@ -225,6 +233,9 @@ func (i *memberIndices) init(declarations []Declaration) { case *EnumCaseDeclaration: i._enumCases = append(i._enumCases, declaration) + + case *PragmaDeclaration: + i._pragmas = append(i._pragmas, declaration) } } } diff --git a/runtime/ast/members.go b/runtime/ast/members.go index a438d67566..30ebfd4773 100644 --- a/runtime/ast/members.go +++ b/runtime/ast/members.go @@ -84,6 +84,10 @@ func (m *Members) EnumCases() []*EnumCaseDeclaration { return m.indices.EnumCases(m.declarations) } +func (m *Members) Pragmas() []*PragmaDeclaration { + return m.indices.Pragmas(m.declarations) +} + func (m *Members) FieldsByIdentifier() map[string]*FieldDeclaration { return m.indices.FieldsByIdentifier(m.declarations) } diff --git a/runtime/contract_update_validation_test.go b/runtime/contract_update_validation_test.go index b8274f6523..da13c83d61 100644 --- a/runtime/contract_update_validation_test.go +++ b/runtime/contract_update_validation_test.go @@ -3060,3 +3060,86 @@ func TestRuntimeContractUpdateProgramCaching(t *testing.T) { ) }) } + +func TestPragmaUpdates(t *testing.T) { + t.Parallel() + + testWithValidators(t, "Remove pragma", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + #foo(bar) + #baz + } + ` + + const newCode = ` + access(all) contract Test { + #baz + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + require.NoError(t, err) + }) + + testWithValidators(t, "Remove removedType pragma", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + #removedType(bar) + #baz + } + ` + + const newCode = ` + access(all) contract Test { + #baz + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.TypeRemovalPragmaRemovalError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "malformed removedType pragma integer", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + #baz + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(3) + #baz + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.InvalidTypeRemovalPragmaError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "malformed removedType qualified name", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + #baz + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(X.Y) + #baz + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.InvalidTypeRemovalPragmaError + require.ErrorAs(t, err, &expectedErr) + }) +} diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index 9831e7a6f2..fe9fed7b3d 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -20,6 +20,7 @@ package stdlib import ( "fmt" + "slices" "sort" "github.com/onflow/cadence/runtime/ast" @@ -215,6 +216,30 @@ func (validator *ContractUpdateValidator) hasErrors() bool { return len(validator.errors) > 0 } +func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaDeclaration) (removedTypes []*ast.NominalType) { + for _, pragma := range pragmas { + invocationExpression, isInvocation := pragma.Expression.(*ast.InvocationExpression) + if !isInvocation { + continue + } + invokedIdentifier, isIdentifier := invocationExpression.InvokedExpression.(*ast.IdentifierExpression) + if !isIdentifier || invokedIdentifier.Identifier.Identifier != "removedType" { + continue + } + removedTypeName, isIdentifer := invocationExpression.Arguments[0].Expression.(*ast.IdentifierExpression) + if !isIdentifer { + validator.report(&InvalidTypeRemovalPragmaError{ + Expression: pragma.Expression, + Range: ast.NewUnmeteredRangeFromPositioned(pragma.Expression), + }) + continue + } + removedTypes = append(removedTypes, ast.NewNominalType(nil, removedTypeName.Identifier, []ast.Identifier{})) + } + + return removedTypes +} + func checkDeclarationUpdatability( validator UpdateValidator, oldDeclaration ast.Declaration, @@ -341,6 +366,20 @@ func checkNestedDeclarations( checkConformance checkConformanceFunc, ) { + // process pragmas first, as they determine whether types can later be removed + oldRemovedTypes := collectRemovedTypePragmas(validator, oldDeclaration.DeclarationMembers().Pragmas()) + removedTypes := collectRemovedTypePragmas(validator, newDeclaration.DeclarationMembers().Pragmas()) + + // #typeRemoval pragmas cannot be removed, so any that appear in the old program must appear in the new program + // they can however, be added, so use the new program's type removals for the purposes of checking the upgrade + for _, oldRemovedType := range oldRemovedTypes { + if !slices.Contains(removedTypes, oldRemovedType) { + validator.report(&TypeRemovalPragmaRemovalError{ + RemovedType: oldRemovedType, + }) + } + } + oldNominalTypeDecls := getNestedNominalTypeDecls(oldDeclaration) // Check nested structs, enums, etc. @@ -774,3 +813,38 @@ func (e *MissingDeclarationError) Error() string { e.Name, ) } + +// InvalidTypeRemovalPragmaError is reported during a contract update +// if a malformed #removedType pragma is encountered +type InvalidTypeRemovalPragmaError struct { + Expression ast.Expression + ast.Range +} + +var _ errors.UserError = &InvalidTypeRemovalPragmaError{} + +func (*InvalidTypeRemovalPragmaError) IsUserError() {} + +func (e *InvalidTypeRemovalPragmaError) Error() string { + return fmt.Sprintf( + "invalid #typeRemoval pragma: %s", + e.Expression.String(), + ) +} + +// TypeRemovalPragmaRemovalError is reported during a contract update +// if a #removedType pragma is removed +type TypeRemovalPragmaRemovalError struct { + RemovedType *ast.NominalType +} + +var _ errors.UserError = &TypeRemovalPragmaRemovalError{} + +func (*TypeRemovalPragmaRemovalError) IsUserError() {} + +func (e *TypeRemovalPragmaRemovalError) Error() string { + return fmt.Sprintf( + "missing #removedType pragma for %s", + e.RemovedType.String(), + ) +} From f3556faedaf4904339d321ca3d901b9d9c4d5284 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 14:37:04 -0400 Subject: [PATCH 2/9] #removedType allows removing a type --- runtime/contract_update_validation_test.go | 142 ++++++++++++++---- ..._v0.42_to_v1_contract_upgrade_validator.go | 2 + runtime/stdlib/contract_update_validation.go | 22 ++- 3 files changed, 129 insertions(+), 37 deletions(-) diff --git a/runtime/contract_update_validation_test.go b/runtime/contract_update_validation_test.go index da13c83d61..a69ef1b991 100644 --- a/runtime/contract_update_validation_test.go +++ b/runtime/contract_update_validation_test.go @@ -3066,80 +3066,158 @@ func TestPragmaUpdates(t *testing.T) { testWithValidators(t, "Remove pragma", func(t *testing.T, withC1Upgrade bool) { + const oldCode = ` + access(all) contract Test { + #foo(bar) + #baz + } + ` + + const newCode = ` + access(all) contract Test { + #baz + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + require.NoError(t, err) + }) + + testWithValidators(t, "Remove removedType pragma", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + #removedType(bar) + #baz + } + ` + + const newCode = ` + access(all) contract Test { + #baz + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.TypeRemovalPragmaRemovalError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "reorder removedType pragmas", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + #removedType(bar) + #removedType(foo) + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(foo) + #removedType(bar) + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + require.NoError(t, err) + }) + + testWithValidators(t, "malformed removedType pragma integer", func(t *testing.T, withC1Upgrade bool) { + const oldCode = ` access(all) contract Test { - #foo(bar) #baz } ` const newCode = ` access(all) contract Test { + #removedType(3) #baz } ` err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) - require.NoError(t, err) + var expectedErr *stdlib.InvalidTypeRemovalPragmaError + require.ErrorAs(t, err, &expectedErr) }) - testWithValidators(t, "Remove removedType pragma", func(t *testing.T, withC1Upgrade bool) { + testWithValidators(t, "malformed removedType qualified name", func(t *testing.T, withC1Upgrade bool) { const oldCode = ` - access(all) contract Test { - #removedType(bar) + access(all) contract Test { + #baz + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(X.Y) #baz + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.InvalidTypeRemovalPragmaError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "#removedType allows type removal", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + access(all) resource R {} } ` const newCode = ` access(all) contract Test { - #baz + #removedType(R) } ` err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) - var expectedErr *stdlib.TypeRemovalPragmaRemovalError - require.ErrorAs(t, err, &expectedErr) + require.NoError(t, err) }) - testWithValidators(t, "malformed removedType pragma integer", func(t *testing.T, withC1Upgrade bool) { + testWithValidators(t, "#removedType allows two type removals", func(t *testing.T, withC1Upgrade bool) { const oldCode = ` - access(all) contract Test { - #baz - } - ` + access(all) contract Test { + access(all) resource R {} + access(all) struct interface I {} + } + ` const newCode = ` - access(all) contract Test { - #removedType(3) - #baz - } - ` + access(all) contract Test { + #removedType(R) + #removedType(I) + } + ` err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) - var expectedErr *stdlib.InvalidTypeRemovalPragmaError - require.ErrorAs(t, err, &expectedErr) + require.NoError(t, err) }) - testWithValidators(t, "malformed removedType qualified name", func(t *testing.T, withC1Upgrade bool) { + testWithValidators(t, "#removedType can be added", func(t *testing.T, withC1Upgrade bool) { const oldCode = ` - access(all) contract Test { - #baz - } - ` + access(all) contract Test { + #removedType(I) + access(all) resource R {} + } + ` const newCode = ` - access(all) contract Test { - #removedType(X.Y) - #baz - } - ` + access(all) contract Test { + #removedType(R) + #removedType(I) + } + ` err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) - var expectedErr *stdlib.InvalidTypeRemovalPragmaError - require.ErrorAs(t, err, &expectedErr) + require.NoError(t, err) }) } diff --git a/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go b/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go index b05311f968..1a28c69579 100644 --- a/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go +++ b/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go @@ -741,6 +741,7 @@ func (validator *CadenceV042ToV1ContractUpdateValidator) checkNestedDeclarationR nestedDeclaration ast.Declaration, oldContainingDeclaration ast.Declaration, newContainingDeclaration ast.Declaration, + removedTypes []ast.Identifier, ) { // enums can be removed from contract interfaces, as they have no interface equivalent and are not @@ -755,6 +756,7 @@ func (validator *CadenceV042ToV1ContractUpdateValidator) checkNestedDeclarationR nestedDeclaration, oldContainingDeclaration, newContainingDeclaration, + removedTypes, ) } diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index fe9fed7b3d..16bdff3ba8 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -42,6 +42,7 @@ type UpdateValidator interface { nestedDeclaration ast.Declaration, oldContainingDeclaration ast.Declaration, newContainingDeclaration ast.Declaration, + removedTypes []ast.Identifier, ) getAccountContractNames(address common.Address) ([]string, error) @@ -216,7 +217,7 @@ func (validator *ContractUpdateValidator) hasErrors() bool { return len(validator.errors) > 0 } -func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaDeclaration) (removedTypes []*ast.NominalType) { +func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaDeclaration) (removedTypes []ast.Identifier) { for _, pragma := range pragmas { invocationExpression, isInvocation := pragma.Expression.(*ast.InvocationExpression) if !isInvocation { @@ -234,7 +235,7 @@ func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaD }) continue } - removedTypes = append(removedTypes, ast.NewNominalType(nil, removedTypeName.Identifier, []ast.Identifier{})) + removedTypes = append(removedTypes, removedTypeName.Identifier) } return removedTypes @@ -335,12 +336,21 @@ func (validator *ContractUpdateValidator) checkNestedDeclarationRemoval( nestedDeclaration ast.Declaration, _ ast.Declaration, newContainingDeclaration ast.Declaration, + removedTypes []ast.Identifier, ) { // OK to remove events - they are not stored if nestedDeclaration.DeclarationKind() == common.DeclarationKindEvent { return } + // OK to remove a type if it is included in a #removedType pragma + if nestedDeclaration.DeclarationIdentifier() != nil && + slices.ContainsFunc(removedTypes, func(removedType ast.Identifier) bool { + return removedType.Identifier == nestedDeclaration.DeclarationIdentifier().Identifier + }) { + return + } + validator.report(&MissingDeclarationError{ Name: nestedDeclaration.DeclarationIdentifier().Identifier, Kind: nestedDeclaration.DeclarationKind(), @@ -373,7 +383,9 @@ func checkNestedDeclarations( // #typeRemoval pragmas cannot be removed, so any that appear in the old program must appear in the new program // they can however, be added, so use the new program's type removals for the purposes of checking the upgrade for _, oldRemovedType := range oldRemovedTypes { - if !slices.Contains(removedTypes, oldRemovedType) { + if !slices.ContainsFunc(removedTypes, func(newRemovedType ast.Identifier) bool { + return newRemovedType.Identifier == oldRemovedType.Identifier + }) { validator.report(&TypeRemovalPragmaRemovalError{ RemovedType: oldRemovedType, }) @@ -443,7 +455,7 @@ func checkNestedDeclarations( }) for _, declaration := range missingDeclarations { - validator.checkNestedDeclarationRemoval(declaration, oldDeclaration, newDeclaration) + validator.checkNestedDeclarationRemoval(declaration, oldDeclaration, newDeclaration, removedTypes) } // Check enum-cases, if there are any. @@ -835,7 +847,7 @@ func (e *InvalidTypeRemovalPragmaError) Error() string { // TypeRemovalPragmaRemovalError is reported during a contract update // if a #removedType pragma is removed type TypeRemovalPragmaRemovalError struct { - RemovedType *ast.NominalType + RemovedType ast.Identifier } var _ errors.UserError = &TypeRemovalPragmaRemovalError{} From fac26462b220568915b63c1d83fee3bd4b71311e Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 14:54:26 -0400 Subject: [PATCH 3/9] types cannot be used when they are removed --- runtime/contract_update_validation_test.go | 99 ++++++++++++++++++++ runtime/stdlib/contract_update_validation.go | 38 +++++++- 2 files changed, 136 insertions(+), 1 deletion(-) diff --git a/runtime/contract_update_validation_test.go b/runtime/contract_update_validation_test.go index a69ef1b991..99dea77c20 100644 --- a/runtime/contract_update_validation_test.go +++ b/runtime/contract_update_validation_test.go @@ -3220,4 +3220,103 @@ func TestPragmaUpdates(t *testing.T) { err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) require.NoError(t, err) }) + + testWithValidators(t, "#removedType must remove a type", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(X) + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + require.NoError(t, err) + }) + + testWithValidators(t, "declarations cannot co-exist with removed type of the same name, composite", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + access(all) resource R {} + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(R) + access(all) resource R {} + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.UseOfRemovedTypeError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "declarations cannot co-exist with removed type of the same name, interface", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + access(all) resource interface R {} + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(R) + access(all) resource interface R {} + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.UseOfRemovedTypeError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "declarations cannot co-exist with removed type of the same name, attachment", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + access(all) attachment R for AnyResource {} + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(R) + access(all) attachment R for AnyResource {} + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.UseOfRemovedTypeError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "#removedType is only scoped to the current declaration, inner", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + access(all) resource R {} + access(all) struct S {} + } + ` + + const newCode = ` + access(all) contract Test { + access(all) struct S { + #removedType(R) + } + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.MissingDeclarationError + require.ErrorAs(t, err, &expectedErr) + }) } diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index 16bdff3ba8..2db003329c 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -369,6 +369,21 @@ func (validator *ContractUpdateValidator) oldTypeID(oldType *ast.NominalType) co return oldImportLocation.TypeID(nil, qualifiedIdentifier) } +func checkTypeNotRemoved( + validator UpdateValidator, + newDeclaration ast.Declaration, + removedTypes []ast.Identifier, +) { + if slices.ContainsFunc(removedTypes, func(removedType ast.Identifier) bool { + return removedType.Identifier == newDeclaration.DeclarationIdentifier().Identifier + }) { + validator.report(&UseOfRemovedTypeError{ + Declaration: newDeclaration, + Range: ast.NewUnmeteredRangeFromPositioned(newDeclaration), + }) + } +} + func checkNestedDeclarations( validator UpdateValidator, oldDeclaration ast.Declaration, @@ -397,6 +412,7 @@ func checkNestedDeclarations( // Check nested structs, enums, etc. newNestedCompositeDecls := newDeclaration.DeclarationMembers().Composites() for _, newNestedDecl := range newNestedCompositeDecls { + checkTypeNotRemoved(validator, newNestedDecl, removedTypes) oldNestedDecl, found := oldNominalTypeDecls[newNestedDecl.Identifier.Identifier] if !found { // Then it's a new declaration @@ -412,6 +428,7 @@ func checkNestedDeclarations( // Check nested attachments, etc. newNestedAttachmentDecls := newDeclaration.DeclarationMembers().Attachments() for _, newNestedDecl := range newNestedAttachmentDecls { + checkTypeNotRemoved(validator, newNestedDecl, removedTypes) oldNestedDecl, found := oldNominalTypeDecls[newNestedDecl.Identifier.Identifier] if !found { // Then it's a new declaration @@ -427,6 +444,7 @@ func checkNestedDeclarations( // Check nested interfaces. newNestedInterfaces := newDeclaration.DeclarationMembers().Interfaces() for _, newNestedDecl := range newNestedInterfaces { + checkTypeNotRemoved(validator, newNestedDecl, removedTypes) oldNestedDecl, found := oldNominalTypeDecls[newNestedDecl.Identifier.Identifier] if !found { // Then this is a new declaration. @@ -839,11 +857,29 @@ func (*InvalidTypeRemovalPragmaError) IsUserError() {} func (e *InvalidTypeRemovalPragmaError) Error() string { return fmt.Sprintf( - "invalid #typeRemoval pragma: %s", + "invalid #removedType pragma: %s", e.Expression.String(), ) } +// UseOfRemovedTypeError is reported during a contract update +// if a type is encountered that is also in a #removedType pragma +type UseOfRemovedTypeError struct { + Declaration ast.Declaration + ast.Range +} + +var _ errors.UserError = &UseOfRemovedTypeError{} + +func (*UseOfRemovedTypeError) IsUserError() {} + +func (e *UseOfRemovedTypeError) Error() string { + return fmt.Sprintf( + "cannot declare %s, type has been removed with a #removedType pragma", + e.Declaration.DeclarationIdentifier(), + ) +} + // TypeRemovalPragmaRemovalError is reported during a contract update // if a #removedType pragma is removed type TypeRemovalPragmaRemovalError struct { From 846408618d57121271f4b5a8e0bc60097893cfb3 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 15:34:14 -0400 Subject: [PATCH 4/9] add tests --- runtime/contract_update_validation_test.go | 26 +++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/runtime/contract_update_validation_test.go b/runtime/contract_update_validation_test.go index 99dea77c20..b214349c2e 100644 --- a/runtime/contract_update_validation_test.go +++ b/runtime/contract_update_validation_test.go @@ -3103,6 +3103,30 @@ func TestPragmaUpdates(t *testing.T) { require.ErrorAs(t, err, &expectedErr) }) + testWithValidators(t, "removedType pragma moved into subdeclaration", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + #removedType(bar) + access(all) struct S { + + } + } + ` + + const newCode = ` + access(all) contract Test { + access(all) struct S { + #removedType(bar) + } + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.TypeRemovalPragmaRemovalError + require.ErrorAs(t, err, &expectedErr) + }) + testWithValidators(t, "reorder removedType pragmas", func(t *testing.T, withC1Upgrade bool) { const oldCode = ` @@ -3221,7 +3245,7 @@ func TestPragmaUpdates(t *testing.T) { require.NoError(t, err) }) - testWithValidators(t, "#removedType must remove a type", func(t *testing.T, withC1Upgrade bool) { + testWithValidators(t, "#removedType can be added without removing a type", func(t *testing.T, withC1Upgrade bool) { const oldCode = ` access(all) contract Test { From 21aa55372cae248602c8ca98a02c77fd18074b28 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 15:59:02 -0400 Subject: [PATCH 5/9] fix slices imports --- go.mod | 1 + go.sum | 2 ++ runtime/stdlib/contract_update_validation.go | 3 ++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index e94021232c..9933baeeaf 100644 --- a/go.mod +++ b/go.mod @@ -55,6 +55,7 @@ require ( github.com/x448/float16 v0.8.4 // indirect github.com/zeebo/assert v1.3.0 // indirect github.com/zeebo/blake3 v0.2.3 // indirect + golang.org/dl v0.0.0-20240507154152-429173cd35bf // indirect golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect golang.org/x/sys v0.15.0 // indirect golang.org/x/term v0.6.0 // indirect diff --git a/go.sum b/go.sum index 31a4dd9659..c01991ad41 100644 --- a/go.sum +++ b/go.sum @@ -114,6 +114,8 @@ go.opentelemetry.io/otel v1.8.0 h1:zcvBFizPbpa1q7FehvFiHbQwGzmPILebO0tyqIR5Djg= go.opentelemetry.io/otel v1.8.0/go.mod h1:2pkj+iMj0o03Y+cW6/m8Y4WkRdYN3AvCXCnzRMp9yvM= go.uber.org/goleak v1.1.10 h1:z+mqJhf6ss6BSfSM671tgKyZBFPTTJM+HLxnhPC3wu0= go.uber.org/goleak v1.1.10/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A= +golang.org/dl v0.0.0-20240507154152-429173cd35bf h1:NcElu4L8vb3Sb/imeHGuw1/sbpVW8JdOmFx+BZFbkOM= +golang.org/dl v0.0.0-20240507154152-429173cd35bf/go.mod h1:fwQ+hlTD8I6TIzOGkQqxQNfE2xqR+y7SzGaDkksVFkw= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.1.0 h1:MDRAIl0xIo9Io2xV565hzXHw3zVseKrJKodhohM5CjU= diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index 2db003329c..a638483a73 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -20,9 +20,10 @@ package stdlib import ( "fmt" - "slices" "sort" + "golang.org/x/exp/slices" + "github.com/onflow/cadence/runtime/ast" "github.com/onflow/cadence/runtime/common" "github.com/onflow/cadence/runtime/errors" From 76f8841ac732ca8c5158a9c29c6a85c8a857e7a3 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 16:21:55 -0400 Subject: [PATCH 6/9] respond to review --- ..._v0.42_to_v1_contract_upgrade_validator.go | 2 +- runtime/stdlib/contract_update_validation.go | 35 ++++++++----------- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go b/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go index 1a28c69579..a3a14ffa3a 100644 --- a/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go +++ b/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go @@ -741,7 +741,7 @@ func (validator *CadenceV042ToV1ContractUpdateValidator) checkNestedDeclarationR nestedDeclaration ast.Declaration, oldContainingDeclaration ast.Declaration, newContainingDeclaration ast.Declaration, - removedTypes []ast.Identifier, + removedTypes map[string]struct{}, ) { // enums can be removed from contract interfaces, as they have no interface equivalent and are not diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index a638483a73..0daefcc9a1 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -22,8 +22,6 @@ import ( "fmt" "sort" - "golang.org/x/exp/slices" - "github.com/onflow/cadence/runtime/ast" "github.com/onflow/cadence/runtime/common" "github.com/onflow/cadence/runtime/errors" @@ -43,7 +41,7 @@ type UpdateValidator interface { nestedDeclaration ast.Declaration, oldContainingDeclaration ast.Declaration, newContainingDeclaration ast.Declaration, - removedTypes []ast.Identifier, + removedTypes map[string]struct{}, ) getAccountContractNames(address common.Address) ([]string, error) @@ -218,10 +216,12 @@ func (validator *ContractUpdateValidator) hasErrors() bool { return len(validator.errors) > 0 } -func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaDeclaration) (removedTypes []ast.Identifier) { +func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaDeclaration) map[string]struct{} { + removedTypes := map[string]struct{}{} + for _, pragma := range pragmas { invocationExpression, isInvocation := pragma.Expression.(*ast.InvocationExpression) - if !isInvocation { + if !isInvocation || len(invocationExpression.Arguments) != 1 { continue } invokedIdentifier, isIdentifier := invocationExpression.InvokedExpression.(*ast.IdentifierExpression) @@ -236,7 +236,7 @@ func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaD }) continue } - removedTypes = append(removedTypes, removedTypeName.Identifier) + removedTypes[removedTypeName.Identifier.Identifier] = struct{}{} } return removedTypes @@ -337,7 +337,7 @@ func (validator *ContractUpdateValidator) checkNestedDeclarationRemoval( nestedDeclaration ast.Declaration, _ ast.Declaration, newContainingDeclaration ast.Declaration, - removedTypes []ast.Identifier, + removedTypes map[string]struct{}, ) { // OK to remove events - they are not stored if nestedDeclaration.DeclarationKind() == common.DeclarationKindEvent { @@ -345,10 +345,7 @@ func (validator *ContractUpdateValidator) checkNestedDeclarationRemoval( } // OK to remove a type if it is included in a #removedType pragma - if nestedDeclaration.DeclarationIdentifier() != nil && - slices.ContainsFunc(removedTypes, func(removedType ast.Identifier) bool { - return removedType.Identifier == nestedDeclaration.DeclarationIdentifier().Identifier - }) { + if _, present := removedTypes[nestedDeclaration.DeclarationIdentifier().Identifier]; present { return } @@ -373,11 +370,9 @@ func (validator *ContractUpdateValidator) oldTypeID(oldType *ast.NominalType) co func checkTypeNotRemoved( validator UpdateValidator, newDeclaration ast.Declaration, - removedTypes []ast.Identifier, + removedTypes map[string]struct{}, ) { - if slices.ContainsFunc(removedTypes, func(removedType ast.Identifier) bool { - return removedType.Identifier == newDeclaration.DeclarationIdentifier().Identifier - }) { + if _, present := removedTypes[newDeclaration.DeclarationIdentifier().Identifier]; present { validator.report(&UseOfRemovedTypeError{ Declaration: newDeclaration, Range: ast.NewUnmeteredRangeFromPositioned(newDeclaration), @@ -398,10 +393,8 @@ func checkNestedDeclarations( // #typeRemoval pragmas cannot be removed, so any that appear in the old program must appear in the new program // they can however, be added, so use the new program's type removals for the purposes of checking the upgrade - for _, oldRemovedType := range oldRemovedTypes { - if !slices.ContainsFunc(removedTypes, func(newRemovedType ast.Identifier) bool { - return newRemovedType.Identifier == oldRemovedType.Identifier - }) { + for oldRemovedType, _ := range oldRemovedTypes { //nolint:maprange + if _, present := removedTypes[oldRemovedType]; !present { validator.report(&TypeRemovalPragmaRemovalError{ RemovedType: oldRemovedType, }) @@ -884,7 +877,7 @@ func (e *UseOfRemovedTypeError) Error() string { // TypeRemovalPragmaRemovalError is reported during a contract update // if a #removedType pragma is removed type TypeRemovalPragmaRemovalError struct { - RemovedType ast.Identifier + RemovedType string } var _ errors.UserError = &TypeRemovalPragmaRemovalError{} @@ -894,6 +887,6 @@ func (*TypeRemovalPragmaRemovalError) IsUserError() {} func (e *TypeRemovalPragmaRemovalError) Error() string { return fmt.Sprintf( "missing #removedType pragma for %s", - e.RemovedType.String(), + e.RemovedType, ) } From 2cc245a3cf5a8ee5156dfc65e4c3fa87c911e145 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 16:31:59 -0400 Subject: [PATCH 7/9] respond to review --- go.mod | 1 - go.sum | 2 -- runtime/stdlib/contract_update_validation.go | 7 +++++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 9933baeeaf..e94021232c 100644 --- a/go.mod +++ b/go.mod @@ -55,7 +55,6 @@ require ( github.com/x448/float16 v0.8.4 // indirect github.com/zeebo/assert v1.3.0 // indirect github.com/zeebo/blake3 v0.2.3 // indirect - golang.org/dl v0.0.0-20240507154152-429173cd35bf // indirect golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect golang.org/x/sys v0.15.0 // indirect golang.org/x/term v0.6.0 // indirect diff --git a/go.sum b/go.sum index c01991ad41..31a4dd9659 100644 --- a/go.sum +++ b/go.sum @@ -114,8 +114,6 @@ go.opentelemetry.io/otel v1.8.0 h1:zcvBFizPbpa1q7FehvFiHbQwGzmPILebO0tyqIR5Djg= go.opentelemetry.io/otel v1.8.0/go.mod h1:2pkj+iMj0o03Y+cW6/m8Y4WkRdYN3AvCXCnzRMp9yvM= go.uber.org/goleak v1.1.10 h1:z+mqJhf6ss6BSfSM671tgKyZBFPTTJM+HLxnhPC3wu0= go.uber.org/goleak v1.1.10/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A= -golang.org/dl v0.0.0-20240507154152-429173cd35bf h1:NcElu4L8vb3Sb/imeHGuw1/sbpVW8JdOmFx+BZFbkOM= -golang.org/dl v0.0.0-20240507154152-429173cd35bf/go.mod h1:fwQ+hlTD8I6TIzOGkQqxQNfE2xqR+y7SzGaDkksVFkw= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.1.0 h1:MDRAIl0xIo9Io2xV565hzXHw3zVseKrJKodhohM5CjU= diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index 0daefcc9a1..df1830fa7a 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -228,6 +228,13 @@ func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaD if !isIdentifier || invokedIdentifier.Identifier.Identifier != "removedType" { continue } + if len(invocationExpression.Arguments) != 1 { + validator.report(&InvalidTypeRemovalPragmaError{ + Expression: pragma.Expression, + Range: ast.NewUnmeteredRangeFromPositioned(pragma.Expression), + }) + continue + } removedTypeName, isIdentifer := invocationExpression.Arguments[0].Expression.(*ast.IdentifierExpression) if !isIdentifer { validator.report(&InvalidTypeRemovalPragmaError{ From f9079b35a535ce7707be5e66a6419109d8c4f2a8 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 16:57:40 -0400 Subject: [PATCH 8/9] use ordered map --- ..._v0.42_to_v1_contract_upgrade_validator.go | 3 ++- runtime/stdlib/contract_update_validation.go | 23 ++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go b/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go index a3a14ffa3a..867026a8d6 100644 --- a/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go +++ b/runtime/stdlib/cadence_v0.42_to_v1_contract_upgrade_validator.go @@ -23,6 +23,7 @@ import ( "github.com/onflow/cadence/runtime/ast" "github.com/onflow/cadence/runtime/common" + "github.com/onflow/cadence/runtime/common/orderedmap" "github.com/onflow/cadence/runtime/errors" "github.com/onflow/cadence/runtime/interpreter" "github.com/onflow/cadence/runtime/sema" @@ -741,7 +742,7 @@ func (validator *CadenceV042ToV1ContractUpdateValidator) checkNestedDeclarationR nestedDeclaration ast.Declaration, oldContainingDeclaration ast.Declaration, newContainingDeclaration ast.Declaration, - removedTypes map[string]struct{}, + removedTypes *orderedmap.OrderedMap[string, struct{}], ) { // enums can be removed from contract interfaces, as they have no interface equivalent and are not diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index df1830fa7a..b2b60f1331 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -24,6 +24,7 @@ import ( "github.com/onflow/cadence/runtime/ast" "github.com/onflow/cadence/runtime/common" + "github.com/onflow/cadence/runtime/common/orderedmap" "github.com/onflow/cadence/runtime/errors" ) @@ -41,7 +42,7 @@ type UpdateValidator interface { nestedDeclaration ast.Declaration, oldContainingDeclaration ast.Declaration, newContainingDeclaration ast.Declaration, - removedTypes map[string]struct{}, + removedTypes *orderedmap.OrderedMap[string, struct{}], ) getAccountContractNames(address common.Address) ([]string, error) @@ -216,8 +217,8 @@ func (validator *ContractUpdateValidator) hasErrors() bool { return len(validator.errors) > 0 } -func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaDeclaration) map[string]struct{} { - removedTypes := map[string]struct{}{} +func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaDeclaration) *orderedmap.OrderedMap[string, struct{}] { + removedTypes := orderedmap.New[orderedmap.OrderedMap[string, struct{}]](len(pragmas)) for _, pragma := range pragmas { invocationExpression, isInvocation := pragma.Expression.(*ast.InvocationExpression) @@ -243,7 +244,7 @@ func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaD }) continue } - removedTypes[removedTypeName.Identifier.Identifier] = struct{}{} + removedTypes.Set(removedTypeName.Identifier.Identifier, struct{}{}) } return removedTypes @@ -344,7 +345,7 @@ func (validator *ContractUpdateValidator) checkNestedDeclarationRemoval( nestedDeclaration ast.Declaration, _ ast.Declaration, newContainingDeclaration ast.Declaration, - removedTypes map[string]struct{}, + removedTypes *orderedmap.OrderedMap[string, struct{}], ) { // OK to remove events - they are not stored if nestedDeclaration.DeclarationKind() == common.DeclarationKindEvent { @@ -352,7 +353,7 @@ func (validator *ContractUpdateValidator) checkNestedDeclarationRemoval( } // OK to remove a type if it is included in a #removedType pragma - if _, present := removedTypes[nestedDeclaration.DeclarationIdentifier().Identifier]; present { + if removedTypes.Contains(nestedDeclaration.DeclarationIdentifier().Identifier) { return } @@ -377,9 +378,9 @@ func (validator *ContractUpdateValidator) oldTypeID(oldType *ast.NominalType) co func checkTypeNotRemoved( validator UpdateValidator, newDeclaration ast.Declaration, - removedTypes map[string]struct{}, + removedTypes *orderedmap.OrderedMap[string, struct{}], ) { - if _, present := removedTypes[newDeclaration.DeclarationIdentifier().Identifier]; present { + if removedTypes.Contains(newDeclaration.DeclarationIdentifier().Identifier) { validator.report(&UseOfRemovedTypeError{ Declaration: newDeclaration, Range: ast.NewUnmeteredRangeFromPositioned(newDeclaration), @@ -400,13 +401,13 @@ func checkNestedDeclarations( // #typeRemoval pragmas cannot be removed, so any that appear in the old program must appear in the new program // they can however, be added, so use the new program's type removals for the purposes of checking the upgrade - for oldRemovedType, _ := range oldRemovedTypes { //nolint:maprange - if _, present := removedTypes[oldRemovedType]; !present { + oldRemovedTypes.Foreach(func(oldRemovedType string, _ struct{}) { + if !removedTypes.Contains(oldRemovedType) { validator.report(&TypeRemovalPragmaRemovalError{ RemovedType: oldRemovedType, }) } - } + }) oldNominalTypeDecls := getNestedNominalTypeDecls(oldDeclaration) From f7982223d955e2cbce437a8c44f20e4eace78be6 Mon Sep 17 00:00:00 2001 From: Daniel Sainati Date: Tue, 28 May 2024 17:22:10 -0400 Subject: [PATCH 9/9] add tests --- runtime/contract_update_validation_test.go | 36 ++++++++++++++++++++ runtime/stdlib/contract_update_validation.go | 2 +- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/runtime/contract_update_validation_test.go b/runtime/contract_update_validation_test.go index b214349c2e..e747a87682 100644 --- a/runtime/contract_update_validation_test.go +++ b/runtime/contract_update_validation_test.go @@ -3187,6 +3187,42 @@ func TestPragmaUpdates(t *testing.T) { require.ErrorAs(t, err, &expectedErr) }) + testWithValidators(t, "removedType with zero args", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + } + ` + + const newCode = ` + access(all) contract Test { + #removedType() + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.InvalidTypeRemovalPragmaError + require.ErrorAs(t, err, &expectedErr) + }) + + testWithValidators(t, "removedType with two args", func(t *testing.T, withC1Upgrade bool) { + + const oldCode = ` + access(all) contract Test { + } + ` + + const newCode = ` + access(all) contract Test { + #removedType(x, y) + } + ` + + err := testDeployAndUpdate(t, "Test", oldCode, newCode, withC1Upgrade) + var expectedErr *stdlib.InvalidTypeRemovalPragmaError + require.ErrorAs(t, err, &expectedErr) + }) + testWithValidators(t, "#removedType allows type removal", func(t *testing.T, withC1Upgrade bool) { const oldCode = ` diff --git a/runtime/stdlib/contract_update_validation.go b/runtime/stdlib/contract_update_validation.go index b2b60f1331..d43840d9dd 100644 --- a/runtime/stdlib/contract_update_validation.go +++ b/runtime/stdlib/contract_update_validation.go @@ -222,7 +222,7 @@ func collectRemovedTypePragmas(validator UpdateValidator, pragmas []*ast.PragmaD for _, pragma := range pragmas { invocationExpression, isInvocation := pragma.Expression.(*ast.InvocationExpression) - if !isInvocation || len(invocationExpression.Arguments) != 1 { + if !isInvocation { continue } invokedIdentifier, isIdentifier := invocationExpression.InvokedExpression.(*ast.IdentifierExpression)