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] Check the return type of dunder methods #16249

Open
MichaReiser opened this issue Feb 19, 2025 · 4 comments
Open

[red-knot] Check the return type of dunder methods #16249

MichaReiser opened this issue Feb 19, 2025 · 4 comments
Labels
red-knot Multi-file analysis & type inference

Comments

@MichaReiser
Copy link
Member

Description

Dunder methods like __bool__, __len__ (and binary/compare?) should return a value that is assignable to what's expected by their "convention". We currently don't check this.

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Feb 19, 2025
@MichaReiser MichaReiser added this to the Red Knot Q1 2025 milestone Feb 19, 2025
@AlexWaygood
Copy link
Member

I'm still not sure that this is actually something a type checker should check, rather than a type-aware linter. The type checker should check (where necessary/appropriate) that the return type is correct when these methods are (explicitly or implicitly) called. But if they're never called, there's no type safety issue. And we already have linter rules to guard against these antipatterns:

@MichaReiser
Copy link
Member Author

Yeah sorry. That's what I meant. We should check the return type when implicitly calling those operations and error if the dunder is incorrectly implemented.

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 19, 2025

Okay, great! For __len__, we have a TODO here for this that's relevant for this issue:

// TODO: Emit diagnostic for non-integers and negative integers

(and binary/compare?)

In general, these can return arbitrary objects and the call still succeeds, so there's nothing more for us to do on these, I don't think:

>>> class Foo:
...     def __lt__(self, other):
...         return Foo()
...         
>>> Foo() < Foo()
<__main__.Foo object at 0x1031cca50>

You could argue it's a bit of an antipattern to return a non-bool from these methods, but again, it's sort of a linter issue than something for a type checker to flag

@carljm
Copy link
Contributor

carljm commented Feb 19, 2025

Yeah I think this would apply to at least __len__, __bool__, __str__, __bytes__ and __repr__. Not sure if there are others I'm forgetting, haven't cross-checked with a comprehensive list of dunders.

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

No branches or pull requests

3 participants