[CodeCompletion] Duplicate existential requirements when restated #16811

AnthonyLatsis · 2018-05-24T01:10:15Z

Also resolves duplicates in cases like

func foo<T: A & B>(...) -> T.#^PARAM^#

class Foo : A, B {
  #^MEMBER^#
}

Please look into how I tell apart restated requirements. If there is a better way (ideally wipe out any self occurences from a type, which isn't trivial as far as I see), I'll be glad to hear.

I would like to know if it's more efficient and better to do

if (decl.getKind() >= DeclKind::AbstractFuncDecl_First &&
    decl.getKind() <= DeclKind::AbstractFuncDecl_Last)

than isa<AbstractFunctionDecl>(decl).

P.S. There is something strange going on with the SemanticContextKind on the tests. Have a look, it can be Super or CurrNominal depending on whether the function argument is or isn't of an archetype type. This was already present, the tests simply checked for one of them among the duplicates.

slavapestov

What happens if one requirement is overloaded and the only difference between the overloads is the generic signatures? I think calling getResult() on the overload signature will produce the same type then. Eg,

func f<T : P>(_: T)
func f<T : Q>(_: T)

slavapestov · 2018-05-24T04:07:38Z

lib/AST/LookupVisibleDecls.cpp

+    if (foundFuncs[{VD->getFullName(), type}].empty()) {
+      parentConsumer.foundDecl(VD, Reason);
+    }
+    foundFuncs[{VD->getFullName(), type}].insert(VD);


The return value of insert() tells you if the entry was inserted or not so you can use that instead.

Why is the value of this map a set of types here, do you actually do anything with them?

No, that's the key type, it's just that it's a pair (I'll change it to a pair, right now it's a tuple). I need both a type and a declName to sort out functions and subscripts.

The return value of insert() tells you if the entry was inserted or not so you can use that instead.

Actually, don't need more that one element per key, != empty suffices, so I better change the container type to TinyVector or simply a pointer.

slavapestov · 2018-05-24T04:08:07Z

lib/AST/LookupVisibleDecls.cpp

+
+  void foundDecl(ValueDecl *VD, DeclVisibilityKind Reason) override {
+    assert(VD);
+    if (!VD->getDeclContext()->getAsProtocolOrProtocolExtensionContext()) {


I don’t understand why this is specific to protocol extensions

It isn't! Thanks for the remark, I'll change this to dyn_cast<ProtocolDecl>

slavapestov · 2018-05-24T04:08:36Z

lib/AST/LookupVisibleDecls.cpp

+      return;
+    }
+    auto type =
+      VD->getOverloadSignatureType()->getAs<AnyFunctionType>()->getResult();


getAs returns null if it’s the wrong type; use castTo() if you don’t expect it to fail

slavapestov · 2018-05-24T04:09:35Z

lib/AST/LookupVisibleDecls.cpp

-          getReasonForSuper(Reason), TypeResolver, GSB, Visited);
-
-    if (auto superclass = Archetype->getSuperclass())
+  if (ArchetypeType *AT = BaseTy->getAs<ArchetypeType>()) {


Is this a non-functional change? Can you split it off into a separate patch if it’s unrelated?

slavapestov · 2018-05-24T04:13:52Z

Using isa is always better than checking the kind directly. I’m not sure what you mean with regard to wiping out Self requirements - can you give an example?

AnthonyLatsis · 2018-05-24T10:22:20Z

What happens if one requirement is overloaded and the only difference between the overloads is the generic signatures? I think calling getResult() on the overload signature will produce the same type then.

I checked for that and surprisingly, it works. I will add a test case. This must be due to the information T carries still being in the signature.
However, I am very curious about type comparing. It is done by pointer equality, but doesn't that mean that two equal types that are separate instances won't be equal? Sometimes it's useful to strip off information from types and compare them, but it works even then through pointers. How come?

UDP After rechecking, it seems you're right after all. It works without getting the overload signature type, which I do get.

AnthonyLatsis · 2018-05-24T10:39:13Z

Using isa is always better than checking the kind directly. I’m not sure what you mean with regard to wiping out Self requirements - can you give an example?

Thank you for the advice. So it is safer, but less efficient.
The problem is to tell whether a ValueDecl is a restatement,

protocol P {
  func foo()
}
protocol P1: P {
  func foo() // like here
}

The type of foo in P is <Self where Self: P> (P)->()->(). In P1, it's <Self where Self: P1> (P1)->()->(). This is simple, I can get the result type here. But how about a generic function that has both Self and T? As you see, the only thing that hiders us is the presence of Self.

I’m not sure what you mean with regard to wiping out Self requirements - can you give an example?

That said, and considering subscripts have to be handled specially since they are not self-curried with the generic signature still floating around, ideally, to tell apart restatements we have to compare with Self removed from the type, be it an associated type or a function.

<Self where Self: P, T>(P) -> (arg: T) -> () becomes <T>(arg: T)->(). Using resultType, I do the same except for the inclusion of the left-over generic signature. I've tried to search for functionality and ended up with the option to manually transform the type, but since everything is fine I haven't gone deeper into it.

Additionally, since the type of an associated type is also dependent on Self (Self.E.Type), but I don't know how to strip the Self off it, I had to create a separate set for properties and associated types, which I compare simply by name. I don't think this is a bad idea though.

slavapestov · 2018-05-24T17:16:37Z

Types are compared for pointer equality because they are memoized. We never allocate the same type twice. However, type aliases and other sugar do create unique types, so you should always use isEqual() (or compare CanTypes) to strip off the sugar before.

I think the reason the example with overloads by generic signature worked is because it was comparing sugared types. Each generic parameter type gets a sugared type with its name as written in source, but when canonicalized, only the index in the signature matters, so I think it will stop working once you use canonical types instead.

Eg, we want these two to be the same decl:

typealias X = Int
func f() -> X
func f() -> Int

AnthonyLatsis · 2018-05-24T17:58:22Z

Thank you for the explanation. I do compute new signatures and create new types here for comparison reasons, but they still somehow manage to correctly compare if they are canonical.

so I think it will stop working once you use canonical types instead.

I faced an opposite problem when fixing the overload issue: two signatures, for example

<Self> (Self) -> () -> ()
<Self> (Self) -> () -> ()

compare as different if I do not canonicalize them and equal otherwise. Is this due to each Self being a different instance? If so, what changes after canonicalization that makes these two equal? The canonicalized version is <τ_0_0> (τ_0_0) -> () -> ()

AnthonyLatsis · 2018-05-24T18:15:32Z

I now remove the requirements from Self when dealing with subscripts or explicitly generic functions. Eg
<Self, T where Self: This, T: Other> (Self) -> (T) -> () becomes
<Self, T where T: Other> (Self) -> (T) -> ()
This fixes overload cases where the only difference is the generic signature. Added a test for that.

Also simplified the container types.

AnthonyLatsis · 2018-05-24T22:05:06Z

Oops, conflicts from #16534

slavapestov · 2018-05-25T00:04:47Z

lib/AST/LookupVisibleDecls.cpp

+    if (params.size() == 1 && !isa<SubscriptDecl>(VD)) {
+      type = AFT->getResult()->getCanonicalType();
+    } else {
+      auto newReqs = sig->getRequirements().drop_front();


Please don’t make assumptions about the order of requirements in the generic signature like this. There must be a better way to accomplish what you’re trying to do. Perhaps you want to drop any conformance requirement with Self on the left hand side instead of assuming it’s the first one. Also this kind of thing should be factored out into a new method; the code here is getting a bit too long.

Perhaps you want to drop any conformance requirement with Self on the left hand side instead of assuming it’s the first one.

OK, fair enough. Who knows how the order might change in the future.
But... ~30 lines is long? :)

It will be longer than 30 lines once you do the right thing here, and it's more about single responsibility per method than raw length.

I see, you mean the type-computing logic.

slavapestov · 2018-05-25T00:07:35Z

Yes, each protocol has its own sugared Self type, but they all canonicalize to the same thing.

slavapestov · 2018-05-25T20:05:27Z

lib/AST/LookupVisibleDecls.cpp

+    if (params.size() == 1 && !isa<SubscriptDecl>(VD)) {
+      return AFT->getResult()->getCanonicalType();
+    }
+    GenericTypeParamType *SelfParam;


You don't have to "search" for it. Instead use VD->getDeclContext()->getSelfInterfaceType()

Thank you, that is exactly what I was looking for...in the wrong place.

slavapestov · 2018-05-25T20:05:54Z

lib/AST/LookupVisibleDecls.cpp

+
+    return GenericFunctionType::get(newSig, AFT->getInput(),
+                                    AFT->getResult(), AFT->getExtInfo())
+    ->getCanonicalType(newSig);


Indentation needs to be fixed, and you can use the no-argument form of getCanonicalType() instead.

slavapestov · 2018-05-25T20:06:16Z

lib/AST/LookupVisibleDecls.cpp

+private:
+  CanType stripSelfRequirementsIfNeeded(ValueDecl *VD, AnyFunctionType *AFT) {
+    // Preserve the generic signature if this is a subscript, which are uncurried,
+    // or if we have genenic params other than Self. Otherwise, strip if off


typo: "genenic"

typo: "strip if off"

slavapestov · 2018-05-25T20:07:05Z

lib/AST/LookupVisibleDecls.cpp

+    // When preserving the generic signature, we remove the requirements
+    // from Self to make they don't prevent us from recognizing restatements.
+
+    // This can't be null since we are dealing with method or


In that case, the function should take a GenericFunctionType instead of an AnyFunctionType, then this comment becomes unnecessary

AnthonyLatsis · 2018-05-29T17:26:05Z

@slavapestov, have a couple of minutes to take a glance?

AnthonyLatsis · 2018-05-31T17:20:34Z

I have the similar SR-7758 waiting which is dependent on the new consumer here. Can I commit to this PR then to solve them together?

AnthonyLatsis · 2018-06-04T12:25:48Z

@slavapestov Any comments? Should I ask Ben or someone else to have a look as well?

AnthonyLatsis · 2018-06-05T12:19:17Z

cc @nkcsgexi @benlangmuir Please have a look when possible.

benlangmuir · 2018-06-05T14:38:23Z

test/IDE/complete_override_access_control_protocol.swift

 // TEST_INTERNAL_DE-DAG: Decl[InstanceMethod]/Super: func colliding() {|}{{; name=.+$}}
 // TEST_INTERNAL_DE-DAG: Decl[InstanceMethod]/Super: func collidingGeneric<T>(x: T) {|}{{; name=.+$}}

-// TEST_PUBLIC_DE: Begin completions, 4 items
-// TEST_PUBLIC_DE-DAG: Decl[InstanceMethod]/Super: public func colliding() {|}{{; name=.+$}}
-// TEST_PUBLIC_DE-DAG: Decl[InstanceMethod]/Super: public func collidingGeneric<T>(x: T) {|}{{; name=.+$}}


Why did we lose the public keyword here?

It was a bit strange but I thought it isn't a big deal since we didn't lose it on the other test. Now I see. Apparently because I am filtering without taking access levels into account. This means that, given

public protocol PublicProtocol {func foo} protocol InternalProtocol {func foo}

, completing in class Foo: PublicProtocol, InternalProtocol and class Foo: InternalProtocol, PublicProtocol will result in the removal of different foos from the look up.
Thanks for pointing it out, I'll fix this by prioritizing declarations with higher access level.

slavapestov · 2018-06-07T00:16:45Z

@swift-ci Please smoke test

AnthonyLatsis · 2018-06-07T04:19:07Z

I guess these are failures from the recently merged removal of self completions from type identifiers.

AnthonyLatsis · 2018-06-08T06:56:14Z

The failures were because some tests began having unstable result ordering and the NEXT checks fail from time to time. This is because while processing, I don't respect the order in which they would have been fed to the parent consumer. Is this something to be concerned about? Should I write additional logic to keep the original order? If just stability is important, the order can be made stable by using MapVector, but it will still be different from how it was. You can have a look at what changed to DAG in the diff..

slavapestov · 2018-06-08T21:08:52Z

@AnthonyLatsis Yes, order stability is important. Please use MapVector and similar structures to ensure it doesn't depend on hash values.

AnthonyLatsis · 2018-06-09T10:48:25Z

Order restored

slavapestov · 2018-06-11T18:02:40Z

@swift-ci Please smoke test

AnthonyLatsis · 2018-06-12T15:08:34Z

I got it, let's try again.

AnthonyLatsis · 2018-06-14T11:59:07Z

@slavapestov Can you smoke test please?

slavapestov · 2018-06-15T06:56:40Z

@swift-ci Please smoke test

[CodeCompletion] Duplicate existential requirements when restated

ede3ba2

slavapestov reviewed May 24, 2018

View reviewed changes

AnthonyLatsis force-pushed the code-compl-dup-restated-requirements branch from a42ea6e to 3db6673 Compare May 24, 2018 18:25

Review amendments & fixed gen sig overload

6c13eba

AnthonyLatsis force-pushed the code-compl-dup-restated-requirements branch from 3db6673 to 6c13eba Compare May 24, 2018 21:11

Merge branch 'master' into code-compl-dup-restated-requirements

f289e46

slavapestov reviewed May 25, 2018

View reviewed changes

AnthonyLatsis added 2 commits May 25, 2018 14:59

isolate computing logic

bfab10b

Merge resolved conflict

e0c0ad6

slavapestov reviewed May 25, 2018

View reviewed changes

AnthonyLatsis added 2 commits May 26, 2018 02:22

review changes

1d06176

remove TokenKinds.h dependency

65e8fff

benlangmuir reviewed Jun 5, 2018

View reviewed changes

slavapestov self-assigned this Jun 7, 2018

AnthonyLatsis added 2 commits June 7, 2018 08:03

Merge branch 'master' into code-compl-dup-restated-requirements

addc434

restore 'public'

3d3f125

restore completion results order stability

ef11b11

AnthonyLatsis force-pushed the code-compl-dup-restated-requirements branch from fe33730 to ef11b11 Compare June 9, 2018 10:54

remove self completion from new test on type identifier

f3fed2e

slavapestov merged commit 4051246 into swiftlang:master Jun 15, 2018

AnthonyLatsis deleted the code-compl-dup-restated-requirements branch February 17, 2022 21:04

AnthonyLatsis mentioned this pull request May 24, 2018

[SR-7035] Autocomplete showing duplicate elements for existentials with restated or composed equal requirements #49583

Closed

[CodeCompletion] Duplicate existential requirements when restated #16811

[CodeCompletion] Duplicate existential requirements when restated #16811

Uh oh!

Conversation

AnthonyLatsis commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented May 24, 2018

Uh oh!

AnthonyLatsis commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnthonyLatsis commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented May 24, 2018

Uh oh!

AnthonyLatsis commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnthonyLatsis commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnthonyLatsis commented May 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnthonyLatsis commented May 31, 2018

Uh oh!

AnthonyLatsis commented Jun 4, 2018

Uh oh!

AnthonyLatsis commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnthonyLatsis commented May 24, 2018 •

edited

Loading

AnthonyLatsis May 24, 2018 •

edited

Loading

AnthonyLatsis May 24, 2018 •

edited

Loading

AnthonyLatsis May 24, 2018 •

edited

Loading

AnthonyLatsis commented May 24, 2018 •

edited

Loading

AnthonyLatsis commented May 24, 2018 •

edited

Loading

AnthonyLatsis commented May 24, 2018 •

edited

Loading

AnthonyLatsis commented May 24, 2018 •

edited

Loading

AnthonyLatsis commented May 29, 2018 •

edited

Loading

AnthonyLatsis commented Jun 5, 2018 •

edited

Loading

AnthonyLatsis Jun 5, 2018 •

edited

Loading

AnthonyLatsis commented Jun 7, 2018 •

edited

Loading

AnthonyLatsis commented Jun 8, 2018 •

edited

Loading