[CSSimplify] Reject key path if root type is AnyObject

[theblixguy]

Follow up to #23497 based on Slava's suggestion. Moved the logic for checking if a KeyPath root type is AnyObject from CSApply to solver, so we can reject a solution if the root type is AnyObject.

Resolves: rdar://problem/49413561

[theblixguy]

cc @jckarter @slavapestov

[slavapestov]

Thank you for following up on this!

[xedin]

LGTM! Before we merge could you please add some more tests e.g.

let _: KeyPath<AnyObject, Int> = \.<valid-member>

and something like:

func foo(_ obj: AnyObject, _ kp: KeyPath<AnyObject, Int>) {
  _ = obj[keyPath: \.<valid-member>]
  _ = obj[keyPath: kp]
}

[theblixguy]

@xedin Done!

[xedin]

@jckarter Can you please take a look as well?

[xedin]

@swift-ci please smoke test

[theblixguy]

Two code completion related failures, not sure how these tests work so need to figure out what's broken. Also some failures in stdlib/Keypath.swift tests.

I think this has to do with returning an unsolved constraint.

[theblixguy]

Ok so the code that's broken is test/stdlib/KeyPath.swift:748 -

keyPath.test("offsets") {
  let SLayout = MemoryLayout<S<Int>>.self
  ...
  expectEqual(SLayout.offset(of: \.self), 0) // expression type 'Int?' is ambiguous without more context
}

is this expected? Auto complete does not provide any suggestions as well after the dot.

[jckarter]

That code looks like it ought to compile.

[theblixguy]

Hmm it's due to creating an unsolved KeyPath constraint if the root type of the KeyPath is a type variable. If I don't do that, then the test passes. @xedin any ideas? Below is the CS dump:

Score: 0 0 0 0 0 0 0 0 0 0 0
Type Variables:
  $T0 [lvalue allowed] as MemoryLayout<S<Int>>.Type @ locator@0x7fbb2d00a818 [DeclRef@/Users/suyashsrijan/Desktop/test_file.swift:4:9]
  $T1 [lvalue allowed] as (PartialKeyPath<S<Int>>) -> Int? @ locator@0x7fbb2d00a8a0 [UnresolvedDot@/Users/suyashsrijan/Desktop/test_file.swift:4:17 -> member]
  $T2 as S<Int> @ locator@0x7fbb2d00a958 [UnresolvedDot@/Users/suyashsrijan/Desktop/test_file.swift:4:17 -> member -> generic parameter 'T']
  $T3 subtype_of_existential bindings={} @ locator@0x7fbb2d00ab48 [KeyPath@/Users/suyashsrijan/Desktop/test_file.swift:4:28]
  $T4 equivalent to $T3 @ locator@0x7fbb2d00ab48 [KeyPath@/Users/suyashsrijan/Desktop/test_file.swift:4:28]
  $T5 bindings={(subtypes of) PartialKeyPath<S<Int>>} @ locator@0x7fbb2d00ab48 [KeyPath@/Users/suyashsrijan/Desktop/test_file.swift:4:28]
  $T6 as Int? @ locator@0x7fbb2d00ac98 [Call@/Users/suyashsrijan/Desktop/test_file.swift:4:17 -> function result]

Active Constraints:

Inactive Constraints:
  $T5 key path from $T3 -> $T4 [[locator@0x7fbb2d00ab48 [KeyPath@/Users/suyashsrijan/Desktop/test_file.swift:4:28]]];
  $T5 arg conv PartialKeyPath<S<Int>> [[locator@0x7fbb2d00ad50 [Call@/Users/suyashsrijan/Desktop/test_file.swift:4:17 -> apply argument -> comparing call argument #0 to parameter #0]]];
Resolved overloads:
  selected overload set choice MemoryLayout<S<Int>>.Type.offset: $T1 == (PartialKeyPath<$T2>) -> Int?
  selected overload set choice SLayout: $T0 == MemoryLayout<S<Int>>.Type


Opened types:
  locator@0x7fbb2d00a8a0 [UnresolvedDot@/Users/suyashsrijan/Desktop/test_file.swift:4:17 -> member] opens τ_0_0 -> $T2

Do we actually need the unsolved constraint (at least in simplifyKeyPathConstraint())? All tests pass without it. Is there an example where not adding an unsolved constraint would lead to error? I know Slava mentioned a scenario earlier, but if we won't ever come across such code then it may be fine to remove it.

[xedin]

@theblixguy Unfortunately we have to do that for correctness, otherwise there is no way to ensure that such type variable is not going to get bound to AnyObject in the future. I think another (potentially even better place) to check for this might be in matchTypesBindTypeVar if the locator of the type variable makes it out to be a root of the key path then check what we are trying to bind it to. That way we don't have to wait until it's bound to something and re-create constraints over and over...

I also want to clean up keypath constraint when I get a chance because we shouldn't even consider it until all of the components are resolved.

[theblixguy]

@xedin Do you mean something like:

if (isa<KeyPathExpr>(typeVar->getImpl().getLocator()->getAnchor())) {
  if (type->isAnyObject()) {
    // Create and record fix
  }
}

? I am not sure if there is a way to check if a locator points to the root of a key path (we don't have a path element for that, only for a key path component and we don't have one for the example above). I was thinking if we could record the ID of the type variable for the key path's root type then we can later check which type we're assigning to the type variable.

[xedin]

@theblixguy Must suggestion is to create a separate locator path element for root and value type variables related to subscript so it's easily identifiable.

[theblixguy]

@xedin Sorry, I don't understand. Are you saying I should add new path elements to the PathElementKind enum and then specify them in CSGen when creating the KeyPath constraint or type variable for the root type?. Also, why related to subscripts?

[xedin]

Sorry, I meant keypath :) Yeah, that's exactly what I mean, root type variable should be identified as (expr, ConstraintLocator::KeyPathRoot) and value one should be (expr, ConstraintLocator::KeyPathValue).

[xedin]

I will take a look at the a bit later today, but we should diagnose that. I remember that keypath overload is treated specially, so it might be possible to identify root type or diagnose inline.

[xedin]

@theblixguy You can check for baseType being AnyObject here https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L3594 and create unviable member with new reason something like UR_KeyPathWithAnyObjectRootType which then could be converted into a choice with a fix in https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L4257

[theblixguy]

Thanks, I have made the change in commit d11b259, however the fix isn't being picked up and the switch statement in fixMemberRef isn't triggered as well. Maybe I am missing something...

[xedin]

Ah, that's probably because that choice doesn't have a declaration, see at the top of d11b259#diff-69306f7b0822cf52e8c0639307ad7407R4269 . It looks fixMemberRef has to be refactored to support that. I can do that in the separate PR if you want?

[theblixguy]

That would be great @xedin!

[xedin]

@theblixguy #23997

[xedin]

@swift-ci please smoke test

[theblixguy]

Unrelated build failure on macOS (something to do with lldb apparently)

[xedin]

@swift-ci please smoke test macOS platform

[theblixguy]

Hmm the macOS test failed because the any object error is being created twice for let _ = obj[keyPath: \.abc]. I can just fix the test for now if you want? Maybe we're creating the fix twice once in fixMemberRef and once in matchTypesBindTypeVars? Which I think makes sense because the reason we create a fix in fixMemberRef is to handle the obj[keyPath: kp] case, because it doesn't get diagnosed in matchTypesBindTypeVars. But the obj[keyPath: \.abc] case does and it seems like we add it as unviable in performMemberLookup as well, so it gets diagnosed twice.

[xedin]

I don’t understand, I thought that you mentioned in your previous comments that this case is not being diagnosed at all? It being diagnosed twice now goes contrary to that comment.

[theblixguy]

Sorry, the case that was not being diagnosed was obj[keyPath: kp] not obj[keyPath: \.abc]:

func doNotCrash_1(_ obj: AnyObject, _ kp: KeyPath<AnyObject, Int>) {
  let _ = obj[keyPath: \.abc] // error
  let _ = obj[keyPath: kp] // no error
}

Now it's being diagnosed, which is great, however the first example is being diagnosed twice now. Maybe we can skip creating a fix in matchTypesBindTypeVars if the anchor points to a subscript because we're already doing that in fixMemberRef.

[xedin]

Ok that makes more sense :) I think locators of the choice and root type supposed to be common besides ‘KeyPathRoot’ element, right? I think the way to resolve this situation would be to check if we already have a fix for choice before creating one for root via hasFixFor call with locator without last path element.

[theblixguy]

Yeah, that's right! Although we're creating a fix for the subscript member and one for the key path root:

let _ = obj[keyPath: \.abc]
        ^ here   &   ^ here

they have separate locators, so cannot use hasFixFor() here it seems. Maybe we should not create a fix for key path root if it's inside a subscript (or we have a fix for it)? Not fully sure how to best determine that! Perhaps we could get the locator for the subscript index, that should point on the key path and then we can check if there’s a fix attached it it

[xedin]

Yeah, that’s what I was asking - how do these locators look, are there common parts to them?

[theblixguy]

They're both different:

locator@0x7f84808bcf48 [Subscript@/Users/suyashsrijan/Desktop/test.swift:6:15 -> subscript member]
locator@0x7fbd83c600a0 [KeyPath@/Users/suyashsrijan/Desktop/test.swift:6:25 ->  keypath root]

It would be nice if we could dump each path element so I can see all the parts that make up the path (could write one if needed). I think the KeyPath path should have the subscript path element in it (?), let me check...

EDIT: Yes, seems like they do:

locator@0x7fa086ac4998 [Subscript@/Users/suyashsrijan/Desktop/test.swift:6:15 -> subscript member -> function argument]

That's what I get in matchTypesBindTypeVar when I dump the locator if the locator's path contains a SubscriptMember path element. Is there a better way to erase a path element in a locator?

[xedin]

Actually I think it's okay to diagnose it twice, once for each keypath for now, let's not over-complicate the changes further.

[xedin]

@swift-ci please test

[theblixguy]

Sure! I have updated the expected-error count to 2 for that test now, so all tests should pass🤞

[xedin]

@swift-ci please test

[xedin]

@swift-ci please test source compatibility

[xedin]

I'll get this squashed and cherry-pick to 5.1 as well.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 78fb5b4

[xedin]

@swift-ci please test macOS platform

[xedin]

@theblixguy Thank you again!