-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[ConstraintSystem] Allow function-function conversions for keypath literals #39612
Conversation
d6eb734
to
69ffb18
Compare
@swift-ci please smoke test |
69ffb18
to
5ac32fe
Compare
@swift-ci please smoke test |
1 similar comment
This comment has been minimized.
This comment has been minimized.
@Jumhyn Is this ready for review or just a prototype? |
@xedin It still has some kinks to be worked out so it’s not ready for a detailed review just yet, but if you have any thoughts about the high level approach here I’d love to hear them. |
Sounds good, just making sure that we don't drop a ball on this one. Please feel free to assign me and/or Holly as a reviewer once it's ready. |
5ac32fe
to
0e8cfe5
Compare
@swift-ci please smoke test |
// FIXME: This error text is bogus due to KeyPath base covariance. | ||
let _: (Base?) -> Base = \Base.base // expected-error {{value of optional type 'Optional<Base>' must be unwrapped to refer to member 'base' of wrapped base type 'Base'}} expected-note {{use unwrapped type 'Base' as key path root}} {{33-37=Base}} |
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.
Note: we get the same error without the function conversion, e.g.,
let _: KeyPath<Base?, Base> = \Base.base
Filed here.
@swift-ci please test |
@swift-ci please test source compatibility |
Build failed |
@swift-ci please test macOS |
Build failed |
@swift-ci please test macOS |
@swift-ci please test Linux |
@swift-ci please test linux |
@Jumhyn This is in my queue, will try to take a look before the end of this week. |
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! I have left a couple of comments inline but nothing major.
0e8cfe5
to
9a7d9f7
Compare
@swift-ci please test source compatibility |
1 similar comment
@swift-ci please test source compatibility |
970aed5
to
01cd88c
Compare
@swift-ci please test |
lib/Sema/CSSimplify.cpp
Outdated
} else if ((!flags.contains(TMF_GenerateConstraints) && | ||
!anyComponentsUnresolved) || |
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.
@xedin one other thing for you to look at when you have a chance to check on this PR—I originally thought your recent changes to keypath simplification would remove the need for this, but we were still failing to properly convert the paths in the testMinimalKeypaths()
test below.
Since those keypaths don't have anything to formally 'resolve' we were hitting this path during constraint generation before the proper disjunction/applicable function constraints had even been added to the system.
Also now probably makes sense to rename anyComponentsUnresolved
since it's become extremely narrow, basically only used for code completion... I'll update to rename that soon.
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 we need to figure out a better way here. Maybe, temporarily, it would be okay to just check
if (Phase == ConstraintSystemPhase::ConstraintGeneration)
return formUnsolved();
at the very start of this method.
But what @amritpan and I worked toward this summer (but couldn't quite land it yet) is moving key path "opening" into the resolveKeyPath
and allowing it to happen only when there is either a contextual type or it's known that there wouldn't be one, this way we don't actually have to worry about eagerly resolving key path literals.
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.
Hmm how could we conclusively determine whether we'll be able to infer a type before we actually attempt solving? Right now, this is the only place we will 'default' to an actual KeyPath
type if no other context is available, so if we move this to resolveKeyPath
it seems like we'd need 100% accuracy, otherwise we risk either 1) binding to KeyPath
too early when we should have inferred a function type somewhere along the way, or else 2) failing to bind to KeyPath
because we were unsure, but then down the line failing to solve the system because we needed the KeyPath
context to come from the literal...
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.
Also—what is the significance of using flags
vs. Phase
here? I guess flags
could be set for constraint generation even if we are not actually walking an expression to set the Phase
?
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.
Hmm how could we conclusively determine whether we'll be able to infer a type before we actually attempt solving?
Only inference can tell us conclusively in certain conditions that's why resolveKeyPath is going to be called on that path (as in in matchTypesBindTypeVar
just like we do for closures). We should only let simplifyKeyPathConstraint
set the type if there is definitely contextual type we could use, once we have that we can remove this change to iterate over constraints I added as well.
Also—what is the significance of using flags vs. Phase here?
Phase
is a safer bet because it identifies explicitly what phase constraint system is currently in, TMF_GenerateConstraints
could technically be set randomly and not as narrow.
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.
Ah I was confusing resolveKeyPath
and resolveKeyPathExpr
from PreCheckExpr
🤦 This makes more sense!
Is there a WIP branch anywhere? I'll make the stopgap change in this PR but would be interested in taking a look that the proper fix next unless @amritpan you still had plans to tackle that?
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 @amritpan opened a PR for the work she was doing because it wasn't complete but I guess it would be reasonable to open a draft PR?
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.
unless @amritpan you still had plans to tackle that
Here's a draft PR of some of the resolveKeyPath
expansion that I worked on with @xedin, in case it helps.
lib/Sema/CSSimplify.cpp
Outdated
if (isKnownKeyPathType(lhs) && isKnownKeyPathType(rhs)) { | ||
// If we have keypath capabilities for both sides and one of the bases | ||
// is unresolved, it is too early to record fix. | ||
if (hasConversionOrRestriction(ConversionRestrictionKind::DeepEquality)) |
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 possible failure modes are:
- key path types mismatch on capability - KeyPath vs. WritableKeyPath for example
- one of the generic parameters (either Root or Value) mismatch:
- If there is any conversion recorded against this match we should let that happen (not just deep equality)
- otherwise we could either just record whole types or try to deep deeper and recursively call repairFailures on Root and Value.
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.
@xedin This bit I had just pulled up from lines 5296-5305 below for reuse. Happy to update it but not sure I totally understand the restricted conversion machinery. From your bullets it sounds like you're saying we should attempt the fix unconditionally if we have a capability mismatch, but if they agree on capability then we should only attempt the fix if we have no restrictions, does that sound right? So that would amount to updating the check to something like the following:
if (hasConversionOrRestriction(ConversionRestrictionKind::DeepEquality)) | |
if (lhs->getAnyGeneric() == rhs->getAnyGeneric() && llvm::any_of(conversionsOrFixes, | |
[](const RestrictionOrFix &correction) { | |
return bool(correction.getRestriction()); | |
})) |
(seems like this may be the best way to check keypath capability from just a Type
instance, unless I'm missing something?)
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'd say:
if (lhs->getAnyGeneric() != rhs->getAnyGeneric() || llvm::any_of(conversionsOrFixes, ...))
Because if key paths have different capatibilities we don't want to diagnose it here, same goes for restrictions - each restriction would diagnose separately when applied.
Unfortunate consequence of the current state of things is that every key path conversion anchored on KeyPathExpr ends up forming a disjunction where second choice has a fix and we'd really match try to avoid that.
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.
Hm, it appears there are some circumstances where this is the only place we end up applying a fix for mismatched capabilities, e.g.:
func issue_65965() {
struct S {
var s: String
let v: String
}
let refKP: ReferenceWritableKeyPath<S, String>
refKP = \.s
// expected-error@-1 {{key path value type 'WritableKeyPath<S, String>' cannot be converted to contextual type 'ReferenceWritableKeyPath<S, String>'}}
}
If we don't apply the fix here then we end up with the general 'type of expression is ambiguous without type annotation' fallback error as opposed to the specific error. Where would you have expected this to get diagnosed if not here?
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.
Looks like what happens is that 1) we infer ReferenceWritableKeyPath
as the type for the literal via the assignment operator, and then in simplifyKeyPathConstraint
attempts to bind to that type rather than convert, so we never have a chance to apply conversion restriction, and 2) even if we change things to allow conversion here since the mismatch ends up happening directly at the keypath constraint we don't even qualify for applying the fix in simplifyRestrictedConstraint
:
if (loc->isForAssignment() || loc->isForCoercion() ||
loc->isForContextualType() ||
loc->isLastElement<LocatorPathElt::ApplyArgToParam>() ||
loc->isForOptionalTry()) {
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.
Hm, we can leave capability out of this for now, I think the only way to fix it would be to always let simplifyKeyPathConstraint
assign a contextual type instead of eagerly binding it, that way it'd always end up with a mismatch in the right place.
lib/Sema/CSSimplify.cpp
Outdated
} else if ((!flags.contains(TMF_GenerateConstraints) && | ||
!anyComponentsUnresolved) || |
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 we need to figure out a better way here. Maybe, temporarily, it would be okay to just check
if (Phase == ConstraintSystemPhase::ConstraintGeneration)
return formUnsolved();
at the very start of this method.
But what @amritpan and I worked toward this summer (but couldn't quite land it yet) is moving key path "opening" into the resolveKeyPath
and allowing it to happen only when there is either a contextual type or it's known that there wouldn't be one, this way we don't actually have to worry about eagerly resolving key path literals.
01cd88c
to
a44f501
Compare
@swift-ci please test |
@swift-ci please test source compatibility |
@swift-ci Please build toolchain |
@swift-ci please test compiler performance |
@swift-ci please test Linux platform |
Hey @xedin I rebased this against your latest work on keypaths and the proposal has been approved! And further work you'd want to see on this before merge? Source compatibility suite is passing but think we should do any broader analysis? |
@xedin just bumping now that we're a bit further into the new year! |
Sorry, I'll try to get some time this week to review this! |
I didn't forget, this is in my queue for tomorrow. @Jumhyn could you please rebase meanwhile? |
Remove keypath subtype asserts; always use cached root type Add tests for keypaths converted to funcs with inout param Add unit test for overload selection
a44f501
to
0d79f45
Compare
@xedin thanks, done! |
@swift-ci please test |
@swift-ci please test source compatibility |
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.
Looks good! I left a few comments inline but they can be addressed in a follow-up.
@@ -697,6 +697,12 @@ ERROR(expr_smart_keypath_application_type_mismatch,none, | |||
"key path of type %0 cannot be applied to a base of type %1", | |||
(Type, Type)) | |||
ERROR(expr_keypath_root_type_mismatch, none, | |||
"key path root type %0 cannot be converted to contextual type %1", |
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.
We should align the wording here with other "cannot convert" diagnostics, how about - cannot convert key path root type %0 to a contextual type %1
?
"key path root type %0 cannot be converted to contextual type %1", | ||
(Type, Type)) | ||
ERROR(expr_keypath_type_mismatch, none, | ||
"key path of type %0 cannot be converted to contextual type %1", |
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.
Same here, there is a "cannot convert value of type ... to a contextual type" diagnostic, would be good to align them.
fnTy->getExtInfo()); | ||
|
||
return matchTypes(kpFnTy, paramFnTy, ConstraintKind::Conversion, subflags, | ||
locator).isSuccess(); |
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 the condition here be !matchTypes(...).isFailure();
which is a bit more defensive even if we cannot get delayed in this case (yet).
In \swiftlang#39612 we added subtyping for keypath-function conversions. However the implementation naively coerced the keypath expression itself to the inferred supertype, resulting in erroneous results. This patch updates the solution application logic to build the keypath-function conversion expression based entirely on the 'natural' keypath type, only converting to the inferred supertype at the end via the usual coerceToType machinery for function conversions.
In swiftlang#39612 we added subtyping for keypaths-as-functions, but during application the implementation naively coerced the keypath expression itself to the inferred supertype, resulting in erroneous results. This patch updates the solution application logic to build the keypath-function conversion expression based entirely on the 'natural' keypath type, only converting to the inferred supertype at the end via the usual coerceToType machinery for function conversions.
Currently the keypath-to-function conversion is extremely narrow—root and value types of the keypath must exactly match the function argument and result types of the function, respectively.
This PR allows the keypath-to-function conversion to partake in the full generality of function-function type conversion. This means a keypath literal can be covariant in result type and contravariant in argument type, etc.