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

Allow returning Literals in __new__ #15687

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

Conversation

Gobot1234
Copy link
Contributor

@github-actions

This comment has been minimized.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, add tests to this change :)

@Gobot1234 Gobot1234 requested a review from sobolevn July 19, 2023 09:27
@github-actions

This comment has been minimized.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, also add an @overload case you are proposing to typeshed

test-data/unit/fixtures/__new__-literals.pyi Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Gobot1234 Gobot1234 requested a review from sobolevn July 31, 2023 22:57
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Let's ask for an extra pair of eyes.
@ilevkivskyi would you, please? :)

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.

Looks good, added a suggestion to improve the tests.

test-data/unit/check-classes.test Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@Gobot1234
Copy link
Contributor Author

Gobot1234 commented Aug 4, 2023

Somehow reveal_type(bool.__new__(bool, Falsy())) is Literal[False] but bool(Falsy()) isn't. Is there some special casing I'm not seeing here?

@ilevkivskyi
Copy link
Member

@Gobot1234 Yes, I think you likely need to add Literal somewhere here

mypy/mypy/typeops.py

Lines 195 to 207 in cfd01d9

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

@Gobot1234
Copy link
Contributor Author

@Gobot1234 Yes, I think you likely need to add Literal somewhere here

mypy/mypy/typeops.py

Lines 195 to 207 in cfd01d9

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

Thank you, never would have spotted that! Everything should work now

@github-actions

This comment has been minimized.

@Gobot1234
Copy link
Contributor Author

Not entirely sure why CI is failing from my testing this seems to work fine
image
image
are the fixtures somehow causing this to fail?

@ilevkivskyi
Copy link
Member

FWIW you are doing something weird with the fixtures. Try placing your updated definitions of bool and int in the fixture, and then add reveal_type()s directly in the test case body (not in [file builtins.py]).

@Gobot1234
Copy link
Contributor Author

Like this?

[case testOverride__new__WithLiteralReturnPassing]
from typing import Literal

class Falsy:
    def __bool__(self) -> Literal[False]: pass

reveal_type(bool(Falsy()))  # N: Revealed type is "Literal[False]"
reveal_type(int())  # N: Revealed type is "Literal[0]"

[file builtins.py]
from typing import Literal, Protocol, overload

class str: pass
class dict: pass
class float: pass
class int:
    def __new__(cls) -> Literal[0]: pass

class _Truthy(Protocol):
    def __bool__(self) -> Literal[True]: pass

class _Falsy(Protocol):
    def __bool__(self) -> Literal[False]: pass

class bool(int):
    @overload
    def __new__(cls, __o: _Truthy) -> Literal[True]: pass
    @overload
    def __new__(cls, __o: _Falsy) -> Literal[False]: pass
    def __new__(cls, __o: object):
        pass
[typing fixtures/typing-medium.pyi]

It doesn't seem to work

@ilevkivskyi
Copy link
Member

No, move all that stuff from [file builtins.py] into actual fixture, e.g. in test-data/unit/fixtures/__new__.pyi

@Gobot1234
Copy link
Contributor Author

No, move all that stuff from [file builtins.py] into actual fixture, e.g. in test-data/unit/fixtures/__new__.pyi

Doing so doesn't seem to have changed anything, sorry if I'm misunderstanding

@ilevkivskyi
Copy link
Member

Hm, to debug this, try adding reveal_type(bool) to figure out when the problem happens, during definition, or when calling it.

@Gobot1234
Copy link
Contributor Author

bool is Any

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

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.

3 participants