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

Honor return type of __new__ even if not a subclass #15182

Open
oscarbenjamin opened this issue May 4, 2023 · 14 comments
Open

Honor return type of __new__ even if not a subclass #15182

oscarbenjamin opened this issue May 4, 2023 · 14 comments
Labels
bug mypy got something wrong

Comments

@oscarbenjamin
Copy link

This follows gh-1020 which was closed by gh-7188. This arises because I am adding type hints to sympy (sympy/sympy#25103).

In the following code we have a subclass whose __new__ method is only guaranteed to return objects that are types of the superclass:

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())

With mypy this is rejected and the type of B() is inferred incorrectly but pyright accepts this and gets the inference for B() correct:

$ 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 
Completed in 0.955sec

The fix for __new__ in gh-7188 was:

  • 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 issue here is about mixing the last two points. Here mypy produces an error but then does not respect __new__'s return type. The return type is not respected since mypy infers that it is of type B unlike pyright which infers type A as specified by the hint. I don't mind mypy reporting an error here but in context -> A is the accurate type hint and mypy should respect that because anything else is just not correct. I can add a type ignore to silence the mypy error but the inference will still be wrong in all downstream/user code and is also inconsistent with pyright.

@oscarbenjamin oscarbenjamin added the bug mypy got something wrong label May 4, 2023
@oscarbenjamin
Copy link
Author

This is handled here where there is an explicit check to ignore the type given in __new__ if it is not a subtype of the class and also in a bunch of other cases:

mypy/mypy/typeops.py

Lines 184 to 196 in 13f35ad

if (
isinstance(explicit_type, (Instance, TupleType, UninhabitedType))
# We have to skip protocols, because it can be a subtype of a return type
# by accident. Like `Hashable` is a subtype of `object`. See #11799
and isinstance(default_ret_type, Instance)
and not default_ret_type.type.is_protocol
# Only use the declared return type from __new__ or declared self in __init__
# if it is actually returning a subtype of what we would return otherwise.
and is_subtype(explicit_type, default_ret_type, ignore_type_params=True)
):
ret_type: Type = explicit_type
else:
ret_type = default_ret_type

I'm not sure where all of those conditions come from but I don't see how any of them can override the hint given in __new__ because __new__ can return anything (this is generally the reason for using __new__ rather than __init__).

@Viicos
Copy link
Contributor

Viicos commented May 4, 2023

Hi @oscarbenjamin, this seems to be a duplicate of #8330. I've opened #14471 since then but I'm kind of stuck on it. Hopefully if someone with the required knowledge could see what's wrong in the PR we could get it merged one day!

@oscarbenjamin
Copy link
Author

this seems to be a duplicate of #8330

Yes, I saw that issue. I thought this was different but now that I've seen the code that causes this I can see that it causes both issues.

To be honest having looked at the code I don't really understand what any of it is doing. It doesn't bear any resemblance to Python's runtime semantics for type.__call__, __new__ and __init__. For example the code here chooses between taking a hint from __new__ or __init__ based on which comes earlier in the MRO:

mypy/mypy/checkmember.py

Lines 1198 to 1199 in 13f35ad

# We take the type from whichever of __init__ and __new__ is first
# in the MRO, preferring __init__ if there is a tie.

That makes no sense to me:

  • __new__ and __init__ do not share method resolution so it does not matter which comes "first" in the MRO: __new__ is always called first and gets to decide whether __init__ is ever called at all.
  • The concrete type returned is determined solely by __new__ (leaving aside metaclasses overriding type.__call__) and importantly __new__ can return anything.
  • __init__ is irrelevant for determining the type of self: it is only ever called with an instance of the class in which it is defined and will not be called at all if __new__ returns something else.

I would expect the inference to work something like this. We have a class:

class A(B, metaclass=Meta):
    def __new__(cls, *args, **kwargs):
        ...
    def __init__(self, *args, **kwargs):
       ....

When we call A(*args, **kwargs) it will return Meta.__call__(A, *args, **kwargs) so we should determine what type that would return. By default Meta is type whose call method looks like:

class type:
    def __call__(A, *args, **kwargs):
        # Use __new__ to create the object
        if A.__new__ is object.__new__:
            obj = object.__new__(A)
        else:
            obj = A.__new__(A, *args, **kwargs)

        # Call __init__ with the created object (only if is an instance of A):
        if isinstance(obj, A):
            if type(obj).__init__ is not object.__init__:
                obj.__init__(*args, **kwargs)

        # Return the object obtained from __new__
        return obj

The final piece to know is that object.__new__(A) returns an object of type A. The call to A.__new__(A, ...) needs further analysis but usually it will eventually come down to object.__new__(A) (via calls like B.__new__(A) along the way).

We see here that the type of the object returned is just the return type of A.__new__(). If A.__new__() returns an object that is not an instance of A then A.__init__() will never be called meaning any hints in A.__init__() are irrelevant. Even if A.__init__() is called it does not change the type of obj (unless it reassigns obj.__class__ but that is very obscure).

@NeilGirdhar
Copy link
Contributor

Respecting the metaclass' __call__ return type is required to type check nnx, which makes extensive use of this feature here.

@Viicos
Copy link
Contributor

Viicos commented Dec 20, 2023

Respecting the metaclass' __call__ return type is required to type check nnx, which makes extensive use of this feature here.

As a workaround, I'm using the __new__ method instead (see #16020 (comment), where I'm hitting the same use case as you)

@jorenham
Copy link

This currently makes it impossible to correctly type builtins.reversed. See python/typeshed#11645 and python/typeshed#11646

@jorenham
Copy link

Like @oscarbenjamin noted, this fails in the contravariant case, i.e. when __new__ returns an instance of its supertype.

But when __new__ is annotated to return something else entirely, mypy will simply ignore it, even if annotated explicitly:

class Other:
    def __new__(cls, arg: T, /) -> T:  # error: "__new__" must return a class instance (got "T")  [misc]
        return arg

assert_type(Other(42), 'int')  # error: Expression is of type "Other", not "int"  [assert-type]

For details, see https://mypy-play.net/?mypy=latest&python=3.12&flags=strict&gist=336602d390b5e6566e9fca93d7fa48a6

@Viicos
Copy link
Contributor

Viicos commented Mar 25, 2024

The PR fixing this can be found here: #16020

@jorenham
Copy link

jorenham commented Jul 5, 2024

@Viicos #16020 does not fix this issue. In the PR description it explicit states:

We avoid fixing #15182 for now.

@jorenham
Copy link

jorenham commented Jul 5, 2024

The official typing specs now include a section on the __new__ method:

For most classes, the return type for the __new__ method is typically Self, but other types are also allowed. For example, the __new__ method may return an instance of a subclass or an instance of some completely unrelated class.

So this confirms that this is, in fact, a bug (and IMO a rather big one).

@oscarbenjamin
Copy link
Author

The relevant parts of the code have not changed since I linked them above. Within mypy __new__ is treated as an alternate version of __init__. There is no analysis of the sequence I described above (which matches the updated typing spec):

  • type.__call__
  • cls.__new__
  • obj.__init__

Having seen the code it is not surprising that other examples don't work as well (gh-14122):

class A:
    pass

class Meta(type):
    def __call__(self, *args, **kwargs) -> A:
        return A()

class B(metaclass=Meta):
    pass

print(type(B())) # A
reveal_type(B()) # B

@Viicos
Copy link
Contributor

Viicos commented Jul 6, 2024

@Viicos #16020 does not fix this issue. In the PR description it explicit states:

We avoid fixing #15182 for now.

Looking at the (updated) test cases, feels like it does? In particular:

# Check for invalid __new__ typing
class A:
def __new__(cls) -> int: # E: Incompatible return type for "__new__" (returns "int", but must return a subtype of "A")
pass
reveal_type(A()) # N: Revealed type is "builtins.int"

While a type ignore comment is needed, int is not a subclass of A and it type checks as expected.

@jorenham
Copy link

jorenham commented Jul 6, 2024

@Viicos Ah good, that's better than nothing I guess. But it still doesn't constitute a full fix, because that error is a false negative.
I also wonder if it'll still "work" when __new__ is overloadaed.

@oscarbenjamin
Copy link
Author

#16020 does not fix this issue.

Looking at the (updated) test cases, feels like it does?

Maybe in some cases but likely not others. As noted in the comments the code uses a "hack" to make this work for some cases. It begins by checking is_new which I believe is already incorrect because __new__ and __init__ are searched in a "combined MRO" as noted above.

I would expect that it still fails for other cases like this:

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)

class C(B):
    def __init__(self): # possibly use self:C here.
        pass

# C.__init__ is earlier in the "combined MRO" than B.__new__

reveal_type(B()) # should be A
reveal_type(C()) # should also be A

I think the overall logic in type_object_type and other places needs a rethink from first principles to be able to handle all cases. The presence of the is_new variable implies a confusion that __new__ and __init__ are somehow interchangeable so anything derived from it is likely incorrect for some cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

No branches or pull requests

4 participants