Fix Value.Type() returning Type referencing nil ptr for eight external values #2388
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
- changed some external value's Type() to check and return nil Type, instead of Type referencing nil pointer, such as nil *ResourceType. - changed type of Dictionary.DictionaryType from Type to *DictionaryType.
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 2e80137 Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## master #2388 +/- ##
==========================================
+ Coverage 78.52% 78.55% +0.02%
==========================================
Files 316 316
Lines 68561 68596 +35
==========================================
+ Hits 53837 53885 +48
+ Misses 12925 12911 -14
- Partials 1799 1800 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
11 tasks
SupunS
approved these changes
Mar 16, 2023
turbolent
reviewed
Mar 20, 2023
There was a problem hiding this comment.
Thanks for fixing this!
Unfortunately, introducing typed nils is too easy.
We should maybe add tests that avoid regressions for these functions, and in addition, prevent incorrect implementations in the future (e.g. by creating an empty instance of each Type implementation, and then, as each nested type is automatically nil, the a call to Type() should be equal to the untyped nil (== nil).
Let me try adding that.
turbolent
reviewed
Mar 21, 2023
turbolent
reviewed
Mar 22, 2023
|
Refactored the existing tests and unified them with similar tests for @fxamacker @SupunS Could you please have a look? |
SupunS
approved these changes
Mar 22, 2023
fxamacker
commented
Mar 22, 2023
turbolent
approved these changes
Mar 22, 2023
|
@dsainati1 @SupunS Could you please have a(nother) look? 🙏 This would unblock Faye on #2364 |
SupunS
approved these changes
Mar 22, 2023
Closed
24 tasks
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.
Changes include:
Dictionary.DictionaryTypefromTypeto*DictionaryType.Type()to check and return nilType, instead ofTypereferencing nil pointer for these external values:DictionaryStructResourceAttachmentEventContractEnumFunctionCloses #2387
masterbranchFiles changedin the Github PR explorer