Skip to content

[ty] Fix semantic token classification for properties accessed on instances #24065

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
MichaReiser merged 12 commits into from
Mar 31, 2026
+253 −28
Merged

[ty] Fix semantic token classification for properties accessed on instances #24065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
df43189
[ty] Fix semantic token classification for properties accessed on ins…
mvanhorn Mar 20, 2026
88b1985
[ty] Remove redundant is_property_instance guard, match PropertyInsta…
mvanhorn Mar 21, 2026
6f01ec3
[ty] Add readonly modifier for read-only properties, fix cargo fmt
mvanhorn Mar 27, 2026
469dc6f
[ty] Set readonly modifier for properties from definition path
mvanhorn Mar 27, 2026
cc4a189
[ty] Fix failing snapshot tests, add property_readonly_modifier test
Mar 30, 2026
0945862
Fix more tests
MichaReiser Mar 30, 2026
d704fbd
fix: prefer PropertyInstanceType with setter, add union fallback test
Mar 30, 2026
9a87642
fix: detect setter via AST decorator, handle union fallback
Mar 30, 2026
de7c1a4
fix: scan class AST for setter decorator instead of relying on type
Mar 30, 2026
ed3fdde
Formatting, snapshots
MichaReiser Mar 31, 2026
ea2b364
Simplify
MichaReiser Mar 31, 2026
355a235
Use `static_member`
MichaReiser Mar 31, 2026
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
258 changes: 232 additions & 26 deletions crates/ty_ide/src/semantic_tokens.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,10 @@ use std::ops::Deref;
use ty_python_semantic::semantic_index::definition::Definition;
use ty_python_semantic::types::TypeVarKind;
use ty_python_semantic::{
HasType, SemanticModel, semantic_index::definition::DefinitionKind, types::Type,
types::ide_support::definition_for_name,
HasType, SemanticModel,
semantic_index::definition::DefinitionKind,
types::Type,
types::ide_support::{definition_for_name, static_member_type_for_attribute},
};

/// Semantic token types supported by the language server.
Expand Down Expand Up @@ -467,6 +469,7 @@ impl<'db> SemanticTokenVisitor<'db> {
}
}

let db = self.model.db();
let attr_name_str = attr_name.id.as_str();
let mut modifiers = SemanticTokenModifier::empty();

Expand All @@ -475,12 +478,13 @@ impl<'db> SemanticTokenVisitor<'db> {
}

let elements = if let Some(union) = ty.as_union() {
union.elements(self.model.db())
union.elements(db)
} else {
std::slice::from_ref(&ty)
};

let mut token_type = UnifiedTokenType::None;
let mut all_properties_are_readonly = true;

for element in elements {
// Classify based on the inferred type of the attribute
Expand All @@ -500,8 +504,9 @@ impl<'db> SemanticTokenVisitor<'db> {
// Module accessed as an attribute (e.g., from os import path)
token_type.add(SemanticTokenType::Namespace);
}
ty if ty.is_property_instance() => {
Type::PropertyInstance(property) => {
token_type.add(SemanticTokenType::Property);
all_properties_are_readonly &= property.setter(db).is_none();
}
_ => {
token_type = UnifiedTokenType::Fallback;
Expand All @@ -510,6 +515,9 @@ impl<'db> SemanticTokenVisitor<'db> {
}

if let Some(uniform) = token_type.into_semantic_token_type() {
if uniform == SemanticTokenType::Property && all_properties_are_readonly {
modifiers |= SemanticTokenModifier::READONLY;
}
return (uniform, modifiers);
}

Expand Down Expand Up @@ -895,7 +903,8 @@ impl SourceOrderVisitor<'_> for SemanticTokenVisitor<'_> {
self.visit_expr(&attr.value);

// Then add token for the attribute name (e.g., 'path' in 'os.path')
let ty = expr.inferred_type(self.model).unwrap_or(Type::unknown());
let ty = static_member_type_for_attribute(self.model, attr)
.unwrap_or_else(|| expr.inferred_type(self.model).unwrap_or(Type::unknown()));
let (token_type, modifiers) = self.classify_from_type_for_attribute(ty, &attr.attr);
self.add_token(&attr.attr, token_type, modifiers);
}
Expand Down Expand Up @@ -1787,7 +1796,7 @@ b: list["int | str"] | None
c: "list[int | str] | None"
d: "list[int | str]" | "None"
e: 'list["int | str"] | "None"'
f: """'list["int | str"]' | 'None'"""
f: """'list["int | str"]' | 'None'"""
"#,
);

Expand Down Expand Up @@ -1899,7 +1908,7 @@ t = MyClass.prop # prop should be property on the class itself
"CONSTANT" @ 413..421: Variable [readonly]
"w" @ 483..484: Variable [definition]
"obj" @ 487..490: Variable
"prop" @ 491..495: Variable
"prop" @ 491..495: Property [readonly]
"v" @ 534..535: Variable [definition]
"MyClass" @ 538..545: Class
"method" @ 546..552: Method
Expand All @@ -1908,7 +1917,204 @@ t = MyClass.prop # prop should be property on the class itself
"__name__" @ 605..613: Variable
"t" @ 651..652: Variable [definition]
"MyClass" @ 655..662: Class
"prop" @ 663..667: Property
"prop" @ 663..667: Property [readonly]
"#);
}

Copy link
Copy Markdown
Member

@MichaReiser MichaReiser Mar 30, 2026

Choose a reason for hiding this comment

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

Let's add a test that showcases that an attribute access on a union is only marked as Property if the field is implemented as a property on all instances.

E.g, the following should be classified as a variable:

    #[test]
    fn property_union_with_non_property_falls_back() {
        let test = SemanticTokenTest::new(
            "
class WithProperty:
    @property
    def value(self) -> int:
        return 1

class WithAttribute:
    value = 2

def f(obj: WithProperty | WithAttribute):
    return obj.value
",
        );

        let tokens = test.highlight_file();

        assert_snapshot!(test.to_snapshot(&tokens), @r#"
        "WithProperty" @ 7..19: Class [definition]
        "property" @ 26..34: Decorator
        "value" @ 43..48: Method [definition]
        "self" @ 49..53: SelfParameter [definition]
        "int" @ 58..61: Class
        "1" @ 78..79: Number
        "WithAttribute" @ 87..100: Class [definition]
        "value" @ 106..111: Variable [definition]
        "2" @ 114..115: Number
        "f" @ 121..122: Function [definition]
        "obj" @ 123..126: Parameter [definition]
        "WithProperty" @ 128..140: Class
        "WithAttribute" @ 143..156: Class
        "obj" @ 170..173: Parameter
        "value" @ 174..179: Variable
        "#);
    }

We should also ensure that READONLY is only set when a property is readonly on all union elements:

#[test]
    fn property_union_readonly_only_if_all_variants_are_readonly() {
        let test = SemanticTokenTest::new(
            "
from random import random

class ReadOnly:
    @property
    def value(self) -> int:
        return 1

class ReadWrite:
    @property
    def value(self) -> int:
        return self._value

    @value.setter
    def value(self, new_value: int) -> None:
        self._value = new_value

obj = ReadOnly() if random() else ReadWrite()
x = obj.value
",
        );

        let tokens = test.highlight_file();

        assert_snapshot!(test.to_snapshot(&tokens), @r#"
        "random" @ 6..12: Namespace
        "random" @ 20..26: Method
        "ReadOnly" @ 34..42: Class [definition]
        "property" @ 49..57: Decorator
        "value" @ 66..71: Method [definition]
        "self" @ 72..76: SelfParameter [definition]
        "int" @ 81..84: Class
        "1" @ 101..102: Number
        "ReadWrite" @ 110..119: Class [definition]
        "property" @ 126..134: Decorator
        "value" @ 143..148: Method [definition]
        "self" @ 149..153: SelfParameter [definition]
        "int" @ 158..161: Class
        "self" @ 178..182: SelfParameter
        "_value" @ 183..189: Variable
        "value" @ 196..201: Method
        "setter" @ 202..208: Method
        "value" @ 217..222: Method [definition]
        "self" @ 223..227: SelfParameter [definition]
        "new_value" @ 229..238: Parameter [definition]
        "int" @ 240..243: Class
        "None" @ 248..252: BuiltinConstant
        "self" @ 262..266: SelfParameter
        "_value" @ 267..273: Variable
        "new_value" @ 276..285: Parameter
        "obj" @ 287..290: Variable [definition]
        "ReadOnly" @ 293..301: Class
        "random" @ 307..313: Variable
        "ReadWrite" @ 321..330: Class
        "x" @ 333..334: Variable [definition]
        "obj" @ 337..340: Variable
        "value" @ 341..346: Property
        "#);
    }

Copy link
Copy Markdown
Contributor Author

@mvanhorn mvanhorn Mar 30, 2026

Choose a reason for hiding this comment

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

Added property_union_with_non_property_falls_back test.

#[test]
fn property_with_return_annotation() {
let test = SemanticTokenTest::new(
"
class Foo:
@property
def prop(self) -> int:
return 4

foo = Foo()
w = foo.prop
x = Foo.prop
",
);

let tokens = test.highlight_file();

assert_snapshot!(test.to_snapshot(&tokens), @r#"
"Foo" @ 7..10: Class [definition]
"property" @ 17..25: Decorator
"prop" @ 34..38: Method [definition]
"self" @ 39..43: SelfParameter [definition]
"int" @ 48..51: Class
"4" @ 68..69: Number
"foo" @ 71..74: Variable [definition]
"Foo" @ 77..80: Class
"w" @ 83..84: Variable [definition]
"foo" @ 87..90: Variable
"prop" @ 91..95: Property [readonly]
"x" @ 96..97: Variable [definition]
"Foo" @ 100..103: Class
"prop" @ 104..108: Property [readonly]
"#);
}

#[test]
fn property_readonly_modifier() {
// Verify that the readonly modifier is set for getter-only properties
// and NOT set for properties that also have a setter.
let test = SemanticTokenTest::new(
"
class Config:
@property
def read_only(self) -> str:
return 'value'

@property
def read_write(self) -> int:
return self._x

@read_write.setter
def read_write(self, value: int) -> None:
self._x = value

cfg = Config()
a = cfg.read_only
b = cfg.read_write
",
);

let tokens = test.highlight_file();

assert_snapshot!(test.to_snapshot(&tokens), @r#"
"Config" @ 7..13: Class [definition]
"property" @ 20..28: Decorator
"read_only" @ 37..46: Method [definition]
"self" @ 47..51: SelfParameter [definition]
"str" @ 56..59: Class
"'value'" @ 76..83: String
"property" @ 90..98: Decorator
"read_write" @ 107..117: Method [definition]
"self" @ 118..122: SelfParameter [definition]
"int" @ 127..130: Class
"self" @ 147..151: SelfParameter
"_x" @ 152..154: Variable
"read_write" @ 161..171: Method
"setter" @ 172..178: Method
"read_write" @ 187..197: Method [definition]
"self" @ 198..202: SelfParameter [definition]
"value" @ 204..209: Parameter [definition]
"int" @ 211..214: Class
"None" @ 219..223: BuiltinConstant
"self" @ 233..237: SelfParameter
"_x" @ 238..240: Variable
"value" @ 243..248: Parameter
"cfg" @ 250..253: Variable [definition]
"Config" @ 256..262: Class
"a" @ 265..266: Variable [definition]
"cfg" @ 269..272: Variable
"read_only" @ 273..282: Property [readonly]
"b" @ 283..284: Variable [definition]
"cfg" @ 287..290: Variable
"read_write" @ 291..301: Property
"#);
}

#[test]
fn property_union_with_non_property_falls_back() {
let test = SemanticTokenTest::new(
"
class WithProperty:
@property
def value(self) -> int:
return 1

class WithAttribute:
value = 2

def f(obj: WithProperty | WithAttribute):
return obj.value
",
);

let tokens = test.highlight_file();

assert_snapshot!(test.to_snapshot(&tokens), @r#"
"WithProperty" @ 7..19: Class [definition]
"property" @ 26..34: Decorator
"value" @ 43..48: Method [definition]
"self" @ 49..53: SelfParameter [definition]
"int" @ 58..61: Class
"1" @ 78..79: Number
"WithAttribute" @ 87..100: Class [definition]
"value" @ 106..111: Variable [definition]
"2" @ 114..115: Number
"f" @ 121..122: Function [definition]
"obj" @ 123..126: Parameter [definition]
"WithProperty" @ 128..140: Class
"WithAttribute" @ 143..156: Class
"obj" @ 170..173: Parameter
"value" @ 174..179: Variable
"#);
}

#[test]
fn property_union_readonly_only_if_all_variants_are_readonly() {
let test = SemanticTokenTest::new(
"
from random import random

class ReadOnly:
@property
def value(self) -> int:
return 1

class ReadWrite:
@property
def value(self) -> int:
return self._value

@value.setter
def value(self, new_value: int) -> None:
self._value = new_value

obj = ReadOnly() if random() else ReadWrite()
x = obj.value
",
);

let tokens = test.highlight_file();

assert_snapshot!(test.to_snapshot(&tokens), @r#"
"random" @ 6..12: Namespace
"random" @ 20..26: Method
"ReadOnly" @ 34..42: Class [definition]
"property" @ 49..57: Decorator
"value" @ 66..71: Method [definition]
"self" @ 72..76: SelfParameter [definition]
"int" @ 81..84: Class
"1" @ 101..102: Number
"ReadWrite" @ 110..119: Class [definition]
"property" @ 126..134: Decorator
"value" @ 143..148: Method [definition]
"self" @ 149..153: SelfParameter [definition]
"int" @ 158..161: Class
"self" @ 178..182: SelfParameter
"_value" @ 183..189: Variable
"value" @ 196..201: Method
"setter" @ 202..208: Method
"value" @ 217..222: Method [definition]
"self" @ 223..227: SelfParameter [definition]
"new_value" @ 229..238: Parameter [definition]
"int" @ 240..243: Class
"None" @ 248..252: BuiltinConstant
"self" @ 262..266: SelfParameter
"_value" @ 267..273: Variable
"new_value" @ 276..285: Parameter
"obj" @ 287..290: Variable [definition]
"ReadOnly" @ 293..301: Class
"random" @ 307..313: Variable
"ReadWrite" @ 321..330: Class
"x" @ 333..334: Variable [definition]
"obj" @ 337..340: Variable
"value" @ 341..346: Property
"#);
}

Expand Down Expand Up @@ -2024,7 +2230,7 @@ x = foobar_cls.prop # prop should be property
"CONSTANT" @ 470..478: Variable [readonly]
"w" @ 561..562: Variable [definition]
"foobar" @ 565..571: Variable
"prop" @ 572..576: Variable
"prop" @ 572..576: Property [readonly]
"foobar_cls" @ 636..646: Variable [definition]
"Foo" @ 649..652: Class
"random" @ 656..662: Variable
Expand All @@ -2034,7 +2240,7 @@ x = foobar_cls.prop # prop should be property
"method" @ 689..695: Method
"x" @ 760..761: Variable [definition]
"foobar_cls" @ 764..774: Variable
"prop" @ 775..779: Property
"prop" @ 775..779: Property [readonly]
"#);
}

Expand Down Expand Up @@ -2112,10 +2318,10 @@ q = Baz.prop # prop should be property on the class as well
"CONSTANT" @ 502..510: Variable [readonly]
"r" @ 558..559: Variable [definition]
"baz" @ 562..565: Variable
"prop" @ 566..570: Variable
"prop" @ 566..570: Property [readonly]
"q" @ 604..605: Variable [definition]
"Baz" @ 608..611: Class
"prop" @ 612..616: Property
"prop" @ 612..616: Property [readonly]
"#);
}

Expand Down Expand Up @@ -2148,7 +2354,7 @@ class Baz:
prop: str = \"hello\"

baz = Baz()
s = baz.method
s = baz.method
t = baz.CONSTANT
r = baz.prop
q = Baz.prop
Expand Down Expand Up @@ -2189,15 +2395,15 @@ q = Baz.prop
"s" @ 392..393: Variable [definition]
"baz" @ 396..399: Variable
"method" @ 400..406: Variable
"t" @ 408..409: Variable [definition]
"baz" @ 412..415: Variable
"CONSTANT" @ 416..424: Variable [readonly]
"r" @ 425..426: Variable [definition]
"baz" @ 429..432: Variable
"prop" @ 433..437: Variable
"q" @ 438..439: Variable [definition]
"Baz" @ 442..445: Class
"prop" @ 446..450: Variable
"t" @ 407..408: Variable [definition]
"baz" @ 411..414: Variable
"CONSTANT" @ 415..423: Variable [readonly]
"r" @ 424..425: Variable [definition]
"baz" @ 428..431: Variable
"prop" @ 432..436: Variable
"q" @ 437..438: Variable [definition]
"Baz" @ 441..444: Class
"prop" @ 445..449: Variable
"#);
}

Expand Down Expand Up @@ -2383,7 +2589,7 @@ class MyClass:
def __init__(self): pass

"""unrelated string"""

x: str = "hello"
"#,
);
Expand Down Expand Up @@ -2414,7 +2620,7 @@ What a good module wooo
def my_func(): pass

"""unrelated string"""

x: str = "hello"
"#,
);
Expand Down Expand Up @@ -3050,10 +3256,10 @@ class BoundedContainer[T: int, U = str]:
"wrapper" @ 339..346: Function [definition]
"args" @ 348..352: Parameter [definition]
"P" @ 354..355: Variable
"args" @ 356..360: Variable
"args" @ 356..360: Property [readonly]
"kwargs" @ 364..370: Parameter [definition]
"P" @ 372..373: Variable
"kwargs" @ 374..380: Variable
"kwargs" @ 374..380: Property [readonly]
"str" @ 385..388: Class
"str" @ 405..408: Class
"func" @ 409..413: Parameter
Expand Down
Loading
Loading