Add root type constraint between KeyPath expressions and applications #17094

mdiep · 2018-06-10T17:52:26Z

This key path application was failing to resolve the key path:

_ = ""[keyPath: \.count]

This adds a constraint between the type the KeyPath application is subscripting and the base type of the KeyPath expression inside of it.

Resolves SR-7380.

slavapestov · 2018-06-10T20:14:56Z

include/swift/AST/Expr.h

@@ -2300,7 +2300,7 @@ class SubscriptExpr final : public LookupExpr,
    return Bits.SubscriptExpr.HasArgLabelLocs;
  }

-  /// Whether this call with written with a trailing closure.
+  /// Whether this call was written with a trailing closure.


This should be a separate patch - if the other fix gets reverted for whatever reason your typo fix will live on :)

Submitted as #17098.

slavapestov · 2018-06-10T20:15:32Z

@jckarter Want to take a look?

CodaFi · 2018-06-10T20:45:27Z

lib/Sema/CSSimplify.cpp

+            for (auto constraint : InactiveConstraints) {
+              if (constraint.getKind() == ConstraintKind::KeyPath &&
+                  constraint.getLocator()->getAnchor() == keyPathExpr) {
+                addConstraint(ConstraintKind::Subtype, root, constraint.getSecondType(), locator);


This typing rule seems to be duplicating the existing rule in CSGen (after drilling). Try looking at what visitUnresolvedMemberExpr is doing.

I'm afraid I don't understand which existing rule you're referring to or what specifically in visitUnresolvedMemberExpr you're pointing me to. 😞 I'll need some more help to understand. Sorry!

I think we should try our best to do this in CSGen because we are trying to avoid dealing with expressions in the solver at all costs...

In CSGen you don't know that this is a KeyPath application—it's just a subscript. Because this is inside the subscript overload, I don't think it can be in CSGen. Or maybe I missing something?

But it should be possible to move it out a level to ConstraintSystem::resolveOverload if that's preferable.

In CSGen you don't know that this is a KeyPath application

I think it can be done. A naive way would be to add an extra "faux root" expression node where you can stash the type variable generated for the root expression in the KeyPath. Then in the post-walk, you can recreate this logic (minus the locator digging) and directly bind the subscript's base to the faux root. For example (don't actually do this, please clean it up):

// ConstraintWalker::walkToExprPost if (auto subscript = dyn_cast<SubscriptExpr>(expr)) { if (auto indexTuple = dyn_cast<TupleExpr>(subscript->getIndex())) { if (indexTuple->getNumElements() == 1) { if (auto keyPathExpr = dyn_cast<KeyPathExpr>(indexTuple->getElement(0))) { auto type = CG.visit(expr); auto &CS = CG.getConstraintSystem(); auto simplifiedType = CS.simplifyType(type); CS.setType(expr, simplifiedType); CS.addConstraint(ConstraintKind::Subtype, CS.getType(subscript->getBase()), CS.getType(keyPathExpr->getFauxRoot()), CS.getConstraintLocator(expr)); return expr; } } } } // ... // ConstraintWalker::visitKeyPathExpr case KeyPathExpr::Component::Kind::UnresolvedProperty: E->setFauxRoot(/**/); CS.setType(E->getFauxRoot(), root); LLVM_FALLTHROUGH;

Essentially, you've hit the nail on the head with the fix here: the constraint system we set up for unresolved member key paths doesn't actually bind the root type variable to anything useful - the value member constraint is just going to fail on an unbound type variable. Now we've just got to do some work to keep the required contextual type propagation clean.

That assumes that a [keyPath: <key path>] expression is a KeyPath application. I was assuming that shouldn't be hardcoded, but it actually is today. i.e., this fails to compile:

extension String { subscript(keyPath: KeyPath<Int, String>) -> String { return "🙈" } } _ = ""[keyPath: \Int.description]

Since that's apparently not valid, it should be fine to add this constraint outside of the KeyPath application constraint. (Although that kinda seems like a bug to me. Why shouldn't you be able to overload a subscript that takes a KeyPath?)

Given that…

Is there any reason it's preferable to use walkToExprPost over visitSubscriptExpr? As long as we're going to assume that [keyPath: <key path>] is a key path application, we might as well remove the overload and do this directly in visitSubscriptExpr?

Is it still preferable to create another expression instead of using the locator? Neither seem ideal to me. 😕 That would mean adding a new type like KeyPathRootExpr, right?

Is there any reason it's preferable to use walkToExprPost over visitSubscriptExpr

I use it out of convenience. Anywhere in the walk that has access to the subscript and its children after they have had constraints generated will do.

As long as we're going to assume that [keyPath: ] is a key path application, we might as well remove the overload and do this directly in visitSubscriptExpr

No such luck in the general case

extension String { subscript<T>(keypath a: T) -> String { return "🙈" } } _ = ""[keypath: 5]

Is it still preferable to create another expression instead of using the locator

Digging in the locators is a hack, adding a fake AST node is a hack. Neither is desirable. You shouldn't need a new type for this, the whole goal of the example I gave was that you need to stash the type variable generated for root somewhere that's accessible to the rest of CSGen.

The suggestion @CodaFi made it along the lines I was thinking as well, but I'm not sure if we really need to create a faux root through, because even if we still let root allocate new type variable but then connect it to something which can inferred that would still be good enough. Another idea, but I'm not sure how feasible that would be, is to actually make it so KeyPathExpr, when created, knows what expression it's attached to, that would be very straightforward to get its type from the constraint system cache and relate it to root in visitKeyPathExpr.

rudkx

I still need to take a closure look at the code changes.

rudkx · 2018-06-11T06:48:07Z

test/expr/unary/keypath/keypath.swift

+func sr7380() {
+  _ = ""[keyPath: \.count]
+}
+


Can you add a tests for array and dictionary example as well, with 'let', 'var', and collection literal variants, e.g.:

let arr = [1] _ = arr[keyPath: \.[0]] let dic = [1:"s"] _ = dic[keyPath: \.[1]]

and then versions for var rather than let, as well as ones that use collection literals rather than the named collections?

Great suggestion. The var case wasn't working; I've fixed that up.

jckarter · 2018-06-11T15:19:31Z

Looks good, thanks! When I tried this originally, I had issues getting the type system to keep working in the face of PartialKeyPath and maintain the correct mutability relationships for Writable and ReferenceWritable key paths. I saw Mark had you add some for writable key paths, but having some for class instance property key paths would be nice too, and it'd be a good idea to also add tests to ensure that both literal key paths and variables work, as well as PartialKeyPaths.

jckarter · 2018-06-11T15:19:43Z

@swift-ci Please smoke test

jckarter · 2018-06-11T15:19:56Z

@swift-ci Please test source compatibility

CodaFi · 2018-06-11T15:48:33Z

lib/Sema/CSSimplify.cpp

+            for (auto constraint : InactiveConstraints) {
+              if (constraint.getKind() == ConstraintKind::KeyPath &&
+                  constraint.getLocator()->getAnchor() == keyPathExpr) {
+                auto keyPathRootTy = constraint.getSecondType();


Collapse this

addConstraint(ConstraintKind::Subtype, root->getWithoutSpecifierType(), keyPathRootTy, locator);

mdiep · 2018-06-11T16:38:57Z

but having some for class instance property key paths would be nice too,

I added a couple tests for this. This error was even worse before:

7380.swift:23:5: error: type 'A' has no subscript members
_ = A()[keyPath: \.a]
    ^~~

it'd be a good idea to also add tests to ensure that both literal key paths and variables work, as well as PartialKeyPaths

I added some tests for \Type.property key paths, but variables seem well tested already. (And since those are type-checked separately, they're pretty orthogonal to this bug and fix.)

jckarter · 2018-06-11T18:27:26Z

How bad was the diagnostic before? @xedin and @rudkx might be able to advise on how to win back some diagnostic quality.

mdiep · 2018-06-11T18:35:57Z

Oh, sorry, I meant that _ = A()[keyPath: \.a] had that nonsense error before but now compiles with this fix.

rudkx

Looks good. A couple small notes of things you should take a look at.

It would be great if we didn't have to iterate over constraints to do this, but offhand I cannot think of how we would do that.

rudkx · 2018-06-11T19:42:35Z

lib/Sema/CSSimplify.cpp

+      if (auto indexTuple = dyn_cast<TupleExpr>(subscript->getIndex())) {
+        if (indexTuple->getNumElements() == 1) {
+          if (auto keyPathExpr = dyn_cast<KeyPathExpr>(indexTuple->getElement(0))) {
+            for (auto constraint : InactiveConstraints) {


Rather than iterating over all constraints here, and checking the anchor expression, you should be able to do something like:

auto *typevar = cs.getType(keyPathExpr)->getAs<TypeVariableType>(); if (typevar) { CG.gatherConstraints(typevar, constraints, ConstraintGraph::GatheringKind::EquivalenceClass); // iterate over only those looking for a ConstraintKind::KeyPath where the constraint.getFirstType()->isEqual(typevar) }

rudkx · 2018-06-11T19:45:15Z

lib/Sema/CSSimplify.cpp

+        path[0].getKind() == ConstraintLocator::SubscriptMember) {
+      if (auto indexTuple = dyn_cast<TupleExpr>(subscript->getIndex())) {
+        if (indexTuple->getNumElements() == 1) {
+          if (auto keyPathExpr = dyn_cast<KeyPathExpr>(indexTuple->getElement(0))) {


I suspect some of the above logic could be turned into cast<>() and asserts based on our expectations of what we know when we decide an overload is a key path application.

For whatever remains, it might be nice to remove some of the depth of control flow via the advice in https://www.llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

You could for example make this a closure with early returns, a separate function, or a:

do { // use break in places where we bail out } while (0);

jckarter · 2018-06-11T19:49:08Z

@swift-ci Please smoke test

jckarter · 2018-06-11T19:49:16Z

@swift-ci Please test source compatibility

mdiep · 2018-06-13T13:27:50Z

How does that look, @rudkx?

rudkx

LGTM!

rudkx · 2018-06-13T13:48:20Z

@swift-ci Please smoke test

rudkx · 2018-06-13T13:48:28Z

@swift-ci Please test source compatibility

mdiep · 2018-06-13T16:39:39Z

I had to switch the check for a tuple back to a dyn_cast.

rudkx · 2018-06-13T18:01:04Z

@swift-ci Please smoke test

jrose-apple · 2018-06-13T21:03:12Z

Worth cherry-picking into 4.2?

jckarter · 2018-06-14T02:12:47Z

Sounds good to me!

slavapestov reviewed Jun 10, 2018

View reviewed changes

CodaFi reviewed Jun 10, 2018

View reviewed changes

mdiep force-pushed the SR-7380 branch from fba0658 to b8542d3 Compare June 10, 2018 23:55

rudkx self-requested a review June 11, 2018 02:49

rudkx reviewed Jun 11, 2018

View reviewed changes

mdiep force-pushed the SR-7380 branch from b8542d3 to 6c22d2b Compare June 11, 2018 14:45

CodaFi reviewed Jun 11, 2018

View reviewed changes

mdiep force-pushed the SR-7380 branch 2 times, most recently from b76e6f8 to 2bf8470 Compare June 11, 2018 16:37

rudkx requested changes Jun 11, 2018

View reviewed changes

mdiep force-pushed the SR-7380 branch 2 times, most recently from 1fb5037 to 4525eaf Compare June 13, 2018 13:26

rudkx approved these changes Jun 13, 2018

View reviewed changes

Add root type constraint between KeyPath expressions and applications

456f69e

mdiep force-pushed the SR-7380 branch from 4525eaf to 456f69e Compare June 13, 2018 16:39

jckarter merged commit cb3d6e2 into swiftlang:master Jun 13, 2018

mdiep deleted the SR-7380 branch June 13, 2018 19:53

mdiep mentioned this pull request Jun 13, 2018

[SR-7380] KeyPath cannot infer type in [keyPath:] context #49928

Closed

rudkx mentioned this pull request Jul 23, 2018

[SR-8335] Cannot type check MemoryLayout.offset(of:) without specifying base type in keypath #50863

Closed

Add root type constraint between KeyPath expressions and applications #17094

Add root type constraint between KeyPath expressions and applications #17094

Uh oh!

Conversation

mdiep commented Jun 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Jun 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jun 11, 2018

Uh oh!

jckarter commented Jun 11, 2018

Uh oh!

jckarter commented Jun 11, 2018

Uh oh!

CodaFi Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdiep commented Jun 11, 2018

Uh oh!

jckarter commented Jun 11, 2018

Uh oh!

mdiep commented Jun 11, 2018

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jun 11, 2018

Uh oh!

jckarter commented Jun 11, 2018

Uh oh!

mdiep commented Jun 13, 2018

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

rudkx commented Jun 13, 2018

Uh oh!

rudkx commented Jun 13, 2018

Uh oh!

mdiep commented Jun 13, 2018

Uh oh!

rudkx commented Jun 13, 2018

Uh oh!

jrose-apple commented Jun 13, 2018

Uh oh!

jckarter commented Jun 14, 2018

CodaFi Jun 11, 2018 •

edited

Loading