Type Checking for attachment uses #2099
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
…nce into sainati/attachments-check-add-remove
…nce into sainati/attachments-check-add-remove
dsainati1
commented
Oct 26, 2022
6 tasks
…hments-check-add-remove
…nce into sainati/attachments-check-add-remove
robert-e-davidson3
approved these changes
Nov 1, 2022
dreamsmasher
reviewed
Nov 1, 2022
dreamsmasher
reviewed
Nov 1, 2022
dreamsmasher
reviewed
Nov 1, 2022
dreamsmasher
reviewed
Nov 1, 2022
dsainati1
commented
Nov 22, 2022
…nce into sainati/attachments-check-add-remove
turbolent
reviewed
Nov 30, 2022
…nce into sainati/attachments-check-add-remove
Base automatically changed from
sainati/attachments-checking
to
feature/attachments
December 1, 2022 19:31
…sainati/attachments-check-add-remove
Codecov Report
@@ Coverage Diff @@
## feature/attachments #2099 +/- ##
=======================================================
+ Coverage 77.65% 77.75% +0.09%
=======================================================
Files 310 312 +2
Lines 65899 66251 +352
=======================================================
+ Hits 51176 51513 +337
- Misses 12936 12948 +12
- Partials 1787 1790 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/attachments commit 8747c50 Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@dsainati1 Points 5 and 6 in the description are not part of the PR anymore, right? |
|
Ah yea good point |
turbolent
reviewed
Dec 6, 2022
runtime/sema/type.go
Outdated
Comment on lines
5511
to
5520
| case *InterfaceType: | ||
| switch typedInnerSubType := typedSubType.Type.(type) { | ||
| case *CompositeType: | ||
| return typedInnerSubType.ExplicitInterfaceConformanceSet().Contains(typedInnerSuperType) | ||
| // An interface type is a supertype of a restricted type if the restricted set contains | ||
| // that explicit interface type. Once interfaces can conform to interfaces, this should instead | ||
| // check that at least one value in the restriction set is a subtype of the interface supertype | ||
| case *RestrictedType: | ||
| return typedInnerSubType.RestrictionSet().Contains(typedInnerSuperType) | ||
| } |
There was a problem hiding this comment.
The implementation ignores the restricted type (T in &T{Us}), does it need to be checked too?
runtime/sema/type.go
Outdated
Comment on lines
5511
to
5520
| case *InterfaceType: | ||
| switch typedInnerSubType := typedSubType.Type.(type) { | ||
| case *CompositeType: | ||
| return typedInnerSubType.ExplicitInterfaceConformanceSet().Contains(typedInnerSuperType) | ||
| // An interface type is a supertype of a restricted type if the restricted set contains | ||
| // that explicit interface type. Once interfaces can conform to interfaces, this should instead | ||
| // check that at least one value in the restriction set is a subtype of the interface supertype | ||
| case *RestrictedType: | ||
| return typedInnerSubType.RestrictionSet().Contains(typedInnerSuperType) | ||
| } |
There was a problem hiding this comment.
Instead of adding this special rule to the subtype checking, maybe this can be a separate function that can be used where needed? (Where is it needed?)
Comment on lines
+5633
to
+5634
| case *RestrictedType: | ||
| return typedSubType.RestrictionSet().Contains(typedSuperType) |
There was a problem hiding this comment.
Same as above: Maybe this can be a separate function that is only used for this purpose.
The IsSubType function is already very complex, and so far assumes / could rely on the fact that interfaces only appear in restricted types. I'm worried the rules / this function becomes incomplete.
Co-authored-by: Bastian Müller <bastian@axiomzen.co>
…low/cadence into sainati/attachments-check-add-remove
…into sainati/attachments-check-add-remove
dsainati1
changed the base branch from
feature/attachments
to
attachments-master-merge
Base automatically changed from
attachments-master-merge
to
feature/attachments
December 9, 2022 18:17
turbolent
approved these changes
Dec 10, 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.
Part of #357 and #2061
This adds type checking for the uses of attachment types. In particular:
attach A() to v, which requires its first expression to be an attachment constructor for whichvis a valid composite base typeremove A from v, which requires its first expression to be an attachment name for whichvis a valid composite base typev[A], which requires its indexing expression to be an attachment name for whichvis a valid composite base type, and returns a value of type&A?AnyStructAttachmentandAnyResourceAttachment, which are the supertypes of all struct and resource attachments respectively. These are subtypes ofAnyStructandAnyResourceas well.masterbranchFiles changedin the Github PR explorer