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

Believe more __new__ return types #16020

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Sep 2, 2023

Fixes #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.

Fixes #14471. Fixes #8330.
We avoid fixing #15182 for now.

Co-authored-by: ikonst

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
@github-actions

This comment has been minimized.

@hauntsaninja hauntsaninja changed the title Believe arbitrary __new__ return types Believe more __new__ return types Sep 2, 2023
@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator Author

There's an issue where we turn TypeType's into CallableType, but the type_obj of CallableType just looks at the return type of the constructor. So this doesn't work as expected:

class A:
    def __new__(cls) -> int: raise

class B(A):
    @classmethod
    def foo(cls) -> int: raise

reveal_type(B.foo())

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninja hauntsaninja marked this pull request as ready for review September 4, 2023 02:44
@Viicos
Copy link
Contributor

Viicos commented Sep 4, 2023

Thanks so much @hauntsaninja, couldn't get this far due to limited knowledge of the mypy internals

@github-actions

This comment has been minimized.

# The following is a hack. mypy often represents types as CallableType, where the signature of
# CallableType is determined by __new__ or __init__ of the type (this logic is in
# type_object_type). Then if we ever need the TypeInfo or an instance of the type, we fish
# around for the return type in CallableType.type_object. Unfortunately, this is incorrect if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we fish around for the return type in CallableType.type_object

Where do we do this? This function certainly didn't, going for simple .ret_type

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I clarified the comment a little bit. type_object also just uses ret_type. Also, thanks for the suggestion to use bound_args, I added you as a co-author of this PR!

@github-actions

This comment has been minimized.

# see e.g. testGenericClassMethodUnboundOnClass. So just copy them over to our type.
# This does the wrong thing with custom __new__, see testNewReturnType15, but is
# no worse than previous behaviour.
ret_type = bound_arg.copy_modified(args=ret_type.args)
Copy link
Contributor

@ikonst ikonst Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can

Suggested change
ret_type = bound_arg.copy_modified(args=ret_type.args)
ret_type = bound_arg.copy_modified()

if you change checkexpr.py's apply_generic_arguments to expand bound_args (not sure if it's correct to do...):

 return callable.copy_modified(
     ret_type=expand_type(callable.ret_type, id_to_type),
+    bound_args=[
+        expand_type(ba, id_to_type) if ba is not None else None
+        for ba in callable.bound_args
+    ],
     variables=remaining_tvars,
     type_guard=type_guard,
 )

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I don't like this. This introduces some hack(s) that can backfire unexpectedly in "regular" cases to support a truly weird (even if somewhat common) use case.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Sep 6, 2023

Yeah, it's a little icky :-( I'm curious what "regular" cases you think this makes problematic? I do think it's something we should support, so would like to find a way forward. E.g. it came up again yesterday in python/typeshed#10662 (comment)

I'd also definitely be open to removing this back compat hack https://github.com/python/mypy/pull/16020/files#diff-1dce7e2a19559e2ff4e08fd46877b87c9cfec3cb5e6d465157788542948e11e4R206-R208 . It does result in a few mypy_primer hits, but they're technically correct.

@ilevkivskyi
Copy link
Member

I'm curious what "regular" cases you think this makes problematic?

I think various operations with class objects, like type applications, subtyping/inference against them, member access on them etc. Also TBH I don't even remember why do we need bound_args, but I will not be surprised if various default visitors don't handle them correctly (because they are so rarely used). So relying on some important information being stored there if quite dangerous.

@ikonst
Copy link
Contributor

ikonst commented Sep 6, 2023

I'd also rather have the type in a specialized attribute than bound_args[0]. Maintenance-wise it's cheaper to add another attribute than to add semantics to bound_args[0].

@Viicos
Copy link
Contributor

Viicos commented Oct 13, 2023

Do you think the following use case with generics will be supported?

from typing import Generic, TypeVar

T = TypeVar("T")

class A(Generic[T]):
   # Having `T` returned at runtime would only be possible with a metaclass.
   # This is a hack that should most probably be in an `if TYPE_CHECKING` block
   # To describe the runtime behaviour of the metaclass' `__call__` method for example.  
    def __new__(cls, *args, **kwargs) -> T:
        ...

class Model:
    pass

reveal_type(A[Model]())  # Type of "A[Model]()" is "Model"

class B(A[Model]):
    pass

reveal_type(B())  # Type of "B()" is "Model"

This is a workaround for FactoryBoy/factory_boy#903 (comment), and could probably be used for any situation where we want to take into account the return type of __call__ (#14122).

pyright supports it.

# if __new__ returns an unrelated type, but we can kind of salvage things here using
# CallableType.bound_args
self_type: Type = typ
if len(item.bound_args) == 1 and item.bound_args[0]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with other comments that using bound_args doesn't look like something we want to do. I think storing the real self type in a new attribute might be a better approach. All type operations should be updated to do something reasonable with the attribute.

It seems like the above should also fix the issue that no error is currently reported here, and perhaps other related issues:

class C:
    def __new__(self) -> D:
        return D()

class D(C):
    x: int

C.x  # No Error, but fails at runtime

We'd use use the new attribute when looking up class attributes.

@willwill2will54
Copy link

I appreciate this PR. It helps correctly type the dominate library, which currently cannot be correctly typed for some of it's API.

Copy link
Contributor

github-actions bot commented Jul 5, 2024

Diff from mypy_primer, showing the effect of this PR on open source code:

dd-trace-py (https://github.com/DataDog/dd-trace-py)
+ ddtrace/propagation/http.py:1059: error: Unused "type: ignore" comment  [unused-ignore]
+ ddtrace/propagation/http.py:1061: error: Argument 2 to "_attach_baggage_to_context" has incompatible type "None"; expected "Context"  [arg-type]
+ ddtrace/propagation/http.py:1062: error: Incompatible return value type (got "None", expected "Context")  [return-value]

steam.py (https://github.com/Gobot1234/steam.py)
- steam/client.py:1321: error: Invalid self argument "type[TradeOffer[Any, Any, Any]]" to attribute function "_from_history" with type "Callable[[type[TradeOffer[MovedItem[UserT], MovedItem[ClientUser], UserT]], ConnectionState, TradeOfferHistoryTrade, Sequence[Description]], TradeOffer[MovedItem[UserT], MovedItem[ClientUser], UserT]]"  [misc]
+ steam/client.py:1321: error: Invalid self argument "type[TradeOffer[ReceivingAssetT, SendingAssetT, UserT]]" to attribute function "_from_history" with type "Callable[[type[TradeOffer[MovedItem[UserT], MovedItem[ClientUser], UserT]], ConnectionState, TradeOfferHistoryTrade, Sequence[Description]], TradeOffer[MovedItem[UserT], MovedItem[ClientUser], UserT]]"  [misc]

pandera (https://github.com/pandera-dev/pandera)
+ pandera/api/dataframe/model.py:395: error: "type[DataFrameBase[Any]]" has no attribute "Config"  [attr-defined]
+ tests/strategies/test_strategies.py:996: error: Need type annotation for "sample_data"  [var-annotated]
+ tests/dask/test_dask.py:30: error: Need type annotation for "ddf"  [var-annotated]
+ tests/dask/test_dask.py:30: note: Error code "var-annotated" not covered by "type: ignore" comment
+ tests/dask/test_dask.py:33: error: Need type annotation for "ddf"  [var-annotated]
+ tests/dask/test_dask.py:33: note: Error code "var-annotated" not covered by "type: ignore" comment

@jorenham
Copy link

jorenham commented Jul 8, 2024

@hauntsaninja Perhaps you're already aware, but the official typing specs have a section on the __new__ method nowadays.

@Viicos
Copy link
Contributor

Viicos commented Jul 8, 2024

This new spec chapter is pretty complex, I don't know how much of it is being already implemented in mypy (e.g. #14122 is not). Perhaps this PR could be considered as a quick win?

@jorenham
Copy link

jorenham commented Jul 8, 2024

I agree @Viicos; all progress towards compatibility with the typing spec is a win.

The reason I mentioned it is that if one is unaware of it; there's a chance of deviating away from it.
But don't get me wrong; I'm not suggesting that that's necessarily the case here, as I'm not too familiar with the mypy codebase (or its current state w.r.t. the typing spec).

@JelleZijlstra
Copy link
Member

You can be assured that @hauntsaninja is aware of the spec change; he signed off on it after all (python/typing-council#25) :)

Fully implementing that spec change in mypy will be more work but this PR seems like a step in the right direction, so hopefully we'll be able to get it ready to merge soon.

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

Successfully merging this pull request may close these issues.

Respecting return type of __new__ does not extend to derived classes Honor return type of __new__
8 participants