Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add validation for enum cases during contract updates #762

Merged
merged 4 commits into from
Apr 6, 2021
Merged

Add validation for enum cases during contract updates #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc47fc3
Add validation for enum cases during contract updates
SupunS Apr 5, 2021
ffa4860
Swap the restriction on adding vs removing enum cases
SupunS Apr 5, 2021
8721f72
Improve missing enum case error message
SupunS Apr 5, 2021
51bb585
Export the fields of errors related to contract update validation
SupunS Apr 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 64 additions & 17 deletions runtime/contract_update_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ func (validator *ContractUpdateValidator) checkDeclarationUpdatability(
// - 'structs' and 'enums'
if oldDeclaration.DeclarationKind() != newDeclaration.DeclarationKind() {
validator.report(&InvalidDeclarationKindChangeError{
name: oldDeclaration.DeclarationIdentifier().Identifier,
oldKind: oldDeclaration.DeclarationKind(),
newKind: newDeclaration.DeclarationKind(),
Name: oldDeclaration.DeclarationIdentifier().Identifier,
OldKind: oldDeclaration.DeclarationKind(),
NewKind: newDeclaration.DeclarationKind(),
Range: ast.NewRangeFromPositioned(newDeclaration.DeclarationIdentifier()),
})

Expand Down Expand Up @@ -147,8 +147,8 @@ func (validator *ContractUpdateValidator) checkFields(oldDeclaration ast.Declara
oldField := oldFields[newField.Identifier.Identifier]
if oldField == nil {
validator.report(&ExtraneousFieldError{
declName: newDeclaration.DeclarationIdentifier().Identifier,
fieldName: newField.Identifier.Identifier,
DeclName: newDeclaration.DeclarationIdentifier().Identifier,
FieldName: newField.Identifier.Identifier,
Range: ast.NewRangeFromPositioned(newField.Identifier),
})

Expand All @@ -163,9 +163,9 @@ func (validator *ContractUpdateValidator) checkField(oldField *ast.FieldDeclarat
err := oldField.TypeAnnotation.Type.CheckEqual(newField.TypeAnnotation.Type, validator)
if err != nil {
validator.report(&FieldMismatchError{
declName: validator.currentDecl.DeclarationIdentifier().Identifier,
fieldName: newField.Identifier.Identifier,
err: err,
DeclName: validator.currentDecl.DeclarationIdentifier().Identifier,
FieldName: newField.Identifier.Identifier,
Err: err,
Range: ast.NewRangeFromPositioned(newField.TypeAnnotation),
})
}
Expand Down Expand Up @@ -193,6 +193,7 @@ func (validator *ContractUpdateValidator) checkNestedDeclarations(
return nil, false
}

// Check nested structs, enums, etc.
newNestedCompositeDecls := newDeclaration.DeclarationMembers().Composites()
for _, newNestedDecl := range newNestedCompositeDecls {
oldNestedDecl, found := getOldCompositeOrInterfaceDecl(newNestedDecl.Identifier.Identifier)
Expand All @@ -204,6 +205,7 @@ func (validator *ContractUpdateValidator) checkNestedDeclarations(
validator.checkDeclarationUpdatability(oldNestedDecl, newNestedDecl)
}

// Check nested interfaces.
newNestedInterfaces := newDeclaration.DeclarationMembers().Interfaces()
for _, newNestedDecl := range newNestedInterfaces {
oldNestedDecl, found := getOldCompositeOrInterfaceDecl(newNestedDecl.Identifier.Identifier)
Expand All @@ -214,6 +216,51 @@ func (validator *ContractUpdateValidator) checkNestedDeclarations(

validator.checkDeclarationUpdatability(oldNestedDecl, newNestedDecl)
}

// Check enum-cases, if theres any.
validator.checkEnumCases(oldDeclaration, newDeclaration)
}

// checkEnumCases validates updating enum cases. Updated enum must:
// - Have at-least the same number of enum-cases as the old enum (Adding is allowed, but no removals).
// - Preserve the order of the old enum-cases (Adding to top/middle is not allowed, swapping is not allowed).
func (validator *ContractUpdateValidator) checkEnumCases(oldDeclaration ast.Declaration, newDeclaration ast.Declaration) {
newEnumCases := newDeclaration.DeclarationMembers().EnumCases()
oldEnumCases := oldDeclaration.DeclarationMembers().EnumCases()

newEnumCaseCount := len(newEnumCases)
oldEnumCaseCount := len(oldEnumCases)

if newEnumCaseCount < oldEnumCaseCount {
validator.report(&MissingEnumCasesError{
DeclName: newDeclaration.DeclarationIdentifier().Identifier,
Expected: oldEnumCaseCount,
Found: newEnumCaseCount,
Range: ast.NewRangeFromPositioned(newDeclaration.DeclarationIdentifier()),
})

// If some enum cases are removed, trying to match each enum case
// may result in too many regression errors. hence return.
return
}

// Check whether the enum cases matches the old enum cases.
for index, newEnumCase := range newEnumCases {
// If there are no more old enum-cases, then these are newly added enum-cases,
// which should be fine.
if index >= oldEnumCaseCount {
continue
}

oldEnumCase := oldEnumCases[index]
if oldEnumCase.Identifier.Identifier != newEnumCase.Identifier.Identifier {
validator.report(&EnumCaseMismatchError{
ExpectedName: oldEnumCase.Identifier.Identifier,
FoundName: newEnumCase.Identifier.Identifier,
Range: ast.NewRangeFromPositioned(newEnumCase),
})
}
}
}

func (validator *ContractUpdateValidator) CheckNominalTypeEquality(expected *ast.NominalType, found ast.Type) error {
Expand Down Expand Up @@ -423,8 +470,8 @@ func (validator *ContractUpdateValidator) checkConformances(

if len(oldConformances) != len(newConformances) {
validator.report(&ConformanceCountMismatchError{
expected: len(oldConformances),
found: len(newConformances),
Expected: len(oldConformances),
Found: len(newConformances),
Range: ast.NewRangeFromPositioned(newDecl.Identifier),
})

Expand All @@ -438,8 +485,8 @@ func (validator *ContractUpdateValidator) checkConformances(
err := oldConformance.CheckEqual(newConformance, validator)
if err != nil {
validator.report(&ConformanceMismatchError{
declName: newDecl.Identifier.Identifier,
err: err,
DeclName: newDecl.Identifier.Identifier,
Err: err,
Range: ast.NewRangeFromPositioned(newConformance),
})
}
Expand All @@ -455,16 +502,16 @@ func (validator *ContractUpdateValidator) report(err error) {

func (validator *ContractUpdateValidator) getContractUpdateError() error {
return &ContractUpdateError{
contractName: validator.contractName,
errors: validator.errors,
location: validator.location,
ContractName: validator.contractName,
Errors: validator.errors,
Location: validator.location,
}
}

func getTypeMismatchError(expectedType ast.Type, foundType ast.Type) *TypeMismatchError {
return &TypeMismatchError{
expectedType: expectedType,
foundType: foundType,
ExpectedType: expectedType,
FoundType: foundType,
Range: ast.NewRangeFromPositioned(foundType),
}
}
Expand Down
140 changes: 136 additions & 4 deletions runtime/contract_update_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1326,6 +1326,106 @@ func TestContractUpdateValidation(t *testing.T) {
"\n | ^^^^^^^^^^^^^^^^^^^^^^^^^ "+
"incompatible type annotations. expected `{TestInterface}`, found `TestStruct{TestInterface}`")
})

t.Run("enum valid", func(t *testing.T) {
const oldCode = `
pub contract Test28 {
pub enum Foo: UInt8 {
pub case up
pub case down
}
}`

const newCode = `
pub contract Test28 {
pub enum Foo: UInt8 {
pub case up
pub case down
}
}`

err := deployAndUpdate("Test28", oldCode, newCode)
require.NoError(t, err)
})

t.Run("enum remove case", func(t *testing.T) {
const oldCode = `
pub contract Test29 {
pub enum Foo: UInt8 {
pub case up
pub case down
}
}`

const newCode = `
pub contract Test29 {
pub enum Foo: UInt8 {
pub case up
}
}`

err := deployAndUpdate("Test29", oldCode, newCode)
require.Error(t, err)

cause := getErrorCause(t, err, "Test29")
assertMissingEnumCasesError(t, cause, "Foo", 2, 1)
})

t.Run("enum add case", func(t *testing.T) {
const oldCode = `
pub contract Test30 {
pub enum Foo: UInt8 {
pub case up
pub case down
}
}`

const newCode = `
pub contract Test30 {
pub enum Foo: UInt8 {
pub case up
pub case down
pub case left
}
}`

err := deployAndUpdate("Test30", oldCode, newCode)
require.NoError(t, err)
})

t.Run("enum swap cases", func(t *testing.T) {
const oldCode = `
pub contract Test31 {
pub enum Foo: UInt8 {
pub case up
pub case down
pub case left
}
}`

const newCode = `
pub contract Test31 {
pub enum Foo: UInt8 {
pub case down
pub case left
pub case up
}
}`

err := deployAndUpdate("Test31", oldCode, newCode)
require.Error(t, err)

updateErr := getContractUpdateError(t, err)
require.NotNil(t, updateErr)
assert.Equal(t, fmt.Sprintf("cannot update contract `%s`", "Test31"), updateErr.Error())

childErrors := updateErr.ChildErrors()
require.Equal(t, 3, len(childErrors))

assertEnumCaseMismatchError(t, childErrors[0], "up", "down")
assertEnumCaseMismatchError(t, childErrors[1], "down", "left")
assertEnumCaseMismatchError(t, childErrors[2], "left", "up")
})
}

func assertDeclTypeChangeError(
Expand Down Expand Up @@ -1371,11 +1471,11 @@ func assertFieldTypeMismatchError(
fieldMismatchError.Error(),
)

assert.IsType(t, &TypeMismatchError{}, fieldMismatchError.err)
assert.IsType(t, &TypeMismatchError{}, fieldMismatchError.Err)
assert.Equal(
t,
fmt.Sprintf("incompatible type annotations. expected `%s`, found `%s`", expectedType, foundType),
fieldMismatchError.err.Error(),
fieldMismatchError.Err.Error(),
)
}

Expand All @@ -1396,11 +1496,43 @@ func assertConformanceMismatchError(
conformanceMismatchError.Error(),
)

assert.IsType(t, &TypeMismatchError{}, conformanceMismatchError.err)
assert.IsType(t, &TypeMismatchError{}, conformanceMismatchError.Err)
assert.Equal(
t,
fmt.Sprintf("incompatible type annotations. expected `%s`, found `%s`", expectedType, foundType),
conformanceMismatchError.err.Error(),
conformanceMismatchError.Err.Error(),
)
}

func assertEnumCaseMismatchError(t *testing.T, err error, expectedEnumCase string, foundEnumCase string) {
require.Error(t, err)
require.IsType(t, &EnumCaseMismatchError{}, err)
enumMismatchError := err.(*EnumCaseMismatchError)

assert.Equal(
t,
fmt.Sprintf(
"mismatching enum case: expected `%s`, found `%s`",
expectedEnumCase,
foundEnumCase,
),
enumMismatchError.Error(),
Comment on lines +1514 to +1519
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of comparing error messages literally, just check declName, expectedEnumCase, and foundEnumCase of the error directly, so changing the error message doesn't break the test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wanted to assert the error message, to make sure they are consistent, and the to-string works with nested errors, etc.

)
}

func assertMissingEnumCasesError(t *testing.T, err error, declName string, expectedCases int, foundCases int) {
require.Error(t, err)
require.IsType(t, &MissingEnumCasesError{}, err)
extraFieldError := err.(*MissingEnumCasesError)
assert.Equal(
t,
fmt.Sprintf(
"missing cases in enum `%s`: expected %d or more, found %d",
declName,
expectedCases,
foundCases,
),
extraFieldError.Error(),
)
}

Expand Down
Loading