-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[ConstraintSystem] Strip labels from enum elements when passed as values #9938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/cc @slavapestov please take a look and let me know what you think |
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
rudkx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
lib/Sema/ConstraintSystem.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safer to have this under the version check, but is it actually necessary?
What I'm wondering is if there might be cases where the Swift 3 behavior is also broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Swift 3 behavior is broken if your enum argument type has multiple elements.
lib/Sema/ConstraintSystem.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please either remove this comment, or rewrite it to talk about the common behavior of both functions and enum cases.
test/Constraints/diagnostics.swift
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this in a new file, test/Compatibility/enum_cases.swift.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this in a new file, test/Constraints/enum_cases.swift.
Both files need a lot more tests, for example it would be nice to test:
- case has no label
- case has label
- case is a tuple with multiple elements, some of which have labels
- assign case to a 'let' binding with an explicit type
|
I wonder if we can get rid of scalar-to-tuple conversions entirely in Swift 4 mode once this goes in. |
Enum elements have to be treated the same way as regular functions when passed as values, which means labels have to be stripped from their argument types. Resolves: rdar://problem/32300339.
|
@slavapestov @rudkx I've addressed all of the notes - reformatted the comment and added a bunch more tests to the {Compatibility, Constraints}/enum_cases.swift . Also added a test which show the different in behavior between swift 3 and 4. |
|
LGTM! |
|
Looks great! |
|
@swift-ci please test and merge |
|
@xedin We should get this into swift-4.0-branch. |
|
@slavapestov Will do! |
Enum elements have to be treated the same way as regular functions
when passed as values, which means labels have to be stripped from
their argument types.
Resolves: rdar://problem/32300339.