Support narrowing unions that include type[None]. #16315

tyralla · 2023-10-23T06:58:11Z

See my comment in the referenced issue.

# Conflicts: # test-data/unit/check-narrowing.test

tyralla · 2023-10-23T12:01:57Z

The primer's results show that the proposed changes also have other benefits. However, I have no idea what the failed mypyc runtime test is about. (I have no experience with Mypyc, so any hint is welcome.)

JelleZijlstra · 2023-10-23T14:40:17Z

The mypyc failure is likely spurious; it's been failing randomly for some time. I retriggered the job.

tyralla · 2023-10-23T15:21:07Z

Yes, you were right, thanks!

ghost · 2023-10-23T19:08:15Z

test-data/unit/check-narrowing.test

+NoneType_ = type(None)
+
+def f(cls: Type[Union[None, int]]) -> None:
+    if issubclass(cls, NoneType_):


One case is still broken, I believe: (python3.10+)

if issubclass(cls, types.Nonetype)

You are right. The definition in types.pyi is not helpful. I assumed something similar to mentioned test:

NoneType = type(None)

But it is:

@final class NoneType: def __bool__(self) -> Literal[False]: ...

My suggestion is to let ExpressionChecker.analyze_ref_expr directly return TypeType(NoneType()) when it encounters the full name "types.NoneType". (already pushed)

tyralla · 2023-10-24T05:30:05Z

The new unreachable error for clinic.py also seems correct because neither bool nor NoneType is an acceptable base type.

AlexWaygood · 2023-10-25T14:18:35Z

The new unreachable error for clinic.py also seems correct because neither bool nor NoneType is an acceptable base type.

The clinic code in question is here: https://github.com/python/cpython/blob/1262e41842957c3b402fc0cf0a6eb2ea230c828f/Tools/clinic/clinic.py#L5828-L5831

I agree that the clinic error is fine, but for a different reason to the one you gave ;)

It looks to me like there's a bug in the annotations for clinic.py. If I apply this diff to clinic.py:

diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py
index 5dd7900851..51964fb38e 100755
--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -5840,6 +5840,7 @@ def bad_node(self, node: ast.AST) -> None:
                         value = unknown
                 else:
                     value = ast.literal_eval(expr)
+                    reveal_type(value)
                     py_default = repr(value)
                     if isinstance(value, (bool, NoneType)):
                         c_default = "Py_" + py_default

And then run mypy --config-file Tools/clinic/mypy.ini, mypy reports (both using mypy 1.6 and using your PR branch):

Tools\clinic\clinic.py:5843: note: Revealed type is "Union[clinic.Sentinels, clinic.Null]"

That's because of an incorrect type declaration in clinic.py here: https://github.com/python/cpython/blob/1262e41842957c3b402fc0cf0a6eb2ea230c828f/Tools/clinic/clinic.py#L5719. The type for that variable should probably be declared as object or Any, not Sentinels | Null. So your PR is improving things there: mypy should be reporting an error on that line for unreachable code with mypy 1.6, but isn't.

tyralla · 2023-10-25T15:19:15Z

I agree that the clinic error is fine, but for a different reason to the one you gave ;)

Thanks for checking and confirming the error report is "true positive". But I see no contradiction between your and mine opinion (except that I definitely should have used more words; sorry for that).

Hm, I just wrote the following example to explain my thinking (the note and error messages are generated by Mypy 1.6):

from typing import final

class Null: ...

@final
class NoneType1: ...

class NoneType2: ...

class Impossible(Null, NoneType1): ...  # error: Cannot inherit from final class "NoneType"  [misc]  (right)

class Possible(Null, NoneType2): ...

x: Null

if isinstance(x, NoneType1):
    reveal_type(x)  # note: Revealed type is "temp.<subclass of "Null" and "NoneType1">"  (wrong)

if isinstance(x, NoneType2):
    reveal_type(x)  # note: Revealed type is "temp.<subclass of "Null" and "NoneType2">"  (right)

I intended to show that the unreachable warning is only correct for final types. (I somehow defined NoneType to be final in my pull request by setting a upper_bound attribute to False somewhere.) But now I see that Mypy understands that combining Null and NoneType1 is impossible due to NoneType1 being final, but still assumes such an impossible subtype can exist when doing type narrowing.

So, either I am confused, or this example reveals another Mypy limitation (that could possibly be easily fixed in this pull request as well).

tyralla · 2023-10-25T16:06:39Z

So, either I am confused, or this example reveals another Mypy limitation (that could possibly be easily fixed in this pull request as well).

already reported here: #15148

tyralla · 2023-10-26T15:59:44Z

So, either I am confused, or this example reveals another Mypy limitation (that could possibly be easily fixed in this pull request as well).

I decided to open #16330. So, in my opinion, the pull request here is ready.

AlexWaygood · 2023-10-26T16:09:05Z

I decided to open #16330. So, in my opinion, the pull request here is ready.

Yeah, wise to defer that to another PR, imo!

AlexWaygood · 2023-12-04T09:21:54Z

@tyralla, looks like there might be some merge conflicts here now — think you'd be able to merge in master and fix them? :)

# Conflicts: # test-data/unit/check-narrowing.test

tyralla · 2023-12-04T15:31:26Z

@AlexWaygood No big deal; just two tests inserted at the same place.

AlexWaygood · 2023-12-04T18:01:00Z

CPython (Argument Clinic) (https://github.com/python/cpython)
- Tools/clinic/clinic.py:5903: error: Subclass of "Sentinels" and "bool" cannot exist: "bool" is final  [unreachable]
- Tools/clinic/clinic.py:5903: error: Subclass of "Sentinels" and "NoneType" cannot exist: "NoneType" is final  [unreachable]
- Tools/clinic/clinic.py:5903: error: Subclass of "Null" and "bool" cannot exist: "bool" is final  [unreachable]
- Tools/clinic/clinic.py:5903: error: Subclass of "Null" and "NoneType" cannot exist: "NoneType" is final  [unreachable]

Hmm, any idea why these errors are going away? I see mypy still emits an error: Statement is unreachable [unreachable] error on line 5904 of clinic.py (following #16330). But the error messages that are going away here are IMO more helpful in figuring out what the actual error is in the clinic.py code

tyralla · 2023-12-05T05:48:08Z

Yes, something is still missing in this pull request. Smaller repro:

from types import NoneType
from typing import final

class X: ...
class Y: ...
@final
class Z: ...

x: X

if isinstance(x, (Y, Z)):
    reveal_type(x)  # N: note: Revealed type is "__main__.<subclass of "X" and "Y">" (right)
if isinstance(x, (Y, NoneType)):
    reveal_type(x)  # E: Statement is unreachable (wrong)

I will have a look at it later.

…oneType` and modify (fix?) the second algorithm tried in `conditional_types_with_intersection`.

…ot exist: "NoneType" is final` messages.

for more information, see https://pre-commit.ci

github-actions · 2023-12-05T09:36:03Z

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

alerta (https://github.com/alerta/alerta)
+ alerta/models/alert.py:38: error: Unused "type: ignore" comment  [unused-ignore]

discord.py (https://github.com/Rapptz/discord.py)
- discord/app_commands/transformers.py:729: error: Dict entry 0 has incompatible type "AppCommandOptionType": "tuple[type[str], <typing special form>]"; expected "AppCommandOptionType": "tuple[type[Any], ...]"  [dict-item]
- discord/app_commands/transformers.py:730: error: Dict entry 1 has incompatible type "AppCommandOptionType": "tuple[type[int], <typing special form>]"; expected "AppCommandOptionType": "tuple[type[Any], ...]"  [dict-item]
- discord/app_commands/transformers.py:731: error: Dict entry 2 has incompatible type "AppCommandOptionType": "tuple[type[bool], <typing special form>]"; expected "AppCommandOptionType": "tuple[type[Any], ...]"  [dict-item]
- discord/app_commands/transformers.py:732: error: Dict entry 3 has incompatible type "AppCommandOptionType": "tuple[type[float], <typing special form>]"; expected "AppCommandOptionType": "tuple[type[Any], ...]"  [dict-item]
- discord/app_commands/transformers.py:739: error: Incompatible default for argument "_none" (default has type "<typing special form>", argument has type "type")  [assignment]
- discord/app_commands/commands.py:235: error: Incompatible default for argument "_none" (default has type "<typing special form>", argument has type "type")  [assignment]

tyralla · 2024-01-09T05:04:41Z

@AlexWaygood This has been ready for a while now. Can you merge it, or is there anything left to do?

AlexWaygood · 2024-01-09T07:32:30Z

@AlexWaygood This has been ready for a while now. Can you merge it, or is there anything left to do?

I'm not a maintainer, just a triager, I'm afraid, so I can review but I can't merge :/ I'm also afraid I'm unusually busy at the moment, so I probably won't be able to take another look at this soon :(

tyralla · 2024-01-09T08:02:12Z

Ah, okay. I didn't know Mypy has triagers (to be honest, I didn't even know that "triager" is an English word...).

So, this means this pull request has to wait until you find time to finish your review and give the maintainers your okay? (No need for a rush; I just wanted to make sure this won't be forgotten.)

AlexWaygood · 2024-01-09T08:06:36Z

So, this means this pull request has to wait until you find time to finish your review and give the maintainers your okay? (No need for a rush; I just wanted to make sure this won't be forgotten.)

Not necessarily; it can be merged as soon as a maintainer approves it. That's maybe slightly more likely to happen if a triager approves it first, but it's very marginal — most PRs at mypy are just reviewed by a maintainer before being merged, as we actually have more maintainers than triagers. But as with most open-source projects, there aren't enough reviewers, so it can unfortunately take some time sometimes :)

hauntsaninja

Nice, thank you for the improvement!

tyralla added 2 commits October 23, 2023 08:53

Support narrowing unions that include type[None].

05f2c71

Merge branch 'master' into fix/narrow_nonetype

1ea576c

# Conflicts: # test-data/unit/check-narrowing.test

tyralla mentioned this pull request Oct 23, 2023

can't narrow a union of type[None] #16279

Closed

This comment has been minimized.

Sign in to view

ghost reviewed Oct 23, 2023

View reviewed changes

special case types.NoneType

32ca5f4

This comment has been minimized.

Sign in to view

tyralla mentioned this pull request Oct 26, 2023

Do not intersect types in isinstance checks if at least one is final. #16330

Merged

tyralla requested a review from a user October 28, 2023 06:17

Merge branch 'master' into fix/narrow_nonetype

fc853e9

# Conflicts: # test-data/unit/check-narrowing.test

This comment has been minimized.

Sign in to view

Add test case `testNarrowingIsInstanceNoIntersectionWithFinalTypeAndN…

fe12f7d

…oneType` and modify (fix?) the second algorithm tried in `conditional_types_with_intersection`.

This comment has been minimized.

Sign in to view

tyralla and others added 2 commits December 5, 2023 10:19

A little additional effort for `Subclass of "..." and "NoneType" cann…

58f0562

…ot exist: "NoneType" is final` messages.

[pre-commit.ci] auto fixes from pre-commit.com hooks

55b2858

for more information, see https://pre-commit.ci

hauntsaninja added the upnext label Jan 9, 2024

hauntsaninja approved these changes Jan 14, 2024

View reviewed changes

hauntsaninja merged commit 261e569 into python:master Jan 14, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support narrowing unions that include type[None]. #16315

Support narrowing unions that include type[None]. #16315

tyralla commented Oct 23, 2023

This comment has been minimized.

tyralla commented Oct 23, 2023

JelleZijlstra commented Oct 23, 2023

tyralla commented Oct 23, 2023

ghost Oct 23, 2023

tyralla Oct 24, 2023

This comment has been minimized.

tyralla commented Oct 24, 2023

AlexWaygood commented Oct 25, 2023 •

edited

tyralla commented Oct 25, 2023

tyralla commented Oct 25, 2023

tyralla commented Oct 26, 2023

AlexWaygood commented Oct 26, 2023

AlexWaygood commented Dec 4, 2023

tyralla commented Dec 4, 2023

This comment has been minimized.

AlexWaygood commented Dec 4, 2023

tyralla commented Dec 5, 2023

This comment has been minimized.

github-actions bot commented Dec 5, 2023

tyralla commented Jan 9, 2024

AlexWaygood commented Jan 9, 2024

tyralla commented Jan 9, 2024

AlexWaygood commented Jan 9, 2024 •

edited

hauntsaninja left a comment

Support narrowing unions that include type[None]. #16315

Support narrowing unions that include type[None]. #16315

Conversation

tyralla commented Oct 23, 2023

This comment has been minimized.

tyralla commented Oct 23, 2023

JelleZijlstra commented Oct 23, 2023

tyralla commented Oct 23, 2023

ghost Oct 23, 2023

Choose a reason for hiding this comment

tyralla Oct 24, 2023

Choose a reason for hiding this comment

This comment has been minimized.

tyralla commented Oct 24, 2023

AlexWaygood commented Oct 25, 2023 • edited

tyralla commented Oct 25, 2023

tyralla commented Oct 25, 2023

tyralla commented Oct 26, 2023

AlexWaygood commented Oct 26, 2023

AlexWaygood commented Dec 4, 2023

tyralla commented Dec 4, 2023

This comment has been minimized.

AlexWaygood commented Dec 4, 2023

tyralla commented Dec 5, 2023

This comment has been minimized.

github-actions bot commented Dec 5, 2023

tyralla commented Jan 9, 2024

AlexWaygood commented Jan 9, 2024

tyralla commented Jan 9, 2024

AlexWaygood commented Jan 9, 2024 • edited

hauntsaninja left a comment

Choose a reason for hiding this comment

AlexWaygood commented Oct 25, 2023 •

edited

AlexWaygood commented Jan 9, 2024 •

edited