[Index][CodeCompletion] Fix #keyPath indexing when a type is referenced #20020
Conversation
rockbruno
changed the title
|
CC @akyrtzi |
| @@ -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) | |||
There was a problem hiding this comment.
It's not clear to me, was this change necessary because of your other changes ? I don't see how.
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
'Indexing' them should not have any effect in compiler errors, is the case that these were not even typechecked before ?
There was a problem hiding this comment.
Are you 100% sure your changes are triggering the failure ?
There was a problem hiding this comment.
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:
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: https://github.com/apple/swift/blob/df64e8a5dcd96e2b402be4445e0d8e913fedf71b/lib/Sema/TypeCheckExprObjC.cpp#L417
which got treated by:
https://github.com/apple/swift/blob/df64e8a5dcd96e2b402be4445e0d8e913fedf71b/lib/Sema/TypeCheckExprObjC.cpp#L400
There was a problem hiding this comment.
@DougGregor @rudkx please review, with these changes, properties from type references in #keypaths are properly typechecked now.
|
@rintaro please review. |
|
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! |
| case KeyPathExpr::Component::Kind::Property: | ||
| case KeyPathExpr::Component::Kind::Type: { | ||
| //For code completion purposes, we only care about the last expression. |
There was a problem hiding this comment.
It's not clear to me how this relates to code completion.
There was a problem hiding this comment.
I might have treated this in the wrong place, but the issue I was having was that this method seems to be called twice on code completion - first to resolve the properties and then again in order to return the property that's trying to be completed, as far as I could tell. I couldn't find another case where this happened, so I assumed that code completion was the only case where the resolved enum cases would trigger here. Is it the wrong place?
The main idea for this block is that if I had something like#keyPath(MyType.myProperty., I had to ignore the type and return myProperty for code completion to show the correct results.
| if (&component == &expr->getComponents().back()) { | ||
| return component.getComponentType(); | ||
| } else { | ||
| continue; |
There was a problem hiding this comment.
This is also changing the behavior for Kind::Property, is that intentional?
There was a problem hiding this comment.
That's because you could in theory be reaching the keypath property from other properties too, like in #keyPath(MyType.myProperty.hashValue).
|
@swift-ci Please smoke test |
|
It's been a year. Sorry @rockbruno, could you update this? I'll take a look and tag @xedin as well if you do. |
|
Sure, I'll rebase this in the next hours. |
rockbruno
force-pushed
the
objc-keypath-indexing-fix
branch
from
df64e8a to
e0a4f2f
Compare
|
I've rebased but there are two pieces of logic that don't exist anymore, so this might not be building. I'll be able to take a look soon. |
|
@nathawes please take a look. |
|
@swift-ci please smoke test |
rockbruno
force-pushed
the
objc-keypath-indexing-fix
branch
from
e0a4f2f to
a3204b9
Compare
|
Sorry for the delay, there were indeed new cases to be considered after rebasing. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
I missed that test case, sorry for that. Fixing it |
|
Unfortunately, these changes aren't working anymore 😢The properties aren't getting type-checked anymore post-rebase. I think I need to do this from scratch. If it ends up being the case, I'll open a new PR for this. Sorry! |
|
I'm going to close this out since it seems like the approach taken here has fizzled. I appreciate your interest in fixing this bug, it sounds like you were really close. |
From SR-9020: Type references in
#keyPathwere 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
#keyPathis 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.