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

Respecting return type of __new__ does not extend to derived classes #8330

Open
ikonst opened this issue Jan 25, 2020 · 17 comments · May be fixed by #14471 or #16020
Open

Respecting return type of __new__ does not extend to derived classes #8330

ikonst opened this issue Jan 25, 2020 · 17 comments · May be fixed by #14471 or #16020

Comments

@ikonst
Copy link
Contributor

ikonst commented Jan 25, 2020

Following #7188, we can now have:

from typing import overload
from typing_extensions import Literal


class Cat:
    @overload
    def __new__(cls, name: Literal['Tabby']) -> 'Tabby': ...
    @overload
    def __new__(cls, name: Literal['Kitty']) -> 'Kitty': ...

    def __new__(cls, name: str) -> 'Cat': ...

class Kitty(Cat):
    pass

class Tabby(Cat):
    pass


reveal_type(Cat('Kitty'))
reveal_type(Cat('Tabby'))

result in:

Revealed type is 'test.Kitty'
Revealed type is 'test.Tabby'

(Meant as a trivial example.)

However, this doesn't extend to derived classes:

reveal_type(Tabby('Kitty'))
reveal_type(Tabby('Tabby'))

would result in:

Revealed type is 'test.Tabby'
Revealed type is 'test.Tabby'

I'm using mypy 0.761.

@ilevkivskyi
Copy link
Member

This is actually intentional, because doing the opposite (as it was initially implemented) caused even more false positives.

I am not sure what to do here, cc @msullivan

@ikonst
Copy link
Contributor Author

ikonst commented Jan 26, 2020

Curious, point to discussion / false positives?

@sbdchd
Copy link
Contributor

sbdchd commented Jul 9, 2021

I think I'm hitting this issue when trying to type an overloaded __new__ in a base class

My attempt overloading __new__ to match up with the untyped source:

from __future__ import annotations
from typing import Any, Dict, List, Type, TypeVar, overload
from typing_extensions import Literal

T = TypeVar("T", bound="MiniBaseSerializer")


class MiniBaseSerializer:
    @overload
    def __new__(cls, *, many: Literal[True]) -> MiniListSerializer:
        ...

    @overload
    def __new__(cls: Type[T], *, many: Literal[False] = ...) -> T:
        ...

    def __new__(cls: Type[T], many: bool = ...) -> Any:
        ...


class MiniListSerializer(MiniBaseSerializer):
    @property
    def validated_data(self) -> List[Dict[str, object]]:
        ...


class MiniSerializer(MiniBaseSerializer):
    @property
    def validated_data(self) -> Dict[str, object]:
        ...


class ExampleSerializer(MiniSerializer):
    # define some fields
    ...


example_many = ExampleSerializer(many=True)
reveal_type(example_many)
# pyright: Type of "example_many" is "MiniListSerializer"
# mypy: Revealed type is 'test_foo.ExampleSerializer'


example_single = ExampleSerializer()
reveal_type(example_single)
# pyright: Type of "example_many" is "MiniListSerializer"
# mypy: Revealed type is 'test_foo.ExampleSerializer*'

It works in pyright, but mypy forgets about the __new__ in the base class

I then tried overloading __init__ and adding a generic, which sort of works, but would require any users to provide a generic type param and it is also kind of lying about what is happening since the actual source code is returning a different type by overloading __new__

from __future__ import annotations
from typing import Dict, Generic, List, TypeVar, overload
from typing_extensions import Literal


T = TypeVar("T")


class MiniBaseSerializer(Generic[T]):
    @overload
    def __init__(self: MiniBaseSerializer[List[Dict[str, object]]], many: Literal[True]) -> None:
        ...

    @overload
    def __init__(self: MiniBaseSerializer[Dict[str, object]], many: Literal[False] = ...) -> None:
        ...

    def __init__(self, many: bool = False) -> None:
        ...

    @property
    def validated_data(self: MiniBaseSerializer[T]) -> T:
        ...


class MiniSerializer(Generic[T], MiniBaseSerializer[T]):
    ...


class ExampleSerializer(MiniSerializer[object]):
    ...


example_many = MiniBaseSerializer(many=True)
reveal_type(example_many)
# pyright - Type of "example_many" is "MiniBaseSerializer[List[Dict[str, object]]]"
# mypy - Revealed type is 'test_bar.MiniBaseSerializer[builtins.list[builtins.dict[builtins.str, builtins.object]]]'


example_single = MiniBaseSerializer()
reveal_type(example_single)
# pyright - Type of "example_single" is "MiniBaseSerializer[Dict[str, object]]"
# mypy - Revealed type is 'test_bar.MiniBaseSerializer[builtins.dict[builtins.str, builtins.object]]'

In search the issues, I also found #9482 which seemed related

Edit: Additionally, in the linked PR, there is:

from typing import TypeVar, Type
# Check for __new__ using type vars
TX = TypeVar('TX', bound='X')
class X:
def __new__(lol: Type[TX], x: int) -> TX:
pass
class Y(X): pass
reveal_type(X(20)) # N: Revealed type is "__main__.X*"
reveal_type(Y(20)) # N: Revealed type is "__main__.Y*"

which has the subclass maintaining the __new__ defined on the superclass

@Viicos
Copy link
Contributor

Viicos commented Jan 12, 2023

As stated in typeddjango/djangorestframework-stubs#315, django-rest-framework is a good example that would benefit from the requested behavior.

I'm curious to hear about the false positives that this caused

@intgr
Copy link
Contributor

intgr commented Jan 12, 2023

Finally found the PR and issue that introduced this restriction:

Since then, subprocess.Popen in typeshed no longer defines __new__ which solves the concrete example in #7597 AFAICT.

Also with PEP 673 Self type freshly merged in #11871, it should be easier to declare __new__ methods that correctly apply to subclasses as def __new__(...) -> Self, removing another reason for this restriction.

Returning non-subclasses from __new__() is a hack for sure, but as explained in #8330 (comment) and #14426 there are real-world libraries like Django REST Framework which are using this hack. And this restriction prevents accurately type hinting this library.

@ikonst
Copy link
Contributor Author

ikonst commented Jan 17, 2023

Thanks for the thorough code archeology, @intgr!

I suppose we can try adjusting it once more and seeing what breaks in mypy-primer etc.

@Viicos
Copy link
Contributor

Viicos commented Jan 18, 2023

I've tried reverting this specific check (Viicos@016abc3), but according to the mypy_primer output, this seems to break a lot of things (my fix is probably wrong)

@intgr
Copy link
Contributor

intgr commented Jan 18, 2023

I've tried reverting this specific check

Maybe you need to replace it with logic that retains the old behavior if __new__ return type was Any (e.g. untyped).

Where can I see the mypy_primer results?

@Viicos
Copy link
Contributor

Viicos commented Jan 18, 2023

I ran mypy_primer locally, but I'll make the PR so that we can have the output

@DougLeonard
Copy link

Returning non-subclasses from \_\_new\_\_() is a hack for sure, but as explained in #8330 (comment) and #14426 there are real-world libraries like Django REST Framework which are using this hack. And this restriction prevents accurately type hinting this library.

Should it even matter if something is a hack or matter if it's been proven to be useful? Wouldn't that be a discussion for the language and/or typing standards (or possibly linter) more than for the implementation of the reference type checker? It seems to be true that arbitrary return values from __new__ are legal python, and intentionally so.

@Viicos
Copy link
Contributor

Viicos commented Feb 17, 2023

Should it even matter if something is a hack or matter if it's been proven to be useful? Wouldn't that be a discussion for the language and/or typing standards (or possibly linter) more than for the implementation of the reference type checker? It seems to be true that arbitrary return values from new are legal python, and intentionally so.

This is the same idea as stated here: #1020 (comment), and I do agree. I haven't got time to get back to the PR, but I'll hope it will be merged when ready in the future

@DougLeonard
Copy link

DougLeonard commented Feb 25, 2023

quoting intgr from PR thread #14471:

Please make sure that if the __new__ return is untyped (returns Any) then it uses the class type implicitly. Not sure if that is already covered, but I suspect not.

I'm sure I'm not as familiar with realities, but specs are desirable. From PEP484:

Any function without annotations should be treated as having the most general type possible, or ignored, by any type checker. "

How do you get from that to restricting an explicit user-provided __new__ function to a particular type by default?

Neither mypy nor pyright are perfect, but an example:

from typing import Any

class Wolf():
    def __new__(cls):
        return object.__new__(cls)

    def howl(self):
        print("howling")


class Sheep():
    def __new__(cls) -> Any:  
        return object.__new__(Wolf)

    def bleat(self):
        print("baaah")


bigbad = Wolf()
bigbad.howl()

shifty = Sheep()
shifty.howl()

Pyright accepts the above, with (case 1) or without(case 2) the ->Any , but mypy wrongly complains about shifty howling.
And really, either seems to be legal python in spec and in implementation. As I understand, intgr's proposal would reject them (edit:) and would require legal code to have explicit type annotations to pass mypy Adding a typed parameter to __new__ changes nothing in both cases.

The rest may be beyond the PR, but strongly relate:

shifty.bleat()

Case 3: Still, fine for Pyright unless you remove the Any (Case 4).
Then Pyright sees the object.new(Wolf) and knows shifty cannot bleat and complains. PEP484:

For a checked function, the default annotation for arguments and for the return type is Any.

Intgr also implies that -> Any and no annotation should be treated the same, so it seems Pyright misses the spec, at least in the case where there's a typed parameter guaranteeing that it's a "checked function."
What about type narrowing with no type hints at all. With no return type hints, Pyright type narrows (deduces) Sheep.__new__() to Wolf.
Is narrowing part of "checking"? Is it legal to do type narrowing inside an un-"checked" function? If it's possible to determine with certainty that a runtime type error will definitely occur, should a checker ever be implicitly required to ignore that to be standard compliant? The first quote says "most general type OR ignored." A general type can be narrowed by other information.

On the other hand, when a value has type Any (default), the type checker will allow all operations on it,

Sure but can the Any return type be narrowed before/when returning?

Another example:

class mouse():
    pass


jerry = mouse()
mouse.howl()

(Case 5) Pyright and mypy both complain this time. But mouse() is now guaranteed to be a mouse, and
and we have no wrapper around object.new('T') in which to ignore checking (or maybe narrowing).

How about something more challenging?

class MayBeSheep():
    def __new__(cls):
        a = input('^')
        if a == "wolf":
            return object.__new__(Wolf)
        else:
            return object.__new__(cls)

    def bleat(self):
        print("baaah")


supershifty = MayBeSheep()
supershifty.howl()
supershifty.bleat()

(Case 6) Supershifty certainly cannot both bleat and howl. No matter what, this a type error. Mypy as always just ignores our new and only complains about howl(). It would be happy if we only tried to bleat.
Pyright again behaves differently with no return type (case 6) than with -> Any. With no return hint, (edit: it seems to assume a union type) and complains about both bleating and howling, referencing the respective problematic type for each. With -> Any (Case 7) it relents and gives no error.
Should the first access be accepted but treated as narrowing, only creating an error on the conflicting one? Would it be different if the conditional used a random bool, or a flip flop instead of an input? Inputs can be constrained. How smart must the checker be and what assumptions can/should it make?

I find it hard to read the PEP's and have any expectations, especially correct ones, about what mypy may or may not do. Does the PEP need to be improved?

@Viicos
Copy link
Contributor

Viicos commented Mar 5, 2023

@DougLeonard you are raising a lot of interesting questions here, and regarding the return object.__new__(Wolf) from the Sheep class, I'm wondering if mypy can infer that the return type is Wolf, as pyright is currently doing. We can't really know right now, as Wolf is not a subclass of Sheep hence why mypy ignores it (and this is what the PR is trying to fix).

Regarding the last example, maybe the type checker could infer a union of both types? My knowledge is limited here, but I can say that narrowing is part of type checking, and I'm still seeing some PRs implementing new kinds of type narrowing.

@DougLeonard
Copy link

@Viicos, Right but pyright is inferring Wolf even when there are no type annotations at all. Should it? You said narrowing is part of checking. I'm not sure if it's right for me to call inferring/deducing Wolf as a kind of narrowing on the implicit -> Any, but the spec says we should not do type checking on un-annotated functions, and it seems they should just be assumed to return Any. Personally, I'd rather see that it is always at least allowable to apply deductions that are guaranteed to be true, and it seems always a desirable feature. If the spec presently says that such deductions/narrowing are actually illegal to do on un-annotated functions, then maybe the spec should be changed. But I'm not sure I'd say (from a naive reading) that the wording must mean that no checking means no deducing. Anyway, presently mypy is of course deducing anyway, but is deducing wrongly.

For the last example pyright seems to infer a union (when ->Any isn't explicit), as it does complain about each of the types respectively. Yes, I think that's pretty good.

@erictraut
Copy link

For details about areas where pyright intentionally differs from mypy, you may find this documentation interesting.

@DougLeonard
Copy link

Thanks. I've seen it. And of course pyright can do whatever pyright wants to do. I believe mypy is meant to follow spec, but then neither really do.

@ikonst
Copy link
Contributor Author

ikonst commented Mar 9, 2023

@erictraut, a very exhaustive list. Can recognize some mypy pet peeves right there. BTW,

S in D and S not in D (where S is a string literal and D is a final TypedDict)

should no longer be the case since #13838 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants