[red-knot] Improve handling of inherited class attributes by AlexWaygood · Pull Request #16160 · astral-sh/ruff

AlexWaygood · 2025-02-14T12:47:56Z

Summary

This PR fixes a number of issues to do with inherited class attributes.

On main, if you run red-knot on this file, you get these results:

from typing_extensions import reveal_type, ClassVar, Any, Literal

def _(flag: bool):
    class Foo:
        x = 1

    class Bar(Foo):
        if flag:
            x = 2

    # error: Attribute `x` on type `Literal[Bar]` is possibly unbound
    reveal_type(Bar.x)  # revealed: `Unknown | Literal[2]`

def _2(flag: bool):
    class Foo2:
        if flag:
            x = 1

    class Bar2(Foo2):
        if flag:
            x = 2

     # error: Attribute `x` on type `Literal[Bar]` is possibly unbound
    reveal_type(Bar2.x)  # revealed: `Unknown | Literal[2]`

class Foo3(Any): ...
reveal_type(Foo3.__repr__)  # revealed: Any

class A:
    x: ClassVar[Literal[1]] = 1

class B(Any): ...
class C(B, A): ...

reveal_type(C.x)  # revealed: `Any`

There are the following issues here:

The first "possibly unbound" diagnostic is a false positive, and the type is incorrect. Although the attribute is possibly unbound on the subclass, it is definitely bound on the superclass. We should union the two types together and understand the attribute as definitely bound.
The second "possibly unbound" diagnostic is correct, but the reported type is incorrect. Since the attribute is possibly unbound on the subclass, we should continue iterating up through the class's MRO until we find a definitely bound attribute on a superclass, and we should union all types we find in the process.
The revealed type for Foo3.__repr__ is not incorrect, but it could be more precise. All classes -- even non-fully-static classes that have Any in their MRO -- inherit from object, and object has a __repr__ method. We therefore know that whatever the type of Foo3.__repr__ is, it must be a consistent subtype of the type of object.__repr__; Literal[object.__repr__] & Any is therefore a better type for Foo3.__repr__ than Any.
A similar principle applies for the final revealed type, for C.x. C is not a fully static type (it has Any in its MRO), but it inherits from a fully static type A that has an x attribute. We therefore know that whatever type the Any in its MRO materialises to, the type of C.x must be a consistent subtype of the type of A.x; Literal[1] & Any is therefore a better type here than simply Any.

This PR fixes these issues by reworking types::Class::class_member. The PR is best reviewed one commit at a time:

The first commit is pure refactoring: it adds some new APIs to Symbol that moves the Symbol closer to our other Outcome enums. This refactoring makes commit 2 much easier.
Commit 2 fixes the bugs described above.

Test Plan

New mdtests added, which all fail on main.

…ibutes on classes inheriting from dynamic types

AlexWaygood · 2025-02-14T13:05:25Z

crates/red_knot_python_semantic/src/symbol.rs

+    /// Transform the symbol into a [`LookupResult`],
+    /// a [`Result`] type in which the `Ok` variant represents a definitely bound symbol
+    /// and the `Err` variant represents a symbol that is either definitely or possibly unbound.
+    pub(crate) fn into_lookup_result(self) -> LookupResult<'db> {
+        match self {
+            Symbol::Type(ty, Boundness::Bound) => Ok(ty),
+            Symbol::Type(ty, Boundness::PossiblyUnbound) => Err(LookupError::PossiblyUnbound(ty)),
+            Symbol::Unbound => Err(LookupError::Unbound),
+        }
+    }


In my work attempting to do a wholesale refactor of our Outcome enums, I looked for a while at whether it might be possible to use Results for all of them. I'm no longer convinced that doing so would necessarily be a good idea, but it's still the case that a lot of the time we want to treat these Outcomes "like Results".

...and for more on that topic, see #16161 😃

Hehe... I think using Result's is good. The main awkward thing about them is composition AND that we can't add helper methods to them (unless we use an extension trait). But I think the "somewhat more verbose" is actually a good thing because it so far has been easy to do the wrong thing and hard to do the right thing. It should be the other way round ;)

MichaReiser

That overall makes sense to me and goes into the direction I'm heading in as well. I'd love to get a second review.

MichaReiser · 2025-02-14T14:51:32Z

crates/red_knot_python_semantic/src/types/infer.rs

+                    // TODO: if `rhs_reflected` is possibly unbound, we should union the two possible
+                    // CallOutcomes together


This should become "easy" with my refactor but requires fixing our float/int/bool parameter assignability first :(

MichaReiser · 2025-02-14T14:52:51Z

crates/red_knot_python_semantic/src/symbol.rs

+    /// Transform the symbol into a [`LookupResult`],
+    /// a [`Result`] type in which the `Ok` variant represents a definitely bound symbol
+    /// and the `Err` variant represents a symbol that is either definitely or possibly unbound.
+    pub(crate) fn into_lookup_result(self) -> LookupResult<'db> {
+        match self {
+            Symbol::Type(ty, Boundness::Bound) => Ok(ty),
+            Symbol::Type(ty, Boundness::PossiblyUnbound) => Err(LookupError::PossiblyUnbound(ty)),
+            Symbol::Unbound => Err(LookupError::Unbound),
+        }
+    }


Hehe... I think using Result's is good. The main awkward thing about them is composition AND that we can't add helper methods to them (unless we use an extension trait). But I think the "somewhat more verbose" is actually a good thing because it so far has been easy to do the wrong thing and hard to do the right thing. It should be the other way round ;)

carljm

I like this, thanks for implementing it!

It does feel a little bit like we have too many ways to represent the same thing now? As in, Symbol and LookupResult seem entirely isomorphic, so why do we have both? Maybe LookupResult should entirely replace Symbol?

crates/red_knot_python_semantic/src/types.rs

carljm · 2025-02-14T22:37:08Z

crates/red_knot_python_semantic/src/types.rs

+            (Symbol::Type(ty, _), Some(dynamic_type)) => Symbol::bound(
+                IntersectionBuilder::new(db)
+                    .add_positive(ty)
+                    .add_positive(dynamic_type)


Compared to the previous code, we are now hardcoding here the idea that all attributes of Any are also Any, and same for other dynamic types; we no longer actually call .member() on them, as the previous code did. I think this is probably fine, but maybe worth noting?

(It does raise some interesting questions, which I don't think should be answered in this PR, about the definition of member for dynamic types. With this PR, and the way we construct the MRO when inheriting Any as (A, Any, object) given class A(Any): pass, for attributes defined on object we will give e.g. Literal[object.__repr__] & Any as the type of A.__repr__. But really Any itself could be considered to implicitly inherit object (because any Python object must), so perhaps the type of x: Any; x.__repr__ should also be Literal[object.__repr__] & Any, not just Any?)

Good points!... but I think if I synthesise your two points here, the conclusion I come to is that it would actually be incorrect to calling Type::Any.member() here? If we were to call a method on Type::Any here, I think we'd want the equivalent of Type::Any::own_class_member() (to avoid traversing up Any's "MRO" and hitting object). But we obviously don't have an own_class_member() method for Any -- and there doesn't seem much point in adding one, since it would always just return Any!

(It does raise some interesting questions, which I don't think should be answered in this PR, about the definition of member for dynamic types. With this PR, and the way we construct the MRO when inheriting Any as (A, Any, object) given class A(Any): pass, for attributes defined on object we will give e.g. Literal[object.__repr__] & Any as the type of A.__repr__. But really Any itself could be considered to implicitly inherit object (because any Python object must), so perhaps the type of x: Any; x.__repr__ should also be Literal[object.__repr__] & Any, not just Any?)

Yeah, I fully agree with all this, and it's consistent with the observations made in e.g. https://github.com/astral-sh/ruff/issues/15381. I guess partially fixing these issues (as I do in this PR) leaves things in a slightly inconsistent state... but I think you're right that scope creep here probably isn't a great idea 😄

Co-authored-by: Carl Meyer <[email protected]>

AlexWaygood · 2025-02-15T17:54:14Z

It does feel a little bit like we have too many ways to represent the same thing now? As in, Symbol and LookupResult seem entirely isomorphic

yep, that's exactly correct!

so why do we have both? Maybe LookupResult should entirely replace Symbol?

It's a great question. In my work looking at ways to refactor our Outcome enums, I looked for quite a while at whether it would be possible (and, whether it would be better!) to use Results everywhere instead of custom Outcome enums (of which Symbol is one). main...alex/typeops isn't the most recent attempt of mine to do this, but it is the most fully developed version of the idea that I produced. (It still has compilation failures, but it demonstrates the concept.)

I still think it would be possible to use Results everywhere for these enums. I'm no longer sure that it would necessarily be an improvement overall to use Results everywhere, however. In some cases, like the cases highlighted in this PR, we want to group the possibly-unbound and definitely-unbound variants together -- the definitely bound case becomes Ok and the other two cases become Err. But there are also lots of places in which we want to group the definitely-bound variant with the possibly-unbound variant, e.g.

ruff/crates/red_knot_python_semantic/src/symbol.rs

Lines 99 to 105 in df45a9d

    
           #[must_use] 
        
           pub(crate) fn map_type(self, f: impl FnOnce(Type<'db>) -> Type<'db>) -> Symbol<'db> { 
        
               match self { 
        
                   Symbol::Type(ty, boundness) => Symbol::Type(f(ty), boundness), 
        
                   Symbol::Unbound => Symbol::Unbound, 
        
               } 
        
           }

There isn't a consistent way in which we want to subdivide the three variants; it depends quite a lot on context. So in some cases it makes more sense to think about it as a Result, and in other cases it just doesn't really help that much, I don't think.

It still might make sense overall to use Results everywhere. But it's a big change, and I was really struggling to pull off a wholesale refactor all at once. So for now I'm trying to propose incremental changes where they seem like clear improvements.

…h/ruff into alex/symbol-into-result

AlexWaygood added the ty Multi-file analysis & type inference label Feb 14, 2025

AlexWaygood requested review from MichaReiser, carljm and sharkdp as code owners February 14, 2025 12:47

Add Symbol::into_lookup_result() and Symbol::unwrap_with_diagnostic

3efb763

AlexWaygood force-pushed the alex/symbol-into-result branch 2 times, most recently from 6d715e2 to 3289df1 Compare February 14, 2025 13:00

Implement better logic for possibly unbound class attributes and attr…

77bd0ca

…ibutes on classes inheriting from dynamic types

AlexWaygood force-pushed the alex/symbol-into-result branch from 3289df1 to 77bd0ca Compare February 14, 2025 13:02

AlexWaygood commented Feb 14, 2025

View reviewed changes

MichaReiser approved these changes Feb 14, 2025

View reviewed changes

carljm approved these changes Feb 14, 2025

View reviewed changes

Update crates/red_knot_python_semantic/src/types.rs

4beae50

Co-authored-by: Carl Meyer <[email protected]>

AlexWaygood added 2 commits February 15, 2025 18:18

clarificatory comments based on review comments

1273527

Merge branch 'alex/symbol-into-result' of https://github.com/astral-s…

947bd95

…h/ruff into alex/symbol-into-result

AlexWaygood enabled auto-merge (squash) February 15, 2025 18:20

AlexWaygood merged commit 93aff36 into main Feb 15, 2025
20 checks passed

AlexWaygood deleted the alex/symbol-into-result branch February 15, 2025 18:22

dhruvmanila mentioned this pull request Feb 17, 2025

red-knot: move symbol lookups in symbol.rs #16152

Merged

mishamsk mentioned this pull request Feb 19, 2025

[red-knot] Add diagnostic for class-object access to pure instance variables #16036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[red-knot] Improve handling of inherited class attributes#16160

[red-knot] Improve handling of inherited class attributes#16160
AlexWaygood merged 5 commits intomainfrom
alex/symbol-into-result

AlexWaygood commented Feb 14, 2025 •

edited

Loading

Uh oh!

AlexWaygood Feb 14, 2025 •

edited

Loading

Uh oh!

AlexWaygood Feb 14, 2025

Uh oh!

MichaReiser Feb 14, 2025

Uh oh!

MichaReiser left a comment

Uh oh!

MichaReiser Feb 14, 2025

Uh oh!

MichaReiser Feb 14, 2025

Uh oh!

carljm left a comment

Uh oh!

Uh oh!

carljm Feb 14, 2025

Uh oh!

AlexWaygood Feb 15, 2025

Uh oh!

AlexWaygood commented Feb 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// TODO: if `rhs_reflected` is possibly unbound, we should union the two possible
		// CallOutcomes together

Comments

Conversation

AlexWaygood commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

AlexWaygood Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carljm Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Feb 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AlexWaygood commented Feb 14, 2025 •

edited

Loading

AlexWaygood Feb 14, 2025 •

edited

Loading