Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion include/swift/AST/Expr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4944,6 +4944,7 @@ class KeyPathExpr : public Expr {
OptionalWrap,
Identity,
TupleElement,
Type,
};

private:
Expand Down Expand Up @@ -5052,7 +5053,17 @@ class KeyPathExpr : public Expr {
propertyType,
loc);
}


/// Create a component for a type.
static Component forType(ConcreteDeclRef typeRef,
Type type,
SourceLoc loc) {
return Component(nullptr, typeRef, nullptr, {}, {},
Kind::Type,
type,
loc);
}

/// Create a component for a subscript.
static Component forSubscript(ASTContext &ctx,
ConcreteDeclRef subscript,
Expand Down Expand Up @@ -5135,6 +5146,7 @@ class KeyPathExpr : public Expr {
case Kind::Property:
case Kind::Identity:
case Kind::TupleElement:
case Kind::Type:
return true;

case Kind::UnresolvedSubscript:
Expand All @@ -5159,6 +5171,7 @@ class KeyPathExpr : public Expr {
case Kind::Property:
case Kind::Identity:
case Kind::TupleElement:
case Kind::Type:
return nullptr;
}
llvm_unreachable("unhandled kind");
Expand All @@ -5178,6 +5191,7 @@ class KeyPathExpr : public Expr {
case Kind::Property:
case Kind::Identity:
case Kind::TupleElement:
case Kind::Type:
llvm_unreachable("no subscript labels for this kind");
}
llvm_unreachable("unhandled kind");
Expand All @@ -5200,6 +5214,7 @@ class KeyPathExpr : public Expr {
case Kind::Property:
case Kind::Identity:
case Kind::TupleElement:
case Kind::Type:
return {};
}
llvm_unreachable("unhandled kind");
Expand All @@ -5222,6 +5237,7 @@ class KeyPathExpr : public Expr {
case Kind::Property:
case Kind::Identity:
case Kind::TupleElement:
case Kind::Type:
llvm_unreachable("no unresolved name for this kind");
}
llvm_unreachable("unhandled kind");
Expand All @@ -5231,6 +5247,7 @@ class KeyPathExpr : public Expr {
switch (getKind()) {
case Kind::Property:
case Kind::Subscript:
case Kind::Type:
return Decl.ResolvedDecl;

case Kind::Invalid:
Expand Down Expand Up @@ -5260,6 +5277,7 @@ class KeyPathExpr : public Expr {
case Kind::Identity:
case Kind::Property:
case Kind::Subscript:
case Kind::Type:
llvm_unreachable("no field number for this kind");
}
llvm_unreachable("unhandled kind");
Expand Down
5 changes: 5 additions & 0 deletions lib/AST/ASTDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2734,6 +2734,7 @@ class PrintExpr : public ExprVisitor<PrintExpr> {
PrintWithColorRAII(OS, ASTNodeColor) << "unresolved_subscript";
printArgumentLabels(component.getSubscriptLabels());
break;

case KeyPathExpr::Component::Kind::Identity:
PrintWithColorRAII(OS, ASTNodeColor) << "identity";
break;
Expand All @@ -2743,6 +2744,10 @@ class PrintExpr : public ExprVisitor<PrintExpr> {
PrintWithColorRAII(OS, DiscriminatorColor)
<< "#" << component.getTupleIndex();
break;

case KeyPathExpr::Component::Kind::Type:
OS << "type_ref ";
break;
}
PrintWithColorRAII(OS, TypeColor)
<< " type='" << GetTypeOfKeyPathComponent(E, i) << "'";
Expand Down
1 change: 1 addition & 0 deletions lib/AST/ASTWalker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,6 +1071,7 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
case KeyPathExpr::Component::Kind::Invalid:
case KeyPathExpr::Component::Kind::Identity:
case KeyPathExpr::Component::Kind::TupleElement:
case KeyPathExpr::Component::Kind::Type:
// No subexpr to visit.
break;
}
Expand Down
1 change: 1 addition & 0 deletions lib/AST/Expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2141,6 +2141,7 @@ void KeyPathExpr::Component::setSubscriptIndexHashableConformances(
case Kind::Property:
case Kind::Identity:
case Kind::TupleElement:
case Kind::Type:
llvm_unreachable("no hashable conformances for this kind");
}
}
Expand Down
3 changes: 2 additions & 1 deletion lib/IDE/SourceEntityWalker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,8 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
for (auto &component : KPE->getComponents()) {
switch (component.getKind()) {
case KeyPathExpr::Component::Kind::Property:
case KeyPathExpr::Component::Kind::Subscript: {
case KeyPathExpr::Component::Kind::Subscript:
case KeyPathExpr::Component::Kind::Type: {
auto *decl = component.getDeclRef().getDecl();
auto loc = component.getLoc();
SourceRange range(loc, loc);
Expand Down
3 changes: 2 additions & 1 deletion lib/SILGen/SILGenExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3564,7 +3564,8 @@ RValue RValueEmitter::visitKeyPathExpr(KeyPathExpr *E, SGFContext C) {
for (auto &component : E->getComponents()) {
switch (auto kind = component.getKind()) {
case KeyPathExpr::Component::Kind::Property:
case KeyPathExpr::Component::Kind::Subscript: {
case KeyPathExpr::Component::Kind::Subscript:
case KeyPathExpr::Component::Kind::Type: {
auto decl = cast<AbstractStorageDecl>(component.getDeclRef().getDecl());

unsigned numOperands = operands.size();
Expand Down
4 changes: 3 additions & 1 deletion lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,8 @@ static bool buildObjCKeyPathString(KeyPathExpr *E,
}
case KeyPathExpr::Component::Kind::TupleElement:
case KeyPathExpr::Component::Kind::Subscript:
// Subscripts and tuples aren't generally represented in KVC.
case KeyPathExpr::Component::Kind::Type:
// Subscripts, tuples and types aren't generally represented in KVC.
// TODO: There are some subscript forms we could map to KVC, such as
// when indexing a Dictionary or NSDictionary by string, or when applying
// a mapping subscript operation to Array/Set or NSArray/NSSet.
Expand Down Expand Up @@ -4265,6 +4266,7 @@ namespace {
case KeyPathExpr::Component::Kind::Subscript:
case KeyPathExpr::Component::Kind::OptionalWrap:
case KeyPathExpr::Component::Kind::TupleElement:
case KeyPathExpr::Component::Kind::Type:
llvm_unreachable("already resolved");
}

Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/CSGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3049,7 +3049,8 @@ namespace {
case KeyPathExpr::Component::Kind::UnresolvedProperty:
// This should only appear in resolved ASTs, but we may need to
// re-type-check the constraints during failure diagnosis.
case KeyPathExpr::Component::Kind::Property: {
case KeyPathExpr::Component::Kind::Property:
case KeyPathExpr::Component::Kind::Type: {
auto memberTy = CS.createTypeVariable(resultLocator,
TVO_CanBindToLValue |
TVO_CanBindToNoEscape);
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6949,6 +6949,7 @@ ConstraintSystem::simplifyKeyPathConstraint(

case KeyPathExpr::Component::Kind::Property:
case KeyPathExpr::Component::Kind::Subscript:
case KeyPathExpr::Component::Kind::Type:
case KeyPathExpr::Component::Kind::UnresolvedProperty:
case KeyPathExpr::Component::Kind::UnresolvedSubscript: {
auto *componentLoc = getConstraintLocator(
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/ConstraintSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,8 @@ ConstraintSystem::getCalleeLocator(ConstraintLocator *locator,
anchor, {*componentElt, ConstraintLocator::SubscriptMember});
case ComponentKind::UnresolvedProperty:
case ComponentKind::Property:
// For a property, the choice is just given by the component.
case ComponentKind::Type:
// For properties and types, the choice is just given by the component.
return getConstraintLocator(anchor, *componentElt);
case ComponentKind::TupleElement:
llvm_unreachable("Not implemented by CSGen");
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/TypeCheckAvailability.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2472,7 +2472,8 @@ class AvailabilityWalker : public ASTWalker {
for (auto &component : KP->getComponents()) {
switch (component.getKind()) {
case KeyPathExpr::Component::Kind::Property:
case KeyPathExpr::Component::Kind::Subscript: {
case KeyPathExpr::Component::Kind::Subscript:
case KeyPathExpr::Component::Kind::Type: {
auto *decl = component.getDeclRef().getDecl();
auto loc = component.getLoc();
SourceRange range(loc, loc);
Expand Down
20 changes: 17 additions & 3 deletions lib/Sema/TypeCheckExprObjC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,16 @@ Optional<Type> TypeChecker::checkObjCKeyPathExpr(DeclContext *dc,
diag::expr_unsupported_objc_key_path_component,
(unsigned)kind);
continue;
case KeyPathExpr::Component::Kind::OptionalWrap:
case KeyPathExpr::Component::Kind::Property:
case KeyPathExpr::Component::Kind::Type: {
//For code completion purposes, we only care about the last expression.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how this relates to code completion.

Copy link
Contributor Author

@rockbruno rockbruno Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also changing the behavior for Kind::Property, is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because you could in theory be reaching the keypath property from other properties too, like in #keyPath(MyType.myProperty.hashValue).

}
}
case KeyPathExpr::Component::Kind::OptionalWrap:
case KeyPathExpr::Component::Kind::Subscript:
llvm_unreachable("already resolved!");
}
Expand Down Expand Up @@ -321,10 +329,11 @@ Optional<Type> TypeChecker::checkObjCKeyPathExpr(DeclContext *dc,
if (auto var = dyn_cast<VarDecl>(found)) {
// Resolve this component to the variable we found.
auto varRef = ConcreteDeclRef(var);
Type type = var->getInterfaceType();
auto resolved =
KeyPathExpr::Component::forProperty(varRef, Type(), componentNameLoc);
KeyPathExpr::Component::forProperty(varRef, type, componentNameLoc);
resolvedComponents.push_back(resolved);
updateState(/*isProperty=*/true, var->getInterfaceType());
updateState(/*isProperty=*/true, type);

// Check that the property is @objc.
if (!var->isObjC()) {
Expand Down Expand Up @@ -390,6 +399,11 @@ Optional<Type> TypeChecker::checkObjCKeyPathExpr(DeclContext *dc,
break;
}

// Resolve this component to the type we found.
auto typeRef = ConcreteDeclRef(type);
auto resolved =
KeyPathExpr::Component::forType(typeRef, newType, componentNameLoc);
resolvedComponents.push_back(resolved);
updateState(/*isProperty=*/false, newType);
continue;
}
Expand Down
7 changes: 7 additions & 0 deletions test/IDE/complete_pound_keypath.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

// RUN: %target-swift-ide-test(mock-sdk: %clang-importer-sdk) -code-completion -source-filename %s -code-completion-token=IN_KEYPATH_2 | %FileCheck -check-prefix=CHECK-IN_KEYPATH %s

// RUN: %target-swift-ide-test(mock-sdk: %clang-importer-sdk) -code-completion -source-filename %s -code-completion-token=IN_KEYPATH_3 | %FileCheck -check-prefix=CHECK-IN_KEYPATH_3 %s


// REQUIRES: objc_interop

Expand Down Expand Up @@ -34,6 +36,10 @@ func completeInKeyPath2() {
_ = #keyPath(ObjCClass.#^IN_KEYPATH_2^#
}

func completeInKeyPath3() {
_ = #keyPath(ObjCClass.prop1.#^IN_KEYPATH_3^#
}

// CHECK-AFTER_POUND-NOT: keyPath

// CHECK-KEYPATH_ARG: Keyword/None/TypeRelation[Identical]: #keyPath({#@objc property sequence#})[#String#]; name=#keyPath(@objc property sequence)
Expand All @@ -42,4 +48,5 @@ func completeInKeyPath2() {
// CHECK-IN_KEYPATH: Decl[InstanceVar]/CurrNominal: prop2[#ObjCClass?#]; name=prop2
// CHECK-IN_KEYPATH: Decl[InstanceVar]/Super: hashValue[#Int#]; name=hashValue

// CHECK-IN_KEYPATH_3: Decl[InstanceVar]/CurrNominal: hashValue[#Int#]; name=hashValue

26 changes: 18 additions & 8 deletions test/Index/index_keypaths.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,31 @@ struct MyStruct {
}
}

class MyClass {
class Inner {
@objc var myProp = 1
}
}

let a = \MyStruct.Inner.myProp
// CHECK: [[@LINE-1]]:25 | {{.*}} | myProp
// CHECK: [[@LINE-2]]:10 | {{.*}} | MyStruct
// CHECK: [[@LINE-3]]:19 | {{.*}} | Inner
let b: KeyPath<MyStruct.Inner, Int> = \.myProp
// CHECK: [[@LINE-1]]:41 | {{.*}} | myProp
let c = \MyClass.Inner.myProp

class MyClass {
class Inner {
@objc var myProp = 1
func method() {
let c: String = #keyPath(myProp)
// CHECK: [[@LINE-1]]:32 | {{.*}} | myProp
}
}
}

let d: String = #keyPath(MyClass.Inner.myProp)
// CHECK: [[@LINE-1]]:26 | {{.*}} | MyClass
// CHECK: [[@LINE-2]]:34 | {{.*}} | Inner
// CHECK: [[@LINE-3]]:40 | {{.*}} | myProp

let e = \MyClass.Inner.myProp
// CHECK: [[@LINE-1]]:24 | {{.*}} | myProp
// CHECK: [[@LINE-2]]:10 | {{.*}} | MyClass
// CHECK: [[@LINE-3]]:18 | {{.*}} | Inner
let d: KeyPath<MyClass.Inner, Int> = \.myProp
let f: KeyPath<MyClass.Inner, Int> = \.myProp
// CHECK: [[@LINE-1]]:40 | {{.*}} | myProp
14 changes: 7 additions & 7 deletions test/expr/primary/keypath/keypath-objc.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rockbruno rockbruno Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rockbruno rockbruno Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

https://github.com/apple/swift/blob/df64e8a5dcd96e2b402be4445e0d8e913fedf71b/lib/Sema/TypeCheckAvailability.cpp#L2437

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

Copy link
Contributor

@akyrtzi akyrtzi Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// String property with a suffix
let _: String = #keyPath(A.propString).description
Expand All @@ -72,36 +72,36 @@ func testKeyPath(a: A, b: B) {

// Array property (make sure we look at the array element).
let _: String = #keyPath(A.propArray)
let _: String = #keyPath(A.propArray.URLsInText)
let _: String = #keyPath(A.propArray.urlsInText)

// Dictionary property (make sure we look at the value type).
let _: String = #keyPath(A.propDict.anyKeyName)
let _: String = #keyPath(A.propDict.anyKeyName.propA)

// Set property (make sure we look at the set element).
let _: String = #keyPath(A.propSet)
let _: String = #keyPath(A.propSet.URLsInText)
let _: String = #keyPath(A.propSet.urlsInText)

// AnyObject property
let _: String = #keyPath(A.propAnyObject.URLsInText)
let _: String = #keyPath(A.propAnyObject.urlsInText)
let _: String = #keyPath(A.propAnyObject.propA)
let _: String = #keyPath(A.propAnyObject.propB)
let _: String = #keyPath(A.propAnyObject.description)

// NSString property
let _: String = #keyPath(A.propNSString.URLsInText)
let _: String = #keyPath(A.propNSString.urlsInText)

// NSArray property (AnyObject array element).
let _: String = #keyPath(A.propNSArray)
let _: String = #keyPath(A.propNSArray.URLsInText)
let _: String = #keyPath(A.propNSArray.urlsInText)

// NSDictionary property (AnyObject value type).
let _: String = #keyPath(A.propNSDict.anyKeyName)
let _: String = #keyPath(A.propNSDict.anyKeyName.propA)

// NSSet property (AnyObject set element).
let _: String = #keyPath(A.propNSSet)
let _: String = #keyPath(A.propNSSet.URLsInText)
let _: String = #keyPath(A.propNSSet.urlsInText)

// Property with keyword name.
let _: String = #keyPath(A.repeat)
Expand Down