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

[Index][CodeCompletion] Fix #keyPath indexing when a type is referenced #20020

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@rockbruno
Contributor

rockbruno commented Oct 24, 2018

From SR-9020: Type references in #keyPath were being resolved but were not being added to the components list, which prevented the expression from being indexed when the expression included a type.

I think referencing a type in #keyPath is rather common as this keyword is normally created outside the scope of the watched property (like in iOS with AVFoundation types), so I created a new component type and assigned the resolved reference to it, which fixed the indexing issue.

It also seems like code completion was crashing when accessing inner properties, so I made a change to it in order to make expressions like #keyPath(myInt.hashValue) complete.

@rockbruno rockbruno changed the title from [Index][SR-9020] Fix #keyPath indexing when a type is referenced to [Index][CodeCompletion] Fix #keyPath indexing when a type is referenced Oct 24, 2018

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Oct 29, 2018

@@ -57,7 +57,7 @@ func testKeyPath(a: A, b: B) {
let _: String = #keyPath(A.propString)
// Property of String property (which looks on NSString)
let _: String = #keyPath(A.propString.URLsInText)
let _: String = #keyPath(A.propString.urlsInText)

This comment has been minimized.

@akyrtzi

akyrtzi Oct 29, 2018

Member

It's not clear to me, was this change necessary because of your other changes ? I don't see how.

This comment has been minimized.

@rockbruno

rockbruno Oct 29, 2018

Contributor

The test started to fail stating the name of the property changed, and I assumed it wasn't the case before because the expression wasn't being indexed, although it does seem weird. Was it not supposed to happen?

unexpected error produced: 'URLsInText' has been renamed to 'urlsInText'

This comment has been minimized.

@akyrtzi

akyrtzi Oct 29, 2018

Member

'Indexing' them should not have any effect in compiler errors, is the case that these were not even typechecked before ?

This comment has been minimized.

@akyrtzi

akyrtzi Oct 29, 2018

Member

Are you 100% sure your changes are triggering the failure ?

This comment has been minimized.

@rockbruno

rockbruno Oct 29, 2018

Contributor

You can actually reproduce this in Xcode 10 with old Swift 2 properties:

import Foundation
let a = #keyPath(Bundle.mainBundle)
print(a)

It will work and return mainBundle, despite the property being called main now. You can get it to throw the error if you get the property directly, which is the case where indexing was fine:

import Foundation
extension Bundle {
    func getPath() {
        let a = #keyPath(mainBundle)
        print(a)
    }
}

EDIT: After debugging the rename warning, I was able to confirm that they weren't being fully typechecked before as the changes now result in this specific logic getting called:

case KeyPathExpr::Component::Kind::Type: {

As the key paths that included types never stopped being UnresolvedProperties, this case was never being reached by them. Interesting that it used to work, I didn't know the old un-renamed properties could still be used like that!

This was the main culprit:

if (resolvedComponents.size() == expr->getComponents().size())

which got treated by:

// Resolve this component to the type we found.

This comment has been minimized.

@akyrtzi

akyrtzi Oct 29, 2018

Member

@DougGregor @rudkx please review, with these changes, properties from type references in #keypaths are properly typechecked now.

@akyrtzi akyrtzi requested a review from rintaro Oct 29, 2018

@akyrtzi

This comment has been minimized.

Member

akyrtzi commented Oct 29, 2018

@rintaro please review.

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Nov 12, 2018

Just a friendly review ping as this got buried on our feeds but no rush, hope you guys can find some time to see this soon!

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