Skip to content
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.

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

[red-knot] Compare expression inference - Instance (+ Rich Comparison, Membership Test) #13872

Open
cake-monotone opened this issue Oct 22, 2024 · 4 comments · May be fixed by #13903
Open

[red-knot] Compare expression inference - Instance (+ Rich Comparison, Membership Test) #13872

cake-monotone opened this issue Oct 22, 2024 · 4 comments · May be fixed by #13903
Labels
red-knot Multi-file analysis & type inference

Comments

@cake-monotone
Copy link
Contributor

cake-monotone commented Oct 22, 2024

The item mentioned in #13618, "Bug with defined method but other type that doesn't match the signature," might complicate the upcoming PR too much. For now, I’ll focus solely on the implementation of Rich Comparison.

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Oct 22, 2024
@cake-monotone cake-monotone changed the title [red-knot] Compare expression inference - Instance (Rich Comparison) [red-knot] Compare expression inference - Instance (+ Rich Comparison, Membership Test) Oct 22, 2024
@cake-monotone
Copy link
Contributor Author

cake-monotone commented Oct 22, 2024

I realized that this issue should also include the "membership test" docs

The "membership test" involves logic for checking __contains__, __iter__, and __getitem__.

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 22, 2024

The algorithm you'll want to use for this will be similar in many ways to the one we implemented for binary arithmetic here (but there are some important differences, including the one you already noted for membership testing):

(Type::Instance(left_class), Type::Instance(right_class), op) => {
if left_class != right_class && right_class.is_subclass_of(self.db, left_class) {
let reflected_dunder = op.reflected_dunder();
let rhs_reflected = right_class.class_member(self.db, reflected_dunder);
if !rhs_reflected.is_unbound()
&& rhs_reflected != left_class.class_member(self.db, reflected_dunder)
{
return rhs_reflected
.call(self.db, &[right_ty, left_ty])
.return_ty(self.db)
.or_else(|| {
left_class
.class_member(self.db, op.dunder())
.call(self.db, &[left_ty, right_ty])
.return_ty(self.db)
});
}
}
left_class
.class_member(self.db, op.dunder())
.call(self.db, &[left_ty, right_ty])
.return_ty(self.db)
.or_else(|| {
if left_class == right_class {
None
} else {
right_class
.class_member(self.db, op.reflected_dunder())
.call(self.db, &[right_ty, left_ty])
.return_ty(self.db)
}
})
}

See also:

@AlexWaygood
Copy link
Member

Very happy to give additional clarifications if you need any further pointers!

@cake-monotone
Copy link
Contributor Author

Thank you!!! Your input was very helpful during the implementation.

BTW, I’m a bit confused about something, and I’d like to discuss it.

The part I’m struggling with is: how type mismatches between a dunder function signature and argument types are handled.

From my understanding, when there’s a type mismatch, the dunder method should return NotImplemented, as shown in the following example:

class A:
    def __lt__(self, other: int) -> bool:
        if not isinstance(other, int):
            return NotImplemented
        else:
            return True

A() < 3  # No issues
A() < "qwr"  # runtime error: not supported operator

# also in builtins
42 < 3.14  # No issues
(42).__lt__(3.14)  # NotImplemented
(3.14).__gt__(42)  # bool

With this assumption, __eq__ and __ne__ should be handled separately.

According to the Python docs, when NotImplemented is returned from __eq__ or __ne__, it falls back to is and is not, respectively.

from __future__ import annotations
from typing import reveal_type

class A:
    def __eq__(self, other: int) -> A:
        ...

class B:
    def __eq__(self, other: int) -> B:
        ...

reveal_type(A() == B())  # revealed: bool? or Unknown?

I think it should reveal bool. However, I'm not sure if this understanding is correct.. What do you think about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants