[ConstraintSystem][SE-0249] Key Path Expressions as Functions #26054

gregomni · 2019-07-10T06:49:27Z

This builds on @brentdax's work in #23435 to handle optional chaining key paths as both KeyPath BGTs and as function types, and also does away with explicit disjunctions to determine which kind of type to turn the key path literal into, so ought to be more performant.

There is a single test diagnosis that gets worse here (a contextual type which matches root/value but specifies an incorrect writability is now ambiguous because the diagnostic path goes into the old CSDiag stuff, and the first step is to discard contextual type, and then the remainder is entirely ambiguous). The solution is future work to improve key path diagnoses in general, I think.

gregomni · 2019-07-10T06:54:07Z

@swift-ci Please smoke test.

beccadax · 2019-07-10T16:26:42Z

From the smoke test, it looks like that didn't regress after all.

beccadax · 2019-07-10T16:31:12Z

lib/Sema/CSRanking.cpp

@@ -1139,17 +1139,6 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
      continue;
    }

-    // If one is a function type and the other is a key path, prefer the other.
-    if (type1->is<FunctionType>() != type2->is<FunctionType>()) {


How do we avoid needing a scoring rule?

The change in the implementation of simplifyKeyPathConstraint avoids creating a disjunction between the two forms now, so there shouldn't ever be two solutions. It leaves a single KeyPathConstraint until there's enough info so that the simplify can pick either function or KeyPath nominal.

I went this way because the constraint system isn't smart enough / has too much ambiguity to figure out key paths with optional chaining without additional help. Thus the somewhat obscure condition at the end of simplify'ing !anyComponentsUnresolved || (definitelyKeyPathType && capability == ReadOnly) -- even if we haven't figured out all the key path component overloads, if we can tell that the literal path will result in a KeyPath nominal and that it'll be read-only (as in an optional chain), then we can immediately bind to the correct nominal type (because no possible component overload can make it non-read-only once a component specifies that it is).

That's essentially a replacement for the logic that used to be in CSGen that immediately bound to a type if there was an optional chain involved instead of creating a KeyPathConstraint at all. The constraint system needs that top-down info in order to figure out the bottom-up overloads for the components.

What happens if you write a key path in a context with no type information at all, like this?

let kp = \A.property let _: KeyPath<A, Prop> = kp

(There should probably be a test like this if there isn't one already.)

If there isn't context that definitively asks for function type vs nominal type, then once the components are all resolved, simplification defaults to the nominal type. (And I'll specifically add tests like that in a moment.)

Hmm...could we end up in a situation where we currently don't have any context, but the typechecker would discover more context in the future if we left the constraint unsolved? @xedin, any thoughts?

gregomni · 2019-07-10T16:50:29Z

From the smoke test, it looks like that didn't regress after all.

Woo! Some other recent change must've improved this, nice!

gregomni · 2019-07-10T16:54:51Z

@swift-ci Please smoke test.

gregomni · 2019-07-11T03:04:02Z

@swift-ci Please smoke test.

gregomni · 2019-07-11T14:44:25Z

@swift-ci Please smoke test Linux.

xedin

I left a couple of comments inline. But I really want for us to consider a meta-question here - should keypath handling be re-designed in a way where keypath type drives access to the components instead of components determining what access is going to be? That way it should be much easier to diagnose invalid writable calls and avoid creating all of the conversions from l-value result to r-value result types.

While generating constraints check whether there is a contextual type - if so, everything is easy, otherwise start with assumption that it's a WritableKeyPath generate constraints for components and try to solve (I think it might make sense to introduce <baseType> KeyPathComponent #N <resultType>), if that fails try read-only keypath and/or function type.

Solving keypath would mean solving all of the associated components so we no longer have to wait for overload choices to be made and re-generate the same constraint or iterate over components multiple times.

xedin · 2019-07-18T21:03:27Z

lib/Sema/CSSimplify.cpp

@@ -5514,6 +5519,26 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
      return SolutionKind::Solved;
    };

+  auto tryMatchRootAndValueFromFunctionType =
+      [&](FunctionType *fnTy) -> SolutionKind {


Could you please reuse logic for matching for root/value types between here and tryMatchRootAndValueFromKeyPathType?

Even better would be to make it so tryMatchRootAndValueFromKeyPathType accepts a Type and then figures out whether it's a function or bound generic, or something else. That way we wouldn't need special cases for function type vs. bound generic in multiple places.

Good idea, done!

xedin · 2019-07-18T21:06:24Z

lib/Sema/CSSimplify.cpp

+  // type variable that's some sort of conversion to a function type, use that
+  // constraint's second type.
+  if (keyPathTy->isTypeVariableOrMember()) {
+    for (auto constraint : getActiveConstraints()) {


Can we delay solving this constraint until keyPathTy is property resolved instead of trying to guess what it might be?

No, unfortunately. In a simple function type example like:

struct Struct { var i: Int } _ = (Array<Struct>()).map(\.i)

The keyPathTy is convertible to (Struct)->T, but since T is unknown without figuring out the key path components, and keyPathTy itself is still just a type variable, the constraint system can't make progress without this code to bind the root and value types, so the result is an error: type of expression is ambiguous without more context.

Interesting, this sounds like we don't model this "correctly", if there is a conversion constraint associated with keypath in theory we should be able to delay solving keypath constraint until type for keypath type is attempted. I'm going to pull these changes in and try it out locally to see what is up.

I have looked at this for a bit tonight and it looks like what we need is special handling in matchTypes in cases where argument is a type variable representing a keypath e.g.:

@@ -2657,8 +2657,16 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind, case ConstraintKind::Subtype: case ConstraintKind::Conversion: case ConstraintKind::ArgumentConversion: - case ConstraintKind::OperatorArgumentConversion: + case ConstraintKind::OperatorArgumentConversion: { + if (typeVar1) { + if (auto *locator = typeVar1->getImpl().getLocator()) { + if (locator->isLastElement(ConstraintLocator::KeyPathType)) + return matchTypesBindTypeVar(typeVar1, type2, kind, flags, locator, + formUnsolvedResult); + } + } return formUnsolvedResult(); + }

This seems to work, although logic at the end of simplifyKeyPathConstraint which deals with keyPathTy being a function type has to be adjusted too because it doesn't have to bind function types together anymore if keyPathTy is already a function type.

The logic at the end of simplifyKeyPathConstraint is easy to fix for function types:

if (definitelyFunctionType) { return SolutionKind::Solved;

The problem then is with KeyPath bound generic types, which regress in a bunch of tests. This is most easily fixable if you adjust the matchTypes handling to only work on function types:

if (locator->isLastElement(ConstraintLocator::KeyPathType) && type2->is<AnyFunctionType>())

That sound okay?

It seems reasonable, sorry I didn't run all of the tests at the time. Do you happen to have an example of how they fail? I'm just curious whether we could fix that without special case in matchTypes.

It looked like mostly bad key path type error diagnoses because the solver wasn't left in a state that the diagnostic code was ready for. E.g.:

struct S { let i: Int } let _ : WritableKeyPath<S, Int> = \.i

The key path tyvar gets bound to WritableKeyPath from the contextual type because of the matchTypes change, then the key path constraint gets simplified, sees that i is a let and tries to bind (read only) KeyPath as the type, and the two don't match. The regression is to ambiguous without more context instead of the targeted diagnosis.

Related:

let _: KeyPath<S, Int>? = \.i

Binds the key path tyvar to an optional directly, instead of resolving the path and being convertible to optional. So a failure to compile something which should work.

I guess it would be fair to fix cases like that separately, but please leave a comment with TODO(diagnostics): in relevant place of matchTypes.

gregomni · 2019-08-03T16:26:27Z

@swift-ci Please smoke test.

gregomni · 2019-08-03T16:31:40Z

@xedin Long-term, your suggestion for how to rewrite key path constraint generation and simplification would be great, and would probably also make diagnoses for key path errors a lot better. But I think that that future potential improvement is separable from this feature / PR.

gregomni · 2019-08-03T20:27:15Z

@swift-ci Please smoke test.

xedin · 2019-08-05T19:45:32Z

lib/Sema/CSApply.cpp

+      auto closureTy =
+          FunctionType::get({ FunctionType::Param(baseTy) }, leafTy);
+      auto closure = new (ctx)
+          AutoClosureExpr(E, leafTy, discriminator, cs.DC);


Why not just a regular implicit closure? I believe that by conversion autoclosure is not supposed to have any arguments.

The problem is the discriminator. ClosureExprs get numbered during parsing, and AutoClosureExprs get numbered during a walk after type checking. The parsing context is already gone by the time this code is running, so the only way to correctly set the discriminator is via an AutoClosureExpr.

Interesting, and there is no way to change that behavior to allow implicit closures? It seems a bit like a hack to use autoclosure here, because autoclosure is not supposed to have any parameters and used only in @autoclosure marked positions, so we are kind of stretching its design here...

Well, the superclass AbstractClosureExpr is what handles parameters, not either of the subclasses. The main difference between ClosureExpr and AutoClosureExpr is that ClosureExpr is supposed to be for explicit closures parsed from the source code, and AutoClosureExpr is supposed to be for implicitly created closures around an expression.

So, it's true, this is the only place where AutoClosureExpr will currently have parameters. But the design of the classes is such that I think we'd be stretching the design much more to try to use ClosureExpr here.

Maybe it would make sense to add a separate sub-class of AbstractClosureExpr which deals with general cases of implicit closures? I'm just trying to think through ideas here since AutoClosureExpr just seems like a specialized use-case to me. Maybe it doesn't really matter that much anyway since the only code which has to deal with that in in SILGen... I can't think of any special cases in CSDiag which deal with expressions like that.

At the very least comment associated with AutoClosureExpr has to be updated to cover new use-case.

gregomni · 2019-08-05T23:25:23Z

@swift-ci Please smoke test.

Fix autoclosure param interface type when it involves archetypes. Had some repeated locals from moving this block of code around - cleaned up. Alternate implementation where KeyPathExpr is essentially a literal type and can be either a KeyPath(R,V) or (R)->V. Some unneccessary code now. Implicit closure pieces need valid sourceLocs in case a coerce expr needs to refer to the beginning of the closure in `\.foo as (A) -> B`. Removed explicit noescape from function type so `let a: (A) -> B = \.foo` is valid. Remove optimization that optional path is always read-only KP type in CSGen, since it can also now be of function type.

Necessary to keep SE-0249 from failing tests.

…uish function type and keypath BGT type without explicit disjunctions.

(Happened when merging latest master into this branch.)

… the solver.

gregomni · 2019-08-06T14:53:24Z

@swift-ci Please smoke test.

gregomni · 2019-08-07T14:14:46Z

@swift-ci Please smoke test.

xedin

LGTM! @gregomni thank you for sticking with this!

xedin · 2019-08-07T21:56:03Z

@swift-ci please test source compatibility

theblixguy · 2020-01-09T13:28:58Z

@gregomni Do you mind adding this to changelog under Swift 5.2 and update the proposal as "Implemented" on Swift evolution repo?

Done here swiftlang/swift#26054

gregomni requested a review from beccadax July 10, 2019 06:49

beccadax reviewed Jul 10, 2019

View reviewed changes

xedin self-requested a review July 12, 2019 21:00

xedin reviewed Jul 18, 2019

View reviewed changes

gregomni requested review from xedin and beccadax August 3, 2019 16:32

gregomni force-pushed the kp_closures branch from 06e355d to 199efb6 Compare August 3, 2019 20:26

xedin reviewed Aug 5, 2019

View reviewed changes

gregomni and others added 12 commits August 6, 2019 07:52

[ConstraintSolver] Favor keypaths over functions

6f4b0e5

Necessary to keep SE-0249 from failing tests.

Add typechecking tests

ffe1684

Add runtime tests for key path literal closures

2a885a0

Make keypath literal in closure only once

66f914d

Allow optional-chaining key paths to match function types

1a9be78

Distinguish subscript member type from result type

33c68e0

Handle simplification of optional-chaining key path literals, disting…

a2cd53d

…uish function type and keypath BGT type without explicit disjunctions.

Didn't regress on this diagnosis after all!

521605c

These two diagnoses changed for the better, not sure why.

3419e5d

(Happened when merging latest master into this branch.)

Add tests with no contextual type info.

bf3a786

Combine internal tryMatchRootAndValue funcs for cleaner code.

9ab22cf

gregomni added 2 commits August 6, 2019 07:52

Update to HEAD, fix Param type changes, merging.

469e06a

Handle conversions of keypath tyvars to function type specifically in…

9c0aad3

… the solver.

gregomni force-pushed the kp_closures branch from 7835946 to 9c0aad3 Compare August 6, 2019 14:53

Added comment explaining additional use of AutoClosureExpr.

9107626

xedin approved these changes Aug 7, 2019

View reviewed changes

gregomni merged commit db3b0d9 into swiftlang:master Aug 8, 2019

mackoj added a commit to mackoj/swift-evolution that referenced this pull request Jan 10, 2020

Update 0249-key-path-literal-function-expressions.md

3d4f82b

Done here swiftlang/swift#26054

gregomni deleted the kp_closures branch August 18, 2020 23:13

[ConstraintSystem][SE-0249] Key Path Expressions as Functions #26054

[ConstraintSystem][SE-0249] Key Path Expressions as Functions #26054

Conversation

gregomni commented Jul 10, 2019

gregomni commented Jul 10, 2019

beccadax commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beccadax Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregomni commented Jul 10, 2019

gregomni commented Jul 10, 2019

gregomni commented Jul 11, 2019

gregomni commented Jul 11, 2019

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

Choose a reason for hiding this comment

gregomni Aug 3, 2019 • edited Loading

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

gregomni commented Aug 3, 2019

gregomni commented Aug 3, 2019

gregomni commented Aug 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin Aug 7, 2019 • edited Loading

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

gregomni commented Aug 5, 2019

gregomni commented Aug 6, 2019

gregomni commented Aug 7, 2019

xedin left a comment

Choose a reason for hiding this comment

xedin commented Aug 7, 2019

theblixguy commented Jan 9, 2020 • edited Loading

beccadax Jul 11, 2019 •

edited

Loading

gregomni Aug 3, 2019 •

edited

Loading

xedin Aug 7, 2019 •

edited

Loading

theblixguy commented Jan 9, 2020 •

edited

Loading