-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Honor return type of __new__
#1020
Comments
This is follow-up to #982. |
Honestly I don't think that Have you seen any real code that violates this rule? |
No, I haven't seen real code that uses this, but I've seen tutorials that mention that this is possible -- they always mention that you probably shouldn't do that. Okay, what about this:
|
SGTM
|
@JukkaL On one hand, constructor returning an instance of different class is unacceptable in static typed languages. But on other hand, this feature is sometimes quite useful in Python. Even In general, it would be nice if mypy minimizes the number of false positives like flagging this code: class C:
def __new__(cls):
return 0
C() + 1 with |
One thought: the return type of class ZeroLiteral: # supertype: int
def __new__(cls) -> int:
return 0 This would also be a general alternative to python/typing#415 and could likely handle #3062 |
We make use of Ours works something like this: from typing import Union
class Base(object):
def __init__(self, name):
self.name = name
def __new__(cls, name):
# type: (str) -> Union[A, B]
if cls is Base:
if name == 'A':
return A(name)
else:
return B(name)
else:
# A or B
return object.__new__(cls)
class A(Base):
pass
class B(Base):
pass
b = Base('B')
reveal_type(b) The revealed type is Also, this seems related to #3307. |
Another use case that I had is to have async constructor, poor man's __ainit__: class WorkItem(metaclass=ABCMeta):
async def __new__(cls, *args, **kwargs):
""" Override __new__ to support our custom `async` constructors. """
o = super().__new__(cls)
await o.ainit(*args, **kwargs)
return o That actually fails in a different place, as in this case new returns the right thing, but class BuildItem(WorkItem): ...
toolsetItem = await BuildItem(...) fails with:
|
Another (less controversial) use case is at least to honor the return type of |
This basically follows the approach Jukka laid out in #1020 four years ago: * If the return type is Any, ignore that and keep the class type as the return type * Otherwise respect `__new__`'s return type * Produce an error if the return type is not a subtype of the class. The main motivation for me in implementing this is to support overloading `__new__` in order to select type variable arguments, which will be useful for subprocess.Popen. Fixes #1020.
This basically follows the approach Jukka laid out in #1020 four years ago: * If the return type is Any, ignore that and keep the class type as the return type * Otherwise respect `__new__`'s return type * Produce an error if the return type is not a subtype of the class. The main motivation for me in implementing this is to support overloading `__new__` in order to select type variable arguments, which will be useful for subprocess.Popen. Fixes #1020.
This basically follows the approach Jukka laid out in #1020 four years ago: * If the return type is Any, ignore that and keep the class type as the return type * Otherwise respect `__new__`'s return type * Produce an error if the return type is not a subtype of the class. The main motivation for me in implementing this is to support overloading `__new__` in order to select type variable arguments, which will be useful for subprocess.Popen. Fixes #1020.
This basically follows the approach Jukka laid out in #1020 four years ago: * If the return type is Any, ignore that and keep the class type as the return type * Otherwise respect `__new__`'s return type * Produce an error if the return type is not a subtype of the class. The main motivation for me in implementing this is to support overloading `__new__` in order to select type variable arguments, which will be useful for subprocess.Popen. Fixes #1020.
This basically follows the approach Jukka laid out in #1020 four years ago: * If the return type is Any, ignore that and keep the class type as the return type * Otherwise respect `__new__`'s return type * Produce an error if the return type is not a subtype of the class. The main motivation for me in implementing this is to support overloading `__new__` in order to select type variable arguments, which will be useful for subprocess.Popen. Fixes #1020.
Maybe #7477 would be another motivating example for this. After applying my I've tried the latest master which contains #7188 but
Defining a copy of |
@gvanrossum I have a use case for this: In my library MorphoCut, a computational graph is constructed of individual processing nodes. For the readability of the code, it would be really great to instantiate a node but getting something else instead of the instance object. Here is a coarse sketch: Expandfrom typing import Dict, Iterable, List, TYPE_CHECKING, Type
# When instantiated, nodes are put into this list
nodes: List["Node"] = []
class Variable:
"""A variable is a placeholder for a value in the stream."""
class Node:
"""A Node processes stream objects."""
# Error: Incompatible return type for "__new__" (returns "Variable", but must return a subtype of "Node")
def __new__(cls: Type["Node"], *args, **kwargs) -> Variable:
print(f"{cls.__name__}.__new__")
node: Node = object.__new__(cls)
# Initialize
node.__init__(*args, **kwargs)
return node.output
def __init__(self) -> None:
print(f"{self.__class__.__name__}.__init__")
self.output = Variable()
nodes.append(self)
def transform_stream(self, stream: Iterable):
for obj in stream:
obj[self.output] = self.transform(obj)
yield obj
def transform(self, obj):
raise NotImplementedError()
class Range(Node):
"""A Node that produces numbers in a range."""
def __init__(self, stop) -> None:
super().__init__()
self.stop = stop
def transform_stream(self, stream: Iterable):
for i in range(self.stop):
yield {self.output: i}
class Incr(Node):
"""A Node that increments the incoming value."""
def __init__(self, x: Variable) -> None:
super().__init__()
self.x = x
def transform(self, obj):
return obj[self.x] + 1
# Build the pipeline. (This is what a user of the library does. Therefore, it needs to be maximally easy to see what is going on.)
value = Range(10)
if TYPE_CHECKING:
reveal_type(value) # Expected: Variable. Actually: Source.
# Error: Argument 1 to "Incr" has incompatible type "Range"; expected "Variable"
value2 = Incr(value)
###
# Build the computational graph by stacking the individual Node.transform_stream
stream: List[Dict] = [{}]
for node in nodes:
stream = node.transform_stream(stream)
# Execute the computational graph
for obj in stream:
print(obj) (In reality, it's still a bit more difficult.) There are other ways of doing this, but all (that I came up with so far) have their downsides. (Class decorators are equally badly supported. Instantiating, then retrieving the |
SymPy. |
Nah, it could be a function returning a class instance. I trust there's a reason, but it's probably a little more complicated than that. :-)
Yeah, that's a good example actually. |
If you use a function returning a class instance, you have to replicate the parameters of the constructor (if you want a proper signature / proper typing). I find this very inconvenient. |
Maybe an example where it might be reasonable for test.pyi: from typing import (
Iterable,
NoReturn,
overload,
)
class A:
@overload
def __new__(cls, x: str | bytes) -> NoReturn: ... # overlapping, but works in practise as type checkers pick the first matching overload
@overload
def __new__(cls, x: Iterable) -> A: ...
reveal_type(A([]))
reveal_type(A("a")) # mypy: DataFrame, pyright: NoReturn The above would help to tightly type |
It depends on the purpose of Mypy:
I believe that the later would better be served by a linter than by a type-checker. |
I have been working adding type hints for this to sympy (sympy/sympy#25103) but hitting up against this issue. To be clear I think that SymPy's design here is not great and ideally expression heads would not be classes. Handling typing in a symbolic context is very difficult though and I'm still not sure what good ways to do this are (the Julia symbolics.jl folks are also struggling with the same thing). We are talking about it but changing this in sympy at this stage would be extremely difficult (there are hundreds of these classes spreading over ~100k lines of code and then more in downstream code as well). I just want to note that pyright handles this differently and behaves exactly in the way that I would expect: from __future__ import annotations
class A:
def __new__(cls) -> A:
return super().__new__(cls)
class B(A):
def __new__(cls) -> A:
return super().__new__(cls)
reveal_type(B()) $ mypy q.py
q.py:8: error: Incompatible return type for "__new__" (returns "A", but must return a subtype of "B") [misc]
q.py:11: note: Revealed type is "q.B"
Found 1 error in 1 file (checked 1 source file)
$ pyright q.py
...
./q.py:11:13 - information: Type of "B()" is "A"
0 errors, 0 warnings, 1 information I don't mind adding
|
I have a real-world case where When building an expression, I want to check for trivial values and abandon the expression if trivial values are found. This can be done in from __future__ import annotations
class Add:
l: IntLike
r: IntLike
def __new__(cls, l: IntLike, r: IntLike) -> IntLike: # type: ignore[misc]
return (
l if r == 0 else
r if l == 0 else
super().__new__(cls)
)
def __init__(self, l: IntLike, r: IntLike):
self.l = l
self.r = r
def __repr__(self) -> str:
return f'({self.l} + {self.r})'
def __add__(self, other: IntLike) -> IntLike:
return Add(self, other)
IntLike = int | Add So for example, This runs just fine, and is perfectly sensible AFAICT. So to me it seems like Mypy's warning here is an opinionated complaint about style rather than an error being flagged. Maybe there could be a flag for allowing / forbidding this? Note that the type of |
Fixes python#1020 (comment) Surprisingly popular comment on a closed issue. We still issue the warning, but we do trust the return type instead of overruling it. Maybe fixes python#16012
My library, cached_classproperty uses this behavior to specify that a cached_classproperty returns a constant from its Are we planning to do anything about this task? Yes, simplicity is important but people have given so many use cases here. Pyright supports this feature too. |
Those two don't go along together. You either use static typing and follow the rules or make your code dynamic. You can't do "whatever you want" in a statically typed world. When you change the well defined signature, it's already unsafe because by nature people will make assumptions that your code follows the common practices/guidelines/rules. Your code lies about what it does and you want mypy to hide it. Most of the examples can be replaced with a more appropriate - even if not the prettiest, but definitely less cursed - construct. |
Can you point to anything that defines what the "rules" are for The docs are clear that The question as correctly asked above is whether mypy is supposed to be a type checker or a linter. Personally I would rather keep linting out of type checking and I definitely don't want opinionated lint rules to be baked into type inference. Note that returning a different type here does not mean that the types are completely undefined. In the SymPy case there is a class It is not difficult to represent what class type:
def __call__(cls, *args, **kwargs):
obj = cls.__new__(cls, *args, **kwargs)
if isinstance(obj, cls):
obj.__init__(*args, **kwargs)
return obj Having looked a little through the mypy code that handles this I imagine that the mypy code could be both simpler and more robust if it could just represent In fact if I just use a class method class Expr:
def __new__(cls, *args) -> 'Expr':
return cls.new(*args)
@classmethod
def new(cls, *args) -> 'Expr':
return object.__new__(cls)
class sin(Expr):
@classmethod
def new(cls, arg) -> Expr:
if arg == 0:
return Integer(0)
else:
return object.__new__(cls)
class Integer(Expr):
@classmethod
def new(cls, arg) -> Expr:
return object.__new__(cls)
exprs = [sin(0), sin(1), Integer(0), Integer(1)]
assert all(isinstance(expr, Expr) for expr in exprs)
reveal_type(sin.new(0)) # Expr (correct)
reveal_type(sin(0)) # sin (incorrect) Here pyright understands the type of $ mypy t.py
t.py:25: note: Revealed type is "t.Expr"
t.py:26: note: Revealed type is "t.sin"
Success: no issues found in 1 source file
$ pyright t.py
./t.py:25:13 - information: Type of "sin.new(0)" is "Expr"
./t.py:26:13 - information: Type of "sin(0)" is "Expr"
0 errors, 0 warnings, 2 informations ``` The fact that mypy can understand The rejection of returning other types from Lines 1535 to 1543 in 5b1a231
The inference in mypy bakes in the assumption that Lines 1244 to 1312 in 5b1a231
The comment in the code
bears no relation to the actual semantics of
Note that |
I believe the returned object must be of the same type. It also doesn't seem to be a coroutine function. which implies it may return anything. Or it could imply that
In my mind if you want to create an instance of
I see it as a correct type check, not as a lint. Since that method is tied to that class, then it should return instances of the type. However, if the docs are misleading and
is true, then you are correct. I wonder why mypy had that special case in the first place, has |
Nothing has changed except mypy making arbitrary decisions. The The signature of class A:
def __new__(cls) -> B:
return B() However mypy has made two opinionated decisions about this that are not based on any previously documented rules or the actual behaviour of Python at runtime:
I don't mind mypy reporting an error at The fact is that |
A caller making incorrect assumptions about a return type is exactly the sort of thing that a typechecker ought to prevent, except that in this case the typechecker itself also incorrectly assumes the return type. Here's an example: from __future__ import annotations
from random import randint
from typing import TYPE_CHECKING
class A:
def __new__(cls) -> int | A: # type: ignore[misc]
if randint(0, 1):
return object.__new__(cls)
else:
return 5
def f(x: A) -> None:
assert isinstance(x, A)
a = A()
if TYPE_CHECKING:
reveal_type(a)
else:
print(a)
f(a) This code will fail half the time. Mypy ought to be able to figure out why, but because it disregards the clearly specified signature of As @oscarbenjamin points out, this is a full-blown inference bug. Maybe the issue should be reopened? |
Currently the return type of
A(...)
is implicitlyA
even ifA.__new__
return something else. If the return type isAny
, maybeA(...)
should have typeAny
, and if it's a subclass, we should also use that. Currently this is a little difficult to implement due to implementation limitations (we derive the type object identity from the return type of a type object). Maybe we should introduce a new attribute toCallable
for this purpose.The text was updated successfully, but these errors were encountered: