[ty] Fix semantic token classification for properties accessed on instances by mvanhorn · Pull Request #24065 · astral-sh/ruff

mvanhorn · 2026-03-20T05:34:08Z

Summary

When a property is accessed on an instance (e.g., foo.prop), the descriptor protocol resolves the inferred type to the property's return type rather than PropertyInstance. This caused is_property_instance() to return false, so properties were classified as variable tokens instead of property tokens.

The fix checks the attribute's definition when the inferred type is not already a PropertyInstance. If the definition resolves to a function whose binding type is PropertyInstance (i.e., a @property-decorated function), the token is classified as Property.

Before this fix:

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

foo = Foo()
w = foo.prop  # classified as Variable

After:

w = foo.prop  # classified as Property

This also fixes property classification for ParamSpec.args and ParamSpec.kwargs, which are properties on ParamSpec.

Test plan

Added property_with_return_annotation test that verifies both instance-level and class-level property access with return type annotations
Updated existing snapshot assertions where properties on instances were previously (incorrectly) classified as variables
All 74 semantic token tests pass
cargo clippy and cargo fmt clean

…tances When a property is accessed on an instance (e.g., `foo.prop`), the descriptor protocol resolves the inferred type to the property's return type rather than `PropertyInstance`. This caused `is_property_instance()` to fail, classifying properties as variables. Fix this by checking the attribute's definition when the inferred type is not already a `PropertyInstance`. If the definition is a function whose binding type is `PropertyInstance`, classify the token as a property. Closes astral-sh/ty#3066

MichaReiser · 2026-03-20T08:22:27Z

crates/ty_ide/src/semantic_tokens.rs

                // 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 (token_type, modifiers) = self.classify_from_type_for_attribute(ty, &attr.attr);
+                let (token_type, modifiers) = if !ty.is_property_instance()


Can we remove the is_property_instance check here and in classify_from_type_for_attribute, or can you add a test that demonstrates when type-based inference is still needed?

Removed the guard in 88b1985. The type-based check in classify_from_type_for_attribute is still needed as a fallback for class-level property access (Cls.prop) where the inferred type IS PropertyInstance. For instance-level access (obj.prop), the descriptor protocol resolves to the getter's return type, so only is_property_from_definition catches those.

crates/ty_ide/src/semantic_tokens.rs

…nce variant directly Removes the `!ty.is_property_instance()` guard from the attribute classification in `visit_expr`. The `is_property_from_definition` check now runs unconditionally for all attribute accesses. Also changes the PropertyInstance case in `classify_from_type_for_attribute` from a guard pattern to a direct variant match for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Make PropertyInstanceType getter/setter fields pub per reviewer suggestion, and set the readonly semantic token modifier when a property has no setter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

MichaReiser · 2026-03-27T07:31:47Z

crates/ty_ide/src/semantic_tokens.rs

                let ty = expr.inferred_type(self.model).unwrap_or(Type::unknown());
-                let (token_type, modifiers) = self.classify_from_type_for_attribute(ty, &attr.attr);
+                let (token_type, modifiers) = if self.is_property_from_definition(attr) {
+                    (SemanticTokenType::Property, SemanticTokenModifier::empty())


We should also make sure to set the readonly modifier here

Fixed in 469dc6f. The is_property_from_definition path now returns the PropertyInstanceType so we can check setter. Also fixed a missing db binding in classify_from_type_for_attribute from the previous commit.

Add readonly modifier when a property's setter is None in the is_property_from_definition code path (line 925). Also fix missing db binding in classify_from_type_for_attribute.

astral-sh-bot · 2026-03-27T19:13:45Z

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 86.61%. The percentage of expected errors that received a diagnostic held steady at 81.56%. The number of fully passing files held steady at 70/132.

astral-sh-bot · 2026-03-27T19:14:34Z

Memory usage report

Memory usage unchanged ✅

astral-sh-bot · 2026-03-27T19:16:50Z

`ecosystem-analyzer` results

No diagnostic changes detected ✅

Full report with detailed diff (timing results)

MichaReiser

Thank you. Can you take a look at the tests? Some of them are now failing. We should also make sure that there's at least one test verifying that the READONLY modifier gets set correctly

Update 6 snapshot assertions where read-only properties now correctly get the [readonly] modifier. Add dedicated test verifying that getter-only properties get [readonly] while getter+setter properties do not. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

mvanhorn · 2026-03-30T13:14:42Z

Fixed in cc4a189. Updated all 6 failing snapshot assertions and added a property_readonly_modifier test that verifies [readonly] is set for getter-only properties and not set for getter+setter properties.

MichaReiser · 2026-03-30T14:33:47Z

crates/ty_ide/src/semantic_tokens.rs

+        "read_only" @ 273..282: Property [readonly]
+        "b" @ 283..284: Variable [definition]
+        "cfg" @ 287..290: Variable
+        "read_write" @ 291..301: Property [readonly]


This property should not be readonly

Fixed — the root cause was find_map returning the getter's PropertyInstance before the setter's. Now uses fold to prefer the setter-aware definition.

MichaReiser · 2026-03-30T14:38:22Z

crates/ty_ide/src/semantic_tokens.rs

+        "prop" @ 663..667: Property [readonly]
+        "#);
+    }
+


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 "#); }

Added property_union_with_non_property_falls_back test.

@MichaReiser

Address review feedback from @MichaReiser: - Fix property_from_definition to prefer the definition with a setter (the getter's PropertyInstanceType has setter=None even when a setter exists as a separate definition) - Fix read_write snapshot: no longer marked readonly - Add property_union_with_non_property_falls_back test verifying that union access falls back to Variable when not all members are properties

mvanhorn · 2026-03-30T17:10:37Z

Fixed in d704fbd. The issue was that property_from_definition used find_map, which returned the getter's PropertyInstanceType (setter=None) before reaching the setter's definition. Changed to fold to prefer the definition with a setter when one exists.

Also added the union fallback test. Note: I don't have cargo installed locally so the union test snapshot is my best estimate of the byte offsets — CI will catch if it needs adjustment.

Two issues with the previous approach: 1. Individual function definitions' PropertyInstanceType has setter=None even when a setter exists as a separate definition. Fix: check for @name.setter decorators in the AST instead of relying on the type. 2. Union types (WithProperty | WithAttribute) were classified as Property when only one member was a property. Fix: if any definition is a non-property (regular assignment or non-decorated function), fall back to Variable. Also fix union test snapshot byte offsets from CI output.

definitions_for_attribute only returns one binding (breaks after first hit), so it misses the setter definition. Instead, find the class containing the getter in the parsed AST and check sibling function definitions for @name.setter decorators. Also fix cargo fmt formatting.

MichaReiser · 2026-03-31T12:07:07Z

I simplified this a little but the following test is now failing again:

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

Specifically, cfg.read_write is marked as readonly. You tried to work around this by doing a separate AST traversal. I think this is problematic. Instead, we should fix definitions_for_attribute to return both the getter and setter definitions. This has the advantage that go to definition shows both the getter and setter, the same as Pylance.
It's probably best if we make this change in a separate PR and the

mvanhorn · 2026-03-31T14:41:10Z

Got it - the AST traversal was a workaround, not the right fix. I'll open a separate PR to update definitions_for_attribute to return both getter and setter definitions, which also fixes go-to-definition showing both.

* main: [ty] Add missing test case for inline functional TypedDict with an invalid type passed to the `name` parameter (#24334) [ty] Use `_cls` as argument name for `collections.namedtuple` (#24333) [ty] Emit diagnostic for functional TypedDict with non-literal name (#24331) Add `nested-string-quote-style` formatting option (#24312) `RUF010`: Mark fix as unsafe when it deletes a comment [ty] Fix semantic token classification for properties accessed on instances (#24065) publish installers to `/installers/ruff/latest` on the mirror (#24247)

Previously, `definitions_for_attribute_in_class_hierarchy` used `break 'scopes` after finding the first definition within a class scope. For properties with both a getter and setter, this returned only the getter definition, causing go-to-definition to show only the getter and preventing correct readonly classification when the setter exists in a different binding. Now the function collects all declarations/bindings from the same scope before breaking, so property getter+setter pairs are both returned. This matches Pylance's behavior as suggested in astral-sh#24065.

mvanhorn requested review from AlexWaygood, BurntSushi, Gankra, MichaReiser, carljm, dcreager and sharkdp as code owners March 20, 2026 05:34

astral-sh-bot bot added the ty Multi-file analysis & type inference label Mar 20, 2026

astral-sh-bot bot assigned BurntSushi Mar 20, 2026

mvanhorn mentioned this pull request Mar 20, 2026

Semantic tokens doesn't classify properties with return annotations as properties astral-sh/ty#3066

Closed

AlexWaygood added the server Related to the LSP server label Mar 20, 2026

MichaReiser reviewed Mar 20, 2026

View reviewed changes

crates/ty_ide/src/semantic_tokens.rs Outdated Show resolved Hide resolved

mvanhorn and others added 2 commits March 21, 2026 06:53

[ty] Add readonly modifier for read-only properties, fix cargo fmt

6f01ec3

Make PropertyInstanceType getter/setter fields pub per reviewer suggestion, and set the readonly semantic token modifier when a property has no setter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mvanhorn requested a review from ibraheemdev as a code owner March 27, 2026 05:41

MichaReiser reviewed Mar 27, 2026

View reviewed changes

[ty] Set readonly modifier for properties from definition path

469dc6f

Add readonly modifier when a property's setter is None in the is_property_from_definition code path (line 925). Also fix missing db binding in classify_from_type_for_attribute.

MichaReiser requested changes Mar 30, 2026

View reviewed changes

AlexWaygood removed their request for review March 30, 2026 13:25

This comment was marked as resolved.

Sign in to view

Fix more tests

0945862

MichaReiser reviewed Mar 30, 2026

View reviewed changes

Matt Van Horn and others added 4 commits March 30, 2026 10:26

Formatting, snapshots

ed3fdde

Simplify

ea2b364

Use static_member

355a235

MichaReiser self-requested a review March 31, 2026 13:02

MichaReiser approved these changes Mar 31, 2026

View reviewed changes

MichaReiser merged commit 0aa8626 into astral-sh:main Mar 31, 2026
51 checks passed

mvanhorn mentioned this pull request Apr 1, 2026

[ty] Return getter and setter definitions for properties #24349

Closed

Conversation

mvanhorn commented Mar 20, 2026

Summary

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astral-sh-bot bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No changes detected ✅

Uh oh!

astral-sh-bot bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Memory usage report

Uh oh!

astral-sh-bot bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ecosystem-analyzer results

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

mvanhorn commented Mar 30, 2026

Uh oh!

This comment was marked as resolved.

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!

mvanhorn commented Mar 30, 2026

Uh oh!

MichaReiser commented Mar 31, 2026

Uh oh!

mvanhorn commented Mar 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

astral-sh-bot bot commented Mar 27, 2026 •

edited

Loading

astral-sh-bot bot commented Mar 27, 2026 •

edited

Loading

astral-sh-bot bot commented Mar 27, 2026 •

edited

Loading

`ecosystem-analyzer` results