Skip to content

[CodeCompletion] Missing init completions for dotExpr #16868

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

Merged
merged 5 commits into from
Jun 8, 2018
Merged

[CodeCompletion] Missing init completions for dotExpr #16868

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
927598f
[CodeCompletion] Missing init completions for dotExpr
AnthonyLatsis May 28, 2018
7ef24b1
[SR-7789][Additions]
AnthonyLatsis May 30, 2018
45c9038
corrections and review amendments
AnthonyLatsis May 31, 2018
931dac8
add super#^.init^# test for static methods
AnthonyLatsis May 31, 2018
ff4d061
removed unmatched brace
AnthonyLatsis Jun 4, 2018
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
57 changes: 25 additions & 32 deletions lib/IDE/CodeCompletion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2606,10 +2606,18 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
assert(isa<ConstructorDecl>(CurrDeclContext) &&
"can call super.init only inside a constructor");
needInit = true;
} else if (addName.empty() && HaveDot &&
Reason == DeclVisibilityKind::MemberOfCurrentNominal) {
// This case is querying the init function as member
needInit = true;
} else if (addName.empty()) {
auto ReasonIsValid =
Reason == DeclVisibilityKind::MemberOfCurrentNominal ||
Reason == DeclVisibilityKind::MemberOfSuper ||
Reason == DeclVisibilityKind::MemberOfProtocolImplementedByCurrentNominal;
auto instTy = ExprType->castTo<AnyMetatypeType>()->getInstanceType();
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be using ExprType directly here, since we don't know its relation to this call to addConstructorCall - see addConstructorCallsFromType for example where we don't know what ExprType will be. It looks like the intention was that this code either uses BaseType (if it is specified) or ExprType - e.g. inside of getTypeOfMember. That's pretty hard to understand, and in practice it looks like BaseType won't always be a metatype even though it sets IsOnMetatype=true.

How do you feel about the following?

  • Make BaseType required when IsOnMetatype=true and pass in ExprType when we call this method.
  • Either accept both metatypes and non-metatypes for BaseType and handle that explicitly (e.g. if it's a metatype, get the instance type otherwise use the type directly), or require callers to always provide one or the other and assert that (e.g. always pass a metatype or always pass a non-metatype).

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest I don't like the name isOnMetatype at all. We don't call initializers on metatypes to begin with, right? I renamed it to isOnType, made some clean-up and changed the code not to have a need to use ExprType here. I decided not to touch anything else yet, since getTypeOfMember, which is the only place where baseType is used, is incorrectly used as well (see SR-7802). After fixing that, we might not need baseType anymore.


if (ReasonIsValid && (HaveDot ||
!(instTy->is<ArchetypeType>() || IsStaticMetatype))) {
// This case is querying the init function as member
needInit = true;
}
}

// If we won't be able to provide a result, bail out.
Expand Down Expand Up @@ -2638,7 +2646,6 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
addTypeAnnotation(Builder, MemberType);
return;
}
assert(ConstructorType);

if (!HaveLParen)
Builder.addLeftParen();
Expand Down Expand Up @@ -2943,38 +2950,24 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
return;
}

if (auto MT = ExprType->getRValueType()->getAs<AnyMetatypeType>()) {
if (HaveDot) {
Type Ty;
for (Ty = MT; Ty && Ty->is<AnyMetatypeType>();
Ty = Ty->getAs<AnyMetatypeType>()->getInstanceType());
assert(Ty && "Cannot find instance type.");

// Add init() as member of the metatype.
if (Reason == DeclVisibilityKind::MemberOfCurrentNominal) {
if (IsStaticMetatype || CD->isRequired() ||
!Ty->is<ClassType>())
addConstructorCall(CD, Reason, None, None);
}
return;
}
}

if (auto MT = ExprType->getAs<AnyMetatypeType>()) {
if (HaveDot)
return;
Type Ty = MT->getInstanceType();
assert(Ty && "Cannot find instance type.");

// If instance type is type alias, showing users that the constructed
// If instance type is type alias, show users that the constructed
// type is the typealias instead of the underlying type of the alias.
Optional<Type> Result = None;
if (auto AT = MT->getInstanceType()) {
if (!CD->getInterfaceType()->is<ErrorType>() &&
(isa<NameAliasType>(AT.getPointer()) &&
AT->getDesugaredType() ==
CD->getResultInterfaceType().getPointer()))
Result = AT;
if (!CD->getInterfaceType()->is<ErrorType>() &&
isa<NameAliasType>(Ty.getPointer()) &&
Ty->getDesugaredType() ==
CD->getResultInterfaceType().getPointer()) {
Result = Ty;
}
addConstructorCall(CD, Reason, None, Result);
if (IsStaticMetatype || CD->isRequired() || IsSelfRefExpr ||
Ty->is<ArchetypeType>() ||
!(Ty->is<ClassType>() || HaveLParen))
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition needs a comment explaining what's going on. For example, why are we checking IsSelfRefExpr or is<ArchetypeType>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. In fact I got it wrong and I might have avoided mistakes with a comment. I will add failing tests to make sure the correct expression doesn't break without consequences.

addConstructorCall(CD, Reason, None, Result);
return;
}
if (IsSuperRefExpr || IsSelfRefExpr) {
if (!isa<ConstructorDecl>(CurrDeclContext))
Expand Down
9 changes: 6 additions & 3 deletions test/IDE/complete_after_self.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ class ThisDerived1 : ThisBase1 {
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[InstanceMethod]/CurrNominal: .derivedFunc0({#self: ThisDerived1#})[#() -> Void#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[StaticVar]/CurrNominal: .derivedStaticVar[#Int#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[StaticMethod]/CurrNominal: .derivedStaticFunc0()[#Void#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Constructor]/CurrNominal: ()[#ThisDerived1#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Constructor]/CurrNominal: ({#a: Int#})[#ThisDerived1#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Constructor]/CurrNominal: .init()[#ThisDerived1#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Constructor]/CurrNominal: .init({#a: Int#})[#ThisDerived1#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[InstanceMethod]/CurrNominal: .test1({#self: ThisDerived1#})[#() -> Void#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[InstanceMethod]/CurrNominal: .test2({#self: ThisDerived1#})[#() -> Void#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[StaticMethod]/CurrNominal: .staticTest1()[#Void#]
Expand All @@ -226,7 +226,7 @@ class ThisDerived1 : ThisBase1 {
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Class]/CurrNominal: .DerivedExtNestedClass[#ThisDerived1.DerivedExtNestedClass#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Enum]/CurrNominal: .DerivedExtNestedEnum[#ThisDerived1.DerivedExtNestedEnum#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[TypeAlias]/CurrNominal: .DerivedExtNestedTypealias[#Int#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Constructor]/CurrNominal: ({#someExtensionArg: Int#})[#ThisDerived1#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[Constructor]/CurrNominal: .init({#someExtensionArg: Int#})[#ThisDerived1#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[InstanceMethod]/Super: .baseFunc0({#self: ThisBase1#})[#() -> Void#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[InstanceMethod]/Super: .baseFunc1({#self: ThisBase1#})[#(Int) -> Void#]
// FUNC_STATIC_SELF_NO_DOT_1-NEXT: Decl[StaticVar]/Super: .baseStaticVar[#Int#]
Expand All @@ -248,6 +248,8 @@ class ThisDerived1 : ThisBase1 {
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[InstanceMethod]/CurrNominal: derivedFunc0({#self: ThisDerived1#})[#() -> Void#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[StaticVar]/CurrNominal: derivedStaticVar[#Int#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[StaticMethod]/CurrNominal: derivedStaticFunc0()[#Void#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[Constructor]/CurrNominal: init()[#ThisDerived1#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[Constructor]/CurrNominal: init({#a: Int#})[#ThisDerived1#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[InstanceMethod]/CurrNominal: test1({#self: ThisDerived1#})[#() -> Void#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[InstanceMethod]/CurrNominal: test2({#self: ThisDerived1#})[#() -> Void#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[StaticMethod]/CurrNominal: staticTest1()[#Void#]
Expand All @@ -258,6 +260,7 @@ class ThisDerived1 : ThisBase1 {
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[Class]/CurrNominal: DerivedExtNestedClass[#ThisDerived1.DerivedExtNestedClass#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[Enum]/CurrNominal: DerivedExtNestedEnum[#ThisDerived1.DerivedExtNestedEnum#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[TypeAlias]/CurrNominal: DerivedExtNestedTypealias[#Int#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[Constructor]/CurrNominal: init({#someExtensionArg: Int#})[#ThisDerived1#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[InstanceMethod]/Super: baseFunc0({#self: ThisBase1#})[#() -> Void#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[InstanceMethod]/Super: baseFunc1({#self: ThisBase1#})[#(Int) -> Void#]
// FUNC_STATIC_SELF_DOT_1-NEXT: Decl[StaticVar]/Super: baseStaticVar[#Int#]
Expand Down
49 changes: 46 additions & 3 deletions test/IDE/complete_constructor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,22 @@

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=EXPLICIT_CONSTRUCTORS_SELECTOR_1 | %FileCheck %s -check-prefix=EXPLICIT_CONSTRUCTORS_SELECTOR_1

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=EXPLICIT_CONSTRUCTORS_VAL_META_1 | %FileCheck %s -check-prefix=EXPLICIT_CONSTRUCTORS_VAL_META_1

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=EXPLICIT_CONSTRUCTORS_VAL_META_2 | %FileCheck %s -check-prefix=EXPLICIT_CONSTRUCTORS_VAL_META_2

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=EXPLICIT_CONSTRUCTORS_BASE_DERIVED_1 | %FileCheck %s -check-prefix=EXPLICIT_CONSTRUCTORS_BASE_DERIVED_1

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=DEFAULT_INIT_FROM_PROT_NODOT | %FileCheck %s -check-prefix=DEFAULT_INIT_FROM_PROT

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=DEFAULT_INIT_FROM_PROT_DOT | %FileCheck %s -check-prefix=DEFAULT_INIT_FROM_PROT

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=DEFAULT_INIT_FROM_PROT_PAREN | %FileCheck %s -check-prefix=DEFAULT_INIT_FROM_PROT

// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE1 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE1
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE2 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE2
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE2_1 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE2
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE2_2 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE2
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE2_3 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE2
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE3 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE3
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE4 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE4
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=INIT_FROM_METATYPE5 | %FileCheck %s -check-prefix=INIT_FROM_METATYPE4
Expand Down Expand Up @@ -141,6 +153,17 @@ func testExplicitConstructorsSelector1() {
// EXPLICIT_CONSTRUCTORS_SELECTOR_1: End completions
}

func testExplicitConstructorsValueMetatype() {
var SS = ExplicitConstructorsSelector1.self
SS#^EXPLICIT_CONSTRUCTORS_VAL_META_1^#
SS(#^EXPLICIT_CONSTRUCTORS_VAL_META_2^#

// EXPLICIT_CONSTRUCTORS_VAL_META_1: Decl[Constructor]/CurrNominal: .init({#int: Int#})[#ExplicitConstructorsSelector1#]
// EXPLICIT_CONSTRUCTORS_VAL_META_1: Decl[Constructor]/CurrNominal: .init({#int: Int#}, {#andFloat: Float#})[#ExplicitConstructorsSelector1#]

// EXPLICIT_CONSTRUCTORS_VAL_META_2-NOT: Decl[Constructor]
}

struct ExplicitConstructorsSelector2 {
init(noArgs _ : ()) {}
init(_ a : Int) {}
Expand Down Expand Up @@ -198,10 +221,12 @@ func testGetInitFromMetatype1() {

func testGetInitFromMetatype2() {
var SS = ExplicitConstructorsBase1.self
SS.#^INIT_FROM_METATYPE2^#
SS.#^INIT_FROM_METATYPE2_1^#
SS#^INIT_FROM_METATYPE2_2^#
SS(#^INIT_FROM_METATYPE2_3^#
}

// INIT_FROM_METATYPE2-NOT: Decl[Constructor]/CurrNominal: init()[#ExplicitConstructorsBase1#]{{; name=.+$}}
// INIT_FROM_METATYPE2-NOT: Decl[Constructor]

func testGetInitFromMetatype3() {
var SS = ExplicitConstructorsBase1.self
Expand Down Expand Up @@ -245,6 +270,24 @@ func testHaveComma1() {
// HAVE_COMMA_1-NOT: Decl[Constructor]
}

//===--- Test that we show default constuctors inherited from protocols

protocol ProtDefaultInit {}
extension ProtDefaultInit { init(foo: Int) {}}

class ConformsToProtDefaultInit: ProtDefaultInit {
init(bar: Int) {}
}

func testHasDefaultInitFromProtocol() {
ConformsToProtDefaultInit#^DEFAULT_INIT_FROM_PROT_NODOT^#
ConformsToProtDefaultInit.#^DEFAULT_INIT_FROM_PROT_DOT^#
ConformsToProtDefaultInit(#^DEFAULT_INIT_FROM_PROT_PAREN^#

// DEFAULT_INIT_FROM_PROT-DAG: Decl[Constructor]/CurrNominal: {{.+}}{#bar: Int#}
// DEFAULT_INIT_FROM_PROT-DAG: Decl[Constructor]/Super: {{.+}}{#foo: Int#}
}

class WithAlias1 {
init(busted: B) {}
init(working: Int) {}
Expand Down
12 changes: 12 additions & 0 deletions test/IDE/complete_init_inherited.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TEST_B | %FileCheck %s -check-prefix=TEST_B
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TEST_C | %FileCheck %s -check-prefix=TEST_C
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TEST_D | %FileCheck %s -check-prefix=TEST_D
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TEST_D_DOT | %FileCheck %s -check-prefix=TEST_D_DOT
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TEST_D_PAREN | %FileCheck %s -check-prefix=TEST_D_PAREN

class A {
init(int i: Int) {}
Expand Down Expand Up @@ -65,6 +67,14 @@ class D : C {
// TEST_D-NEXT: Keyword[self]/CurrNominal: .self[#D.Type#]; name=self
// TEST_D-NEXT: End completions

// TEST_D_DOT: Decl[Constructor]/CurrNominal: init({#d: D#})[#D#]; name=init(d: D)
// TEST_D_DOT-NEXT: Decl[Constructor]/CurrNominal: init({#int: Int#})[#D#]; name=init(int: Int)
// TEST_D_DOT-NEXT: Decl[Constructor]/Super: init({#c: C#})[#C#]; name=init(c: C)

// TEST_D_PAREN: Decl[Constructor]/CurrNominal: ['(']{#d: D#}[')'][#D#]; name=d: D
// TEST_D_PAREN-NEXT: Decl[Constructor]/CurrNominal: ['(']{#int: Int#}[')'][#D#]; name=int: Int
// TEST_D_PAREN-NEXT: Decl[Constructor]/Super: ['(']{#c: C#}[')'][#C#]; name=c: C

func testA() {
A#^TEST_A^#
}
Expand All @@ -79,4 +89,6 @@ func testC() {

func testD() {
D#^TEST_D^#
D.#^TEST_D_DOT^#
D(#^TEST_D_PAREN^#
}