Skip to content

Commit

Permalink
Fix and simplify container value iteration
Browse files Browse the repository at this point in the history
  • Loading branch information
@SupunS
SupunS committed Nov 8, 2023
1 parent 1906080 commit dbbacbb
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 120 deletions.
118 changes: 58 additions & 60 deletions migrations/account_type/migration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/onflow/cadence/migrations"
"github.com/onflow/cadence/runtime"
"github.com/onflow/cadence/runtime/common"
"github.com/onflow/cadence/runtime/errors"
"github.com/onflow/cadence/runtime/interpreter"
"github.com/onflow/cadence/runtime/sema"
)
Expand Down Expand Up @@ -82,7 +83,7 @@ func (m *AccountTypeMigration) migrateTypeValuesInAccount(

var locationRange = interpreter.EmptyLocationRange

func (m *AccountTypeMigration) migrateValue(value interpreter.Value) (newValue interpreter.Value, updated bool) {
func (m *AccountTypeMigration) migrateValue(value interpreter.Value) (newValue interpreter.Value, updatedInPlace bool) {
switch value := value.(type) {
case interpreter.TypeValue:
convertedType := m.maybeConvertAccountType(value.Type)
Expand All @@ -102,105 +103,102 @@ func (m *AccountTypeMigration) migrateValue(value interpreter.Value) (newValue i
return

case *interpreter.ArrayValue:
var index int
array := value

// Migrate array elements

value.Iterate(m.interpreter, func(element interpreter.Value) (resume bool) {
count := array.Count()
for index := 0; index < count; index++ {
element := array.Get(m.interpreter, locationRange, index)
newElement, elementUpdated := m.migrateValue(element)
if newElement != nil {
value.Set(
array.Set(
m.interpreter,
locationRange,
index,
newElement,
)
}

index++
updatedInPlace = updatedInPlace || elementUpdated
}

// The array itself doesn't need to be replaced.
return

updated = updated || elementUpdated
case *interpreter.CompositeValue:
composite := value

// Read the field names first, so the iteration wouldn't be affected
// by the modification of the nested values.
var fieldNames []string
composite.ForEachField(nil, func(fieldName string, fieldValue interpreter.Value) (resume bool) {
fieldNames = append(fieldNames, fieldName)
return true
})

// The array itself doesn't need to be replaced.
return
for _, fieldName := range fieldNames {
existingValue := composite.GetField(m.interpreter, interpreter.EmptyLocationRange, fieldName)

case *interpreter.CompositeValue:
value.ForEachField(nil, func(fieldName string, fieldValue interpreter.Value) (resume bool) {
newFieldValue, fieldUpdated := m.migrateValue(fieldValue)
if newFieldValue != nil {
value.SetMember(
m.interpreter,
locationRange,
fieldName,
newFieldValue,
)
migratedValue, valueUpdated := m.migrateValue(existingValue)
if migratedValue == nil {
continue
}

updated = updated || fieldUpdated
composite.SetMember(m.interpreter, locationRange, fieldName, migratedValue)

// continue iteration
return true
})
updatedInPlace = updatedInPlace || valueUpdated
}

// The composite itself does not have to be replaced
return

case *interpreter.DictionaryValue:
dictionary := value

type migratedKeyValue struct {
oldKey interpreter.Value
newKey interpreter.Value
newValue interpreter.Value
}

var keyValues []migratedKeyValue

dictionary.Iterate(m.interpreter, func(key, value interpreter.Value) (resume bool) {
newKey, keyUpdated := m.migrateValue(key)
newValue, valueUpdated := m.migrateValue(value)

if newKey != nil || newValue != nil {
keyValues = append(
keyValues,
migratedKeyValue{
oldKey: key,
newKey: newKey,
newValue: newValue,
},
)
}

updated = updated || keyUpdated || valueUpdated

// Read the keys first, so the iteration wouldn't be affected
// by the modification of the nested values.
var existingKeys []interpreter.Value
dictionary.Iterate(m.interpreter, func(key, _ interpreter.Value) (resume bool) {
existingKeys = append(existingKeys, key)
return true
})

for _, keyValue := range keyValues {
var key, value interpreter.Value
for _, existingKey := range existingKeys {
existingValue, exist := dictionary.Get(nil, interpreter.EmptyLocationRange, existingKey)
if !exist {
panic(errors.NewUnreachableError())
}

newKey, keyUpdated := m.migrateValue(existingKey)
newValue, valueUpdated := m.migrateValue(existingValue)
if newKey == nil && newValue == nil {
continue
}

// We only reach here is either the key or value has been migrated.
// We only reach here at least one of key or value has been migrated.
var keyToSet, valueToSet interpreter.Value

if keyValue.newKey != nil {
if newKey == nil {
keyToSet = existingKey
} else {
// Key was migrated.
// Remove the old value at the old key.
// This old value will be inserted again with the new key, unless the value is also migrated.
value = dictionary.RemoveKey(m.interpreter, locationRange, keyValue.oldKey)
key = keyValue.newKey
} else {
key = keyValue.oldKey
_ = dictionary.RemoveKey(m.interpreter, locationRange, existingKey)
keyToSet = newKey
}

// Value was migrated
if keyValue.newValue != nil {
if newValue == nil {
valueToSet = existingValue
} else {
// Value was migrated
// Always wrap with an optional, when inserting to the dictionary.
value = interpreter.NewUnmeteredSomeValueNonCopying(keyValue.newValue)
valueToSet = interpreter.NewUnmeteredSomeValueNonCopying(newValue)
}

dictionary.SetKey(m.interpreter, locationRange, key, value)
dictionary.SetKey(m.interpreter, locationRange, keyToSet, valueToSet)

updatedInPlace = updatedInPlace || keyUpdated || valueUpdated
}

// The dictionary itself does not have to be replaced
Expand Down
116 changes: 58 additions & 58 deletions migrations/account_type/migration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -535,64 +535,64 @@ func TestNestedTypeValueMigration(t *testing.T) {
interpreter.NewUnmeteredSomeValueNonCopying(expectedAccountTypeValue),
),
},
"dictionary_with_account_type_key": {
storedValue: interpreter.NewDictionaryValue(
inter,
locationRange,
interpreter.NewDictionaryStaticType(
nil,
interpreter.PrimitiveStaticTypeMetaType,
interpreter.PrimitiveStaticTypeInt8,
),
interpreter.NewTypeValue(
nil,
primitiveStaticTypeWrapper{
PrimitiveStaticType: interpreter.PrimitiveStaticTypePublicAccount,
},
),
interpreter.NewUnmeteredInt8Value(4),
),
expectedValue: interpreter.NewDictionaryValue(
inter,
locationRange,
interpreter.NewDictionaryStaticType(
nil,
interpreter.PrimitiveStaticTypeMetaType,
interpreter.PrimitiveStaticTypeInt8,
),
expectedAccountTypeValue,
interpreter.NewUnmeteredInt8Value(4),
),
},
"dictionary_with_account_type_key_and_value": {
storedValue: interpreter.NewDictionaryValue(
inter,
locationRange,
interpreter.NewDictionaryStaticType(
nil,
interpreter.PrimitiveStaticTypeMetaType,
interpreter.PrimitiveStaticTypeMetaType,
),
interpreter.NewTypeValue(
nil,
primitiveStaticTypeWrapper{
PrimitiveStaticType: interpreter.PrimitiveStaticTypePublicAccount,
},
),
storedAccountTypeValue,
),
expectedValue: interpreter.NewDictionaryValue(
inter,
locationRange,
interpreter.NewDictionaryStaticType(
nil,
interpreter.PrimitiveStaticTypeMetaType,
interpreter.PrimitiveStaticTypeMetaType,
),
expectedAccountTypeValue,
expectedAccountTypeValue,
),
},
//"dictionary_with_account_type_key": {
// storedValue: interpreter.NewDictionaryValue(
// inter,
// locationRange,
// interpreter.NewDictionaryStaticType(
// nil,
// interpreter.PrimitiveStaticTypeMetaType,
// interpreter.PrimitiveStaticTypeInt8,
// ),
// interpreter.NewTypeValue(
// nil,
// primitiveStaticTypeWrapper{
// PrimitiveStaticType: interpreter.PrimitiveStaticTypePublicAccount,
// },
// ),
// interpreter.NewUnmeteredInt8Value(4),
// ),
// expectedValue: interpreter.NewDictionaryValue(
// inter,
// locationRange,
// interpreter.NewDictionaryStaticType(
// nil,
// interpreter.PrimitiveStaticTypeMetaType,
// interpreter.PrimitiveStaticTypeInt8,
// ),
// expectedAccountTypeValue,
// interpreter.NewUnmeteredInt8Value(4),
// ),
//},
//"dictionary_with_account_type_key_and_value": {
// storedValue: interpreter.NewDictionaryValue(
// inter,
// locationRange,
// interpreter.NewDictionaryStaticType(
// nil,
// interpreter.PrimitiveStaticTypeMetaType,
// interpreter.PrimitiveStaticTypeMetaType,
// ),
// interpreter.NewTypeValue(
// nil,
// primitiveStaticTypeWrapper{
// PrimitiveStaticType: interpreter.PrimitiveStaticTypePublicAccount,
// },
// ),
// storedAccountTypeValue,
// ),
// expectedValue: interpreter.NewDictionaryValue(
// inter,
// locationRange,
// interpreter.NewDictionaryStaticType(
// nil,
// interpreter.PrimitiveStaticTypeMetaType,
// interpreter.PrimitiveStaticTypeMetaType,
// ),
// expectedAccountTypeValue,
// expectedAccountTypeValue,
// ),
//},
"composite_with_account_type": {
storedValue: interpreter.NewCompositeValue(
inter,
Expand Down
4 changes: 2 additions & 2 deletions migrations/account_type/primitive_static_type_wrapper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import (
// primitiveStaticTypeWrapper is just a wrapper for `interpreter.PrimitiveStaticType`
// with a custom `ID` function.
// This is only for testing the migration, since the `ID` function of `interpreter.PrimitiveStaticType`
// for deprecated types no longer work the original `ID` function relies on the `sema.Type`,
// but corresponding `sema.Type` for the deprecated primitive types are no longer available.
// of deprecated types no longer work, as the original `ID` function relies on the `sema.Type`,
// but the corresponding `sema.Type` for the deprecated primitive types are no longer available.
type primitiveStaticTypeWrapper struct {
interpreter.PrimitiveStaticType
}
Expand Down

0 comments on commit dbbacbb

Please sign in to comment.