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

Add root type constraint between KeyPath expressions and applications #17094

Merged
merged 1 commit into from Jun 13, 2018

Conversation

Projects
None yet
7 participants
@mdiep
Contributor

mdiep commented Jun 10, 2018

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.

include/swift/AST/Expr.h Outdated
@@ -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 comment has been minimized.

@slavapestov

slavapestov Jun 10, 2018

Member

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

This comment has been minimized.

@mdiep

mdiep Jun 10, 2018

Contributor

Submitted as #17098.

@slavapestov

This comment has been minimized.

Member

slavapestov commented Jun 10, 2018

@jckarter Want to take a look?

lib/Sema/CSSimplify.cpp Outdated
for (auto constraint : InactiveConstraints) {
if (constraint.getKind() == ConstraintKind::KeyPath &&
constraint.getLocator()->getAnchor() == keyPathExpr) {
addConstraint(ConstraintKind::Subtype, root, constraint.getSecondType(), locator);

This comment has been minimized.

@CodaFi

CodaFi Jun 10, 2018

Collaborator

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

This comment has been minimized.

@mdiep

mdiep Jun 11, 2018

Contributor

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!

This comment has been minimized.

@xedin

xedin Jun 11, 2018

Member

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

This comment has been minimized.

@mdiep

mdiep Jun 11, 2018

Contributor

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.

This comment has been minimized.

@CodaFi

CodaFi Jun 11, 2018

Collaborator

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;

This comment has been minimized.

@CodaFi

CodaFi Jun 11, 2018

Collaborator

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.

This comment has been minimized.

@mdiep

mdiep Jun 11, 2018

Contributor

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…

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

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

This comment has been minimized.

@CodaFi

CodaFi Jun 11, 2018

Collaborator

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.

This comment has been minimized.

@xedin

xedin Jun 11, 2018

Member

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 rudkx self-requested a review Jun 11, 2018

@rudkx

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

test/expr/unary/keypath/keypath.swift Outdated
func sr7380() {
_ = ""[keyPath: \.count]
}

This comment has been minimized.

@rudkx

rudkx Jun 11, 2018

Member

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?

This comment has been minimized.

@mdiep

mdiep Jun 11, 2018

Contributor

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

@jckarter

This comment has been minimized.

Member

jckarter commented Jun 11, 2018

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

This comment has been minimized.

Member

jckarter commented Jun 11, 2018

@swift-ci Please smoke test

@jckarter

This comment has been minimized.

Member

jckarter commented Jun 11, 2018

@swift-ci Please test source compatibility

lib/Sema/CSSimplify.cpp Outdated
for (auto constraint : InactiveConstraints) {
if (constraint.getKind() == ConstraintKind::KeyPath &&
constraint.getLocator()->getAnchor() == keyPathExpr) {
auto keyPathRootTy = constraint.getSecondType();

This comment has been minimized.

@CodaFi

CodaFi Jun 11, 2018

Collaborator

Collapse this

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

This comment has been minimized.

Contributor

mdiep commented Jun 11, 2018

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

This comment has been minimized.

Member

jckarter commented Jun 11, 2018

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

@mdiep

This comment has been minimized.

Contributor

mdiep commented Jun 11, 2018

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.

lib/Sema/CSSimplify.cpp Outdated
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) {

This comment has been minimized.

@rudkx

rudkx Jun 11, 2018

Member

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)
  }
lib/Sema/CSSimplify.cpp Outdated
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))) {

This comment has been minimized.

@rudkx

rudkx Jun 11, 2018

Member

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

This comment has been minimized.

Member

jckarter commented Jun 11, 2018

@swift-ci Please smoke test

@jckarter

This comment has been minimized.

Member

jckarter commented Jun 11, 2018

@swift-ci Please test source compatibility

@mdiep

This comment has been minimized.

Contributor

mdiep commented Jun 13, 2018

How does that look, @rudkx?

@rudkx

rudkx approved these changes Jun 13, 2018

LGTM!

@rudkx

This comment has been minimized.

Member

rudkx commented Jun 13, 2018

@swift-ci Please smoke test

@rudkx

This comment has been minimized.

Member

rudkx commented Jun 13, 2018

@swift-ci Please test source compatibility

@mdiep

This comment has been minimized.

Contributor

mdiep commented Jun 13, 2018

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

@rudkx

This comment has been minimized.

Member

rudkx commented Jun 13, 2018

@swift-ci Please smoke test

@jckarter jckarter merged commit cb3d6e2 into apple:master Jun 13, 2018

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@mdiep mdiep deleted the mdiep:SR-7380 branch Jun 13, 2018

@jrose-apple

This comment has been minimized.

Member

jrose-apple commented Jun 13, 2018

Worth cherry-picking into 4.2?

@jckarter

This comment has been minimized.

Member

jckarter commented Jun 14, 2018

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment