Fix JSON encoding of type values with recursive type #1705
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit ec9fb29 Results
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dsainati1
approved these changes
Jun 7, 2022
Codecov Report
@@ Coverage Diff @@
## master #1705 +/- ##
=======================================
Coverage 76.63% 76.64%
=======================================
Files 292 292
Lines 61195 61207 +12
=======================================
+ Hits 46898 46910 +12
+ Misses 12684 12683 -1
- Partials 1613 1614 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
SupunS
reviewed
Jun 7, 2022
encoding/json/encode.go
Outdated
Comment on lines
664
to
670
| switch typ := typ.(type) { | ||
| case cadence.CompositeType, cadence.InterfaceType: | ||
| if _, ok := results[typ]; ok { | ||
| return typ.ID() | ||
| } | ||
| results[typ] = struct{}{} | ||
| } |
There was a problem hiding this comment.
What if these two checks are swapped, just to make sure any unhandled/ newly-added types are gracefully handled?
Suggested change
| switch typ := typ.(type) { | |
| case cadence.CompositeType, cadence.InterfaceType: | |
| if _, ok := results[typ]; ok { | |
| return typ.ID() | |
| } | |
| results[typ] = struct{}{} | |
| } | |
| if _, ok := results[typ]; ok { | |
| switch typ := typ.(type) { | |
| case cadence.CompositeType, cadence.InterfaceType: | |
| return typ.ID() | |
| default: | |
| panic(fmt.Errorf("unsupported recursive type: %s", typ)) | |
| } | |
| } | |
| results[typ] = struct{}{} |
There was a problem hiding this comment.
Yeah, good idea, it would be great to ensure all recursive types are supported!
This suggestions does not quite work as-is: It records all types as "seen", so e.g. even a primitive type like cadence.IntType is erroneously considered an unsupported recursive type.
Tried to fix this by only recording the recursive types:
var supportedRecursiveType bool
switch typ.(type) {
case cadence.CompositeType, cadence.InterfaceType:
supportedRecursiveType = true
}
if _, ok := results[typ]; ok {
if !supportedRecursiveType {
panic(fmt.Errorf("failed to prepare type: unsupported recursive type: %T", typ))
}
return typ.ID()
}
if supportedRecursiveType {
results[typ] = struct{}{}
}That then because we always check any type (results[typ]), this panics for some types, e.g. functions, which are not pointers at the moment:
panic: runtime error: hash of unhashable type cadence.FunctionType
I guess we should keep the logic above and make all types hashable?
There was a problem hiding this comment.
Ah yes, I wasn't thinking of the other types.
This looks good to me. Thanks for adding that!
SupunS
approved these changes
Jun 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1704
Description
Preparation of static types may run into a recursion. Track results and return the type ID in the recursive case.
masterbranchFiles changedin the Github PR explorer