[ConstraintSystem] Allow function-function conversions for keypath literals #39612

Jumhyn · 2021-10-06T14:57:37Z

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.

lib/Sema/CSApply.cpp

MaxDesiatov · 2021-10-08T15:35:25Z

@swift-ci please smoke test

Jumhyn · 2021-10-08T20:58:33Z

@swift-ci please smoke test

xedin · 2021-10-15T06:31:07Z

@Jumhyn Is this ready for review or just a prototype?

Jumhyn · 2021-10-15T11:14:25Z

@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.

xedin · 2021-10-15T16:39:56Z

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.

Jumhyn · 2022-02-08T04:20:44Z

@swift-ci please smoke test

Jumhyn · 2022-02-09T02:41:59Z

test/expr/unary/keypath/keypath.swift

+      // 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}}


Note: we get the same error without the function conversion, e.g.,

let _: KeyPath<Base?, Base> = \Base.base

Filed here.

Jumhyn · 2022-02-09T02:43:21Z

Hey @hborla @xedin I finally had a chance to iron out the last details here, this should be ready for review!

Jumhyn · 2022-02-09T04:01:45Z

@swift-ci please test

Jumhyn · 2022-02-09T04:02:09Z

@swift-ci please test source compatibility

swift-ci · 2022-02-09T09:36:13Z

Build failed
Swift Test OS X Platform
Git Sha - 0e8cfe5aa72e5b5353fe35d651ec48ef57229a6c

Jumhyn · 2022-02-09T16:27:08Z

@swift-ci please test macOS

swift-ci · 2022-02-09T19:31:32Z

Build failed
Swift Test OS X Platform
Git Sha - 0e8cfe5aa72e5b5353fe35d651ec48ef57229a6c

Jumhyn · 2022-02-14T02:59:17Z

@swift-ci please test macOS

Jumhyn · 2022-02-16T21:14:48Z

@swift-ci please test Linux

Jumhyn · 2022-02-16T23:08:49Z

@swift-ci please test linux

xedin · 2022-02-17T00:12:02Z

@Jumhyn This is in my queue, will try to take a look before the end of this week.

xedin

LGTM! I have left a couple of comments inline but nothing major.

lib/Sema/CSApply.cpp

lib/Sema/CSSimplify.cpp

xedin · 2022-02-22T20:01:21Z

@swift-ci please test source compatibility

xedin · 2022-03-18T02:05:54Z

@swift-ci please test source compatibility

unittests/Sema/KeypathFunctionConversionTests.cpp

Jumhyn · 2023-10-11T15:15:57Z

@swift-ci please test

Jumhyn · 2023-10-11T16:31:59Z

lib/Sema/CSSimplify.cpp

+  } else if ((!flags.contains(TMF_GenerateConstraints) &&
+              !anyComponentsUnresolved) ||


@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.

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.

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...

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?

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.

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?

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?

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.

xedin · 2023-10-11T21:39:31Z

lib/Sema/CSSimplify.cpp

+      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))


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.

@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:

Suggested change

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?)

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.

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?

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()) {

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.

xedin · 2023-10-11T21:47:13Z

lib/Sema/CSSimplify.cpp

+  } else if ((!flags.contains(TMF_GenerateConstraints) &&
+              !anyComponentsUnresolved) ||


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.

unittests/Sema/KeypathFunctionConversionTests.cpp

lib/Sema/CSSimplify.cpp

Jumhyn · 2023-12-12T21:34:56Z

@swift-ci please test

Jumhyn · 2023-12-14T02:02:49Z

@swift-ci please test source compatibility

rjmccall · 2023-12-14T03:32:37Z

@swift-ci Please build toolchain

Jumhyn · 2023-12-14T13:07:58Z

@swift-ci please test compiler performance

Jumhyn · 2023-12-14T13:08:14Z

@swift-ci please test Linux platform

Jumhyn · 2024-01-06T15:57:29Z

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?

Jumhyn · 2024-01-17T14:17:55Z

@xedin just bumping now that we're a bit further into the new year!

xedin · 2024-01-17T17:53:11Z

Sorry, I'll try to get some time this week to review this!

xedin · 2024-01-23T03:59:23Z

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

Jumhyn · 2024-01-23T13:52:29Z

@xedin thanks, done!

Jumhyn · 2024-01-23T13:52:45Z

@swift-ci please test

Jumhyn · 2024-01-23T13:52:59Z

@swift-ci please test source compatibility

xedin

Looks good! I left a few comments inline but they can be addressed in a follow-up.

xedin · 2024-01-23T18:46:55Z

include/swift/AST/DiagnosticsSema.def

@@ -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",


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?

xedin · 2024-01-23T18:47:40Z

include/swift/AST/DiagnosticsSema.def

+      "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",


Same here, there is a "cannot convert value of type ... to a contextual type" diagnostic, would be good to align them.

xedin · 2024-01-23T18:53:16Z

lib/Sema/CSSimplify.cpp

+                                         fnTy->getExtInfo());
+
+      return matchTypes(kpFnTy, paramFnTy, ConstraintKind::Conversion, subflags,
+                        locator).isSuccess();


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.

Jumhyn commented Oct 6, 2021

View reviewed changes

lib/Sema/CSApply.cpp Show resolved Hide resolved

Jumhyn force-pushed the keypath-function-conversion branch from d6eb734 to 69ffb18 Compare October 7, 2021 15:05

Jumhyn changed the title ~~[WIP][ConstraintSystem] Allow function-function conversions for keypath literals~~ [ConstraintSystem] Allow function-function conversions for keypath literals Oct 8, 2021

Jumhyn force-pushed the keypath-function-conversion branch from 69ffb18 to 5ac32fe Compare October 8, 2021 20:58

This comment has been minimized.

Sign in to view

Jumhyn force-pushed the keypath-function-conversion branch from 5ac32fe to 0e8cfe5 Compare February 8, 2022 04:18

Jumhyn commented Feb 9, 2022

View reviewed changes

Jumhyn requested review from hborla and xedin February 9, 2022 02:42

Jumhyn added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Feb 9, 2022

xedin approved these changes Feb 18, 2022

View reviewed changes

lib/Sema/CSApply.cpp Outdated Show resolved Hide resolved

lib/Sema/CSApply.cpp Outdated Show resolved Hide resolved

lib/Sema/CSSimplify.cpp Show resolved Hide resolved

Jumhyn force-pushed the keypath-function-conversion branch from 0e8cfe5 to 9a7d9f7 Compare February 19, 2022 21:30

Jumhyn commented Oct 10, 2023

View reviewed changes

unittests/Sema/KeypathFunctionConversionTests.cpp Show resolved Hide resolved

Jumhyn force-pushed the keypath-function-conversion branch from 970aed5 to 01cd88c Compare October 11, 2023 14:39

Jumhyn commented Oct 11, 2023

View reviewed changes

xedin reviewed Oct 11, 2023

View reviewed changes

Jumhyn force-pushed the keypath-function-conversion branch from 01cd88c to a44f501 Compare December 11, 2023 01:52

Jumhyn added swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process and removed swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review labels Jan 6, 2024

Jumhyn requested a review from xedin January 6, 2024 15:55

Allow function-function conversions for keypath literals

0d79f45

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

Jumhyn force-pushed the keypath-function-conversion branch from a44f501 to 0d79f45 Compare January 23, 2024 13:52

xedin approved these changes Jan 23, 2024

View reviewed changes

Jumhyn merged commit 0735629 into swiftlang:main Feb 3, 2024
7 checks passed

Jumhyn mentioned this pull request Feb 3, 2024

Follow ups for keypath function conversion PR #71372

Merged

justice-adams-apple mentioned this pull request Feb 6, 2024

[Source compatibility suite] penny-bot failing to build with main branch #71423

Closed

Jumhyn mentioned this pull request Feb 19, 2024

Fix solution application for keypath function subtype conversions #71737

Merged

		// 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}}

		} else if ((!flags.contains(TMF_GenerateConstraints) &&
		!anyComponentsUnresolved) \|\|

-        if (hasConversionOrRestriction(ConversionRestrictionKind::DeepEquality))
+        if (lhs->getAnyGeneric() == rhs->getAnyGeneric() && llvm::any_of(conversionsOrFixes,
+                        [](const RestrictionOrFix &correction) {
+                          return bool(correction.getRestriction());
+                        }))

[ConstraintSystem] Allow function-function conversions for keypath literals #39612

[ConstraintSystem] Allow function-function conversions for keypath literals #39612

Conversation

Jumhyn commented Oct 6, 2021 • edited

MaxDesiatov commented Oct 8, 2021

Jumhyn commented Oct 8, 2021

This comment has been minimized.

xedin commented Oct 15, 2021

Jumhyn commented Oct 15, 2021

xedin commented Oct 15, 2021

Jumhyn commented Feb 8, 2022

Choose a reason for hiding this comment

Jumhyn commented Feb 9, 2022

Jumhyn commented Feb 9, 2022

Jumhyn commented Feb 9, 2022

swift-ci commented Feb 9, 2022

Jumhyn commented Feb 9, 2022

swift-ci commented Feb 9, 2022

Jumhyn commented Feb 14, 2022

Jumhyn commented Feb 16, 2022

Jumhyn commented Feb 16, 2022

xedin commented Feb 17, 2022

xedin left a comment

Choose a reason for hiding this comment

xedin commented Feb 22, 2022

xedin commented Mar 18, 2022

Jumhyn commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin Oct 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amritpan Oct 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jumhyn commented Dec 12, 2023

Jumhyn commented Dec 14, 2023

rjmccall commented Dec 14, 2023

Jumhyn commented Dec 14, 2023

Jumhyn commented Dec 14, 2023

Jumhyn commented Jan 6, 2024

Jumhyn commented Jan 17, 2024

xedin commented Jan 17, 2024

xedin commented Jan 23, 2024

Jumhyn commented Jan 23, 2024

Jumhyn commented Jan 23, 2024

Jumhyn commented Jan 23, 2024

xedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jumhyn commented Oct 6, 2021 •

edited

xedin Oct 12, 2023 •

edited

amritpan Oct 30, 2023 •

edited