[CodeComplete] Compute type relations for global cached results

[ahoppen]

Computing the type relation for every item in the code completion cache is way to expensive (~4x slowdown for global completion that imports SwiftUI). Instead, compute a type’s supertypes (protocol conformances and superclasses) once and write their USRs to the cache. To compute a type relation we can then check if the contextual type is in the completion item’s supertypes.

This reduces the overhead of computing the type relations (again global completion that imports SwiftUI) to ~6% – measured by instructions executed.

Technically, we might miss some conversions like

• retroactive conformances inside another module (because we can’t cache them if that other module isn’t imported)
• complex generic conversions (just too complicated to model using USRs)

Because of this, we never report an unrelated type relation for global items but always default to unknown.

But I believe this change covers the most common cases and is a good tradeoff between accuracy and performance.

rdar://83846531

---

@rintaro I needed to undo the change you suggested in #40999 (comment):

When we were only storing AST-based types, we could have a boolean flag to also consider metatypes. This trick is harder to pull off with USR-based types, because we can’t just invoke the MetatypeType constructor with the instance type.

We could hack together a USR-based instance type to Metatype transformation by appending an "m" to the instance type’s USR but I decided to store the metatype as another possible result type of CodeCompletionResultType because:

1. Performing USR manipulations when computing type relations seems pretty fragile to me
2. We don’t have an option to store a metatype’s supertypes if we just append an "m" to the USR and thus can’t determine "Convertible" type relations for metatypes (to be fair we can’t do this today either but it should be possible by computing the metatype supertypes in USRBasedType::fromType
3. I think we can afford to save another word by making CodeCompletionResultType contain SmallVector<Type, 1> instead of Type.

[ahoppen]

@swift-ci Please smoke test macOS

[ahoppen]

@swift-ci Please smoke test

[ahoppen]

@swift-ci Please smoke test

[rintaro]

I'm still thinking how to avoid using vector for result type...

[rintaro]

Handle Opaque->getSuperclass() too?

class MyClass {}
protocol MyProto {}
func fn<T: MyClass & MyProto>() -> T {
  return #^HERE^#
}

[ahoppen]

Which result are you expecting here?

class MyClass {}
protocol MyProto {}
func fn<T: MyClass & MyProto>() -> T {
  return MyClass() // error: cannot convert return expression of type 'MyClass' to return type 'T'
}

When adding MyClass: MyProto.

class MyClass: MyProto {}
protocol MyProto {}
func fn<T: MyClass & MyProto>() -> T { // warning: redundant conformance constraint 'T' : 'MyProto'
  return MyClass() // error: cannot convert return expression of type 'MyClass' to return type 'T'
}

[rintaro]

Hm, OK, we can't know the actual expected type if the context type is a generic parameter. But that applies to this as well, right?

class MyClass {}
func fn<T: MyProto>() -> T {
  return #^HERE^#
}

So should we limit this to OpaqueTypeArchetypeType?

Anyway, I believe we still need to handle getSuperclass() too.

class C {}
class P {}
class D: C, P {}
class E: P {}
func fn() -> some C & P {
  return #^HERE^#
}

D is OK, but E is not.

[ahoppen]

Yeah, that makes sense. Implemented in 10d5d0d

[rintaro]

An example of archetype case is something like this?

func returnCollecton<T: MyProto>() -> T {}

If so, can we construct USRBasedType with empty USR and non-empty super types?

[ahoppen]

This actually works. We are reporting convertible type relations for types that conform to MyProto here. I added a test case for it. (dab4a2f)

[rintaro]

I mean when suggesting it. The test case would be something like:

//-- Lib.swift
public protocol MyProto {}
public func returnSomeMyProto() -> some MyProto { ... }

//-- test.swift
import Lib
func foo<T: MyProto>(_: T)
func test() {
  foo(#^HERE^#) // 'returnSomeMyProto()' should be prioritized.
} 

[ahoppen]

I see, I decided to special case some with a single protocol for now as representing the conjunction of two types isn’t currently supported in USRBasedType and it’s not too simple to put it in. Since the entire USRBasedType thing is just an under-approximation anyway, I think it’s fine if we can’t represent some MyProto & MyOtherProto for now. 6aff8ef

[ahoppen]

I'm still thinking how to avoid using vector for result type…

I actually think the vector is a rather nice solution.

• It solves the problem in a pretty elegant way that doesn’t require any USR-manipulations or anything of the sort
• It doesn’t consume too much additional memory (just one size_t for the SmallVector<_, 1> in the common case that there is only one result type)
• It’s extensible should we find other code completion items that can have multiple result type because there are multiple potential candidates where this might come in handy at some point. I can’t think of any concrete examples but I have a feeling that we might discover more completion results that have such a type duality.

[ahoppen]

@swift-ci Please smoke test

[rintaro]

I understand that using a "collection" might be a good solution.

However, again, CodeCompletionResultType resides in BumpPtrAllocator so we can't use SmallVector which requires the destructor call. ArrayRef might be OK, but how about using llvm::TrailingObject so we can pack the size() with IsNotApplicable flag, and save a pointer size of data(). USRBasedType could use TrailingObject too...

(I'm OK withArrayRef for now. TrailingObject can be followups )

[ahoppen]

Ah, now I finally understand what your concern about the collection was. As discussed offline, I decided to store the result types using two pointers since we don’t need a collection with more than 2 elements at the moment anyway 9ffc3a0

[rintaro]

Hm, OK, we can't know the actual expected type if the context type is a generic parameter. But that applies to this as well, right?

class MyClass {}
func fn<T: MyProto>() -> T {
  return #^HERE^#
}

So should we limit this to OpaqueTypeArchetypeType?

Anyway, I believe we still need to handle getSuperclass() too.

class C {}
class P {}
class D: C, P {}
class E: P {}
func fn() -> some C & P {
  return #^HERE^#
}

D is OK, but E is not.

[rintaro]

I mean when suggesting it. The test case would be something like:

//-- Lib.swift
public protocol MyProto {}
public func returnSomeMyProto() -> some MyProto { ... }

//-- test.swift
import Lib
func foo<T: MyProto>(_: T)
func test() {
  foo(#^HERE^#) // 'returnSomeMyProto()' should be prioritized.
} 

[rintaro]

protocol P1 {}
protocol P2: P1 {}

class B: P2 {}

protocol Q1 {}
protocol Q2: Q1 {}

class C: B, Q2 {}

In this case, USRBasedType::forType(C) would be like

p1 = {"P1"}
p2 = {"P2", &p1}
b  = {"B", &p1, &p2}
q1 = {"Q1"}
q2 = {"Q2", &q1}
c  = {"C", &p1, &p2, &q1, &q2, &b}

right? This looks like a waste of the space. Could you limit Supertypes only contains the "direct" super types? So the structure be like:

p1 = {"P1"}
p2 = {"P2", &p1}
b  = {"B", &p2}
q1 = {"Q1"}
q2 = {"Q2", &q1}
c  = {"C", &q2, &b}

When calculating the type relation, we should be able to collect all super type USRs by recursively look into the graph. Of course we need to be careful not to visit the same node twice, but it shouldn't be hard.

[ahoppen]

Good idea. It might slow down the computation of USRBasedTypes a little but since that should only happen once to build up the cache, I think it’s a good tradeoff daf8fb0

[ahoppen]

@swift-ci Please smoke test

[ahoppen]

@swift-ci Please smoke test

[ahoppen]

@swift-ci Please smoke test

[rintaro]

Added some comment, but nothing critical. We can do them in followups

[rintaro]

Does this support protocol with associated types? Adding a test case would be great.

[ahoppen]

It does, but while playing around with them I discovered another bug

public protocol ProtoWithAssocType {
  associatedtype MyAssoc
}


public func makeProtoWithAssocType() -> some ProtoWithAssocType { return StructWithAssocType() }

func protoWithAssocTypeInGenericContext<T: ProtoWithAssocType>() -> T {
   return #^COMPLETE^#
 }

would suggest makeProtoWithAssocType as “Identical” even though it is not valid here. The problem was that we were doing some of the special opaque return type handling for USRBasedTypes for all archetypes. Fix was to only apply them to OpaqueTypeArchetypeType. This regressed test cases which are using a generic context, but I think that’s fine since the main known limitation of the USR-based type comparisons is that they don’t support generic contexts. 5a1dd49

[rintaro]

When the hierarchy contains diamond conformance,

protocol Base {}
protocol P1: Base {}
protocol P2: Base {}
struct S: P1, P2 {}

I think Base is visited twice. I'm not sure this happens lot, and not sure it causes excessive waste. But this logic should have "visited" check.

[ahoppen]

Added it b12d82a

[rintaro]

Insert assert(ResultType1.is<Type>() == ResultType2.is<Type>()); or do we have a scenario mixing them?

[ahoppen]

We don’t have a scenario where we’re mixing them but we never make assumptions that both of them are of the same kind either. Adding the assertion just adds an artificial restriction that’s not necessary IMO.

[ahoppen]

@swift-ci Please smoke test

[ahoppen]

@swift-ci Please SourceKit stress test

[ahoppen]

@swift-ci Please smoke test

[ahoppen]

@swift-ci Please SourceKit stress test

[ahoppen]

@swift-ci Please SourceKit stress test

[ahoppen]

@swift-ci Please smoke test

[hamishknight]

https://ci.swift.org/job/oss-swift-pr-test-macoss/123 started failing after this change, reverting in #41627