Let's leave this comment as is. I do think we want to classify this as a property and I find variable on an instance more difficult to understand. We can rephrase it to instance property if you prefer that.

The issue with foobar.prop is that ty isn't the type of the value (that is, the type of foobar) but the type of foobar.prop and foobar.pop inferes to Unknown (because the properties lack a return type annotation).

If we change the code snippet like so

from random import random

class Foo:
    @property
    def prop(self) -> int:
        return 4

class Bar:
    @property
    def prop(self) -> int:
        return 3


foobar: Foo | Bar = Foo() if random() else Bar()
w = foobar.prop                                  # prop should be variable on an instance

then ty is int as expected.

Which makes me question if the fix here is correct. I think what we want is to infer the type of foobar and if that's a union, classify variant.attr for each union variant.

Mhm yes, i agree with your analysis, but i don't think the semantic token type changes between Unknown and int, because the same match arm will be taken?

So i think the match arm to check for a property here does not do the correct thing for foobar.prop as you said, but i think that preexists this change and is also happening in the preexisting, other test where i changed the comment.

Regarding your second sentence; i think this is already done somewhat by the type inference anyway? Since at least for foobar.method the inferred type is a union of bound methods, which we can than classify.

Would you be fine with deferring the bugfix for property classification or would you rather have it fixed in this PR? (I feel like that fix could be more involved than the simple loop i added here until now 😅 )

I don't think the fix requires that many changes. It's probably something like:

let value_ty = value.inferred_type(model);

if let Some(union) = value_ty.as_union() {
	// classify each variant
	// if all classify the same, return early
}

// Old, unchanged logic:
let ty = expr.inferred_type(model);
...

I thought a bit more about this and i think this change alone would still not be perfect, consider this case:

from random import random

class Baz:
    if random():
        @property
        def prop(self) -> int:
            return 42
    else:
        @property
        def prop(self) -> str:
            return "hello"

baz = Baz()
x = baz.prop

where the value_ty would not be a union.
(There may actually be a real bug in type inference for the above example: astral-sh/ty#3032)

I think to fix the property classification you would need to look up the attribute name on the class of the object and check if it is defined with @property. Is that something that is already supported by some function you are aware of in the codebase?

I'll create a follow up issue for this

What's the reason that this is an immediately invoked lambda?

I thought i could combine it with the outer match somehow, but iiuc r-a transformed it to a "if let guard" which seems to only be stabilized very recently. I'll change it.

lmk if you find the functional style even less appealing 😅