Fix migrating values with empty intersection type #3138
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 f605ce8 Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
turbolent
changed the title
turbolent
commented
Feb 27, 2024
| intersectionSuperType := typedSuperType.LegacyType //nolint:staticcheck | ||
|
|
||
| switch intersectionSuperType { | ||
| case nil, AnyResourceType, AnyStructType, AnyType: |
There was a problem hiding this comment.
Integrated handling for no legacy type, see diff in description
Comment on lines
+7721
to
+7726
| case nil: | ||
| // An intersection type `{Us}` is a subtype of an intersection type `{Vs}` / `{Vs}` / `{Vs}`: | ||
| // when `Vs` is a subset of `Us`. | ||
|
|
||
| return typedSuperType.EffectiveIntersectionSet(). | ||
| IsSubsetOf(typedSubType.EffectiveIntersectionSet()) |
There was a problem hiding this comment.
Integrated rule for no legacy type. Same as commented out code at top
Comment on lines
+7734
to
+7735
| if intersectionSuperType != nil && | ||
| !IsSubType(intersectionSubtype, intersectionSuperType) { |
There was a problem hiding this comment.
Added support for no legacy type
Comment on lines
+7751
to
+7752
| if intersectionSuperType != nil && | ||
| !IsSubType(intersectionSubtype, intersectionSuperType) { |
There was a problem hiding this comment.
Added support for no legacy type
Comment on lines
+7767
to
+7768
| if intersectionSuperType != nil && | ||
| !IsSubType(typedSubType, intersectionSuperType) { |
There was a problem hiding this comment.
Added support for no legacy type
|
|
||
| intersectionSubType := typedSubType.LegacyType //nolint:staticcheck | ||
| switch intersectionSubType { | ||
| case nil, AnyResourceType, AnyStructType, AnyType: |
There was a problem hiding this comment.
Integrated rule for no legacy type
| // `Us` and `Ws` do *not* have to be subsets: | ||
| // The owner may freely restrict and unrestrict. | ||
|
|
||
| return intersectionSubType == intersectionSuperType |
There was a problem hiding this comment.
Both intersectionSubType and intersectionSuperType are non-nil here
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3138 +/- ##
==========================================
- Coverage 80.60% 80.53% -0.07%
==========================================
Files 381 381
Lines 92456 92616 +160
==========================================
+ Hits 74520 74585 +65
- Misses 15235 15327 +92
- Partials 2701 2704 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dsainati1
approved these changes
Feb 28, 2024
dsainati1
reviewed
Feb 28, 2024
SupunS
approved these changes
Mar 4, 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.
Work towards #3096
Description
Migrated values may have empty intersection types, as they originally were restricted types, i.e. they have a legacy type.
Creating empty intersection types without (correctly) defensively panics, but prevents migrating values.
Removing the panic and only accepting empty intersection types is not enough: The resulting sema intersection type is then used for various purposes during the migration, e.g. subtype checking when inserting into a dictionary, performing the appropriate transfer (copy or move), etc.
Bring back the legacy type in
sema.IntersectionType, just like it already exists ininterpreter.StaticIntersectionType, and restore the subtyping rules which were removed in #2596 (see https://github.com/onflow/cadence/pull/2596/files#diff-c01765841e64b85306258f5ee4daff2bdc5b9e9e107167f55a6dba4162e8bdb8).The subtyping rules for Cadence 1.0 intersection type got integrated into the restored rules.
Because this is hard to review, I prepared a diff of the original subtyping rules for restricted types (taken from #2596) and the new subtying rules for intersection types of this PR:
masterbranchFiles changedin the Github PR explorer