Properly handle Never type argument to Capabilities.get
#3203
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 468b11b Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
turbolent
approved these changes
Mar 28, 2024
There was a problem hiding this comment.
Thank you for looking into this!
We can definitely merge this fix, but maybe also want to improve this further: Currently Never is only rejected at run-time, we maybe want to already reject this statically.
We currently do not have the means in the language to express the constraint that the type parameter should have a lower bound. However, we have a mechanism for it in the checker: FunctionType.TypeArgumentsCheck. Maybe extend this solution by adding such a static check (potentially in a separate PR).
We have a similar case in / report for revertibleRandom, see https://github.com/dapperlabs/cadence-internal/issues/188. In a comment of the issue I noted how it would be nice to extend the type bounds to express this in a way where it is also communicated to users: https://github.com/dapperlabs/cadence-internal/issues/188#issuecomment-1906752366. I had opened #3059 a while back with a PoC of this idea, maybe have a look, I'd love to hear your thoughts!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3203 +/- ##
==========================================
+ Coverage 80.68% 80.70% +0.01%
==========================================
Files 380 380
Lines 93183 93209 +26
==========================================
+ Hits 75188 75226 +38
+ Misses 15290 15278 -12
Partials 2705 2705
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Yeah this makes sense. What is left to do on #3059? I can pick this up |
|
@dsainati1 sounds good! I tried to leave TODOs in both the PR description (see tasklist) as well as in the code. We can maybe discuss it in the next Impl. Sync |
SupunS
approved these changes
Apr 1, 2024
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 https://github.com/dapperlabs/cadence-internal/issues/176
Properly return a
nilvalue in this case rather than panicking.masterbranchFiles changedin the Github PR explorer