-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[CSApply] Fix source breakage related to deep-equality types and SE-0110 #12934
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
@swift-ci please smoke test |
/cc @DougGregor |
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.
I think there's a minor cleanup to be had, but otherwise LGTM
lib/Sema/CSApply.cpp
Outdated
llvm_unreachable("Should be handled above"); | ||
|
||
auto genericArgs1 = bound1->getGenericArgs(); | ||
auto genericArgs2 = bound2->getGenericArgs(); |
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.
How about using getAnyOptionalObjectType()
here, because this is regarding optionals? It'll make for a much smaller change.
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.
I don't think it's only optionals, it's any generic type, but I can make it only about optionals...
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.
Huh. Okay!
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.
If it's more general that optionals, then leave it as is. Thank you for clarifying!
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.
E.g.
struct S<T> {}
let x: S<(()) -> Void> = S()
func foo(_: S<() -> Void>) {}
foo(x) // should work in 3 mode
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.
I'll add one more test like this just for clarity and merge, thank you!
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.
@slavapestov @DougGregor Looks like it doesn't work if I simply break in case of S<() -> Void>
vs. S<(()) -> Void>
it goes through whole coerceToType
method and fails on unreachable as unhandled coercion (if I return expr
it fails later on saying that types didn't match up in ShuffleExpr). Do you know if there is any specific coercion that could be applied here or should make it so the hack works only for optionals as @DougGregor suggested and let everything else fall-through into unreachable?
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.
I guess if there is no coercion like that we shouldn't even be forming solutions in Swift 3 mode...
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.
@DougGregor it looks like we can't simply fix Verifier to ignore this because it requires function-to-function conversion but I can see that we only support conversion + injection for optionals and not other bound generic types, is that correct? I think the best would be to make it so only optional produces solution in swift 3 and other bound generic types do not, WDYT?
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.
LGTM except for the nitpick
test/Constraints/diagnostics.swift
Outdated
@@ -1117,3 +1117,10 @@ func badTypes() { | |||
// expected-error@-1 {{type of expression is ambiguous without more context}} | |||
// FIXME: terrible diagnostic | |||
} | |||
|
|||
// rdar://problem/35198459 - source-compat-suite failure: Moya (toType->hasUnresolvedType() && "Should have handled this above") |
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.
This should go into test/Compatibility/tuple_arguments and test/Constraints/tuple_arguments
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.
Thanks, will move! I can't ever seems to figure out where to put new tests :(
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.
That's OK, they're not well organized to begin with IMO and everyone has different opinions, but I've been trying to keep all SE-0110-related test cases in one place.
Fix problem related to Swift 3 mode (with assertions), since Swift 3 mode allows passing arguments with extra parens to parameters which don't expect them, it should be supported by "deep equality" types e.g. Optional<T>: ```swift func foo(_: (() -> Void)?) {} func bar() -> ((()) -> Void)? { return nil } foo(bar) // This expression should compile in Swift 3 mode ``` Resolves: rdar://problem/35198459
@swift-ci please test and merge |
I'm going to fix the problem with Optional type in this PR and look into what to do about |
@swift-ci please test |
@swift-ci please test source compatibility |
Fix problem related to Swift 3 mode (with assertions),
since Swift 3 mode allows passing arguments with extra parens
to parameters which don't expect them, it should be supported
by "deep equality" types like Optional e.g.
Resolves: rdar://problem/35198459