No member completions offered for variable of type `T | Unknown`

[rodrigoscc]

Summary

Hello,

I've found a case where autocomplete won't work even tho the type of the variable is clear. It is when I iterate over a list, the variable attributes are not autocompleted inside the for loop:

i type is correctly detected as Unknown | A and I can even go to definition of its attributes, but autocomplete doesn't work. Here's the playground link: https://play.ty.dev/80851291-8503-4aaf-b5aa-f286949c820b

Thanks for this amazing LSP! :)

Version

0.0.6

[sinon]

The loop aspect doesn't seem to be the cause it's any union of types (not specifically to Unknown) seems to be stop the completions.

https://play.ty.dev/937e266f-1665-4b5d-a2c0-d7b4174c3a0d

import random
from ty_extensions import Unknown
class A:
    def __init__(self, msg: str):
        self.msg = msg

class B:
    def __init__(self, msg: str):
        self.message = msg


a = [A("hello"), A("no")]. # list[A | Unknown]
b: list[A] = [A("hello"), A("no")]
c: list[A | B] = [A("hello"), A("no"), B("yes")]

def something() -> A | B:
    return random.choice([A("a"), B("b")])

d = something()

# no completion
d.m

for i in a:
    # no completion
    i.m

for i in b:
    # works
    i.msg

for i in c:
    # no completion
    i.m

[AlexWaygood]

Thanks @rodrigoscc for the report and thanks @sinon for the initial triage!!

The problem here is this branch in our routine for collecting "all members" of a given type. On a union type T | S, the available members will be the intersection of <set of members available on T> with <set of members available on S>. That makes sense from a type-theory perspective, but it's obviously suboptimal for this specific case.

The problem is that a dynamic type such as Any or Unknown has an infinite number of members available on it, but it would take a "very long time"™️ for us to enumerate an infinite number of members, so instead of attempting to do that the all_members() routine just returns an empty set if you ask for all members available on a dynamic type. And the intersection of any set with an empty set is another empty set.

The long and short of it is that this routine is too naive currently when it comes to unions that include dynamic types. Shouldn't be too hard to fix.

[Wizzerinus]

Honestly I think this approach could be changed in general, not only for unions of dynamic types. Consider, for example, the type T | None (with T being some concrete type). While getting members from this type is not sound at runtime, it is still often useful to get autocompletion for its members, for example, i.e. to see what is possible to do with values of type T, without having to insert an assert obj is not None before that.

[AlexWaygood]

Honestly I think this approach could be changed in general, not only for unions of dynamic types. Consider, for example, the type T | None (with T being some concrete type). While getting members from this type is not sound at runtime, it is still often useful to get autocompletion for its members, for example, i.e. to see what is possible to do with values of type T, without having to insert an assert obj is not None before that.

Maybe. But wouldn't it be confusing for a user to have a situation where ty suggests an autocompletion, you accept that autocompletion suggestion, and then ty immediately complains about the code that it just added for you?

We should look at what other language servers like pylance do here.

[Wizzerinus]

Basedpyright gives all (or at least, multiple of) union fields in the completions:

So does pyrefly:

(EDIT: fixed Basedpyright and Pyrefly ss's being swapped)

[AlexWaygood]

Thanks @Wizzerinus, that's super helpful!

I would still be inclined to view that as a separate issue that should maybe be fixed in a separate PR. But it's extremely valuable to know how other LSPs handle union types here!

[AlexWaygood]

@zsol mentioned that this was one of his top annoyances with ty right now, and it was also mentioned as a point of frustration in https://github.com/astral-sh/ty/issues/2373, so we should treat this as high-priority

[carljm]

Though I think if we do #1240 (and make it default) that will reduce the priority on this quite a lot. (We should still do it, of course.)