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

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

Merged
merged 1 commit into from Dec 4, 2023

Conversation

tyralla
Copy link
Contributor

@tyralla tyralla commented Oct 26, 2023

Fixes #15148

I think it also fixes the initial bug reported in #12163 (this is why I added a TypeVar test case) but not this bug reported later in the same issue.

@github-actions
Copy link
Contributor

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

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

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/util/typing.py:205: error: Unused "type: ignore" comment  [unused-ignore]

@tyralla
Copy link
Contributor Author

tyralla commented Oct 26, 2023

Regarding the Mypy primer:

The (correct) new error reports for clinic.py show that this commit somehow partially overlaps with #16315.

For sphinx, I am unsure why the affected branch was not considered unreachable in the first place, like in the following example (where I apply Mypy 1.6):

from typing import Any, Type, _SpecialForm

cls: Type[Any]

if isinstance(cls, _SpecialForm):
    cls.asdf  # error: Statement is unreachable

@tyralla
Copy link
Contributor Author

tyralla commented Oct 27, 2023

The sphinx issue caused me some headaches, but I think I've collected some hints.

The argument cls of function restify is annotated with type | None but should be annotated (at least) with str | type | None. Mypy should complain about this in the implemented if isinstance(cls, str): check but doesn't because it assumes the intersection "sphinx.util.typing.<subclass of "type" and "str">" which in my opinion is impossible because of instance layout conflicts (and I cannot think of a way this makes sense).

Interestingly, Mypy 1.6 agrees with my reasoning when annotating with Type instead of type:

from typing import Type

t1: type
t2: Type

if isinstance(t1, str):
    reveal_type(t1)  # note: Revealed type is "temp.<subclass of "type" and "str">"
if isinstance(t2, str):
    reveal_type(t2)  # error: Statement is unreachable

If I change the annotation of cls from type | None to Type | None, Mypy 1.6 also does not require the type: ignore[attr-defined] comment anymore because it considers the affected line unreachable.

So, everything seems clear. However, when applying Mypy 1.6 on the following example code involving _SpecialForm instead of the complete sphinx code, it finds out about the unreachability no matter if annotating with type or Type:

from typing import Type, _SpecialForm

t1: type
t2: Type

if isinstance(t1, _SpecialForm):  # error: Subclass of "type" and "<typing special form>" cannot exist: would have incompatible method signatures
    reveal_type(t1)  # error: Statement is unreachable
if isinstance(t2, _SpecialForm):
    reveal_type(t2)  # error: Statement is unreachable

To conclude, strange things might be going on, but they are not related to this pull request.

Copy link
Contributor

@KotlinIsland KotlinIsland left a comment

Choose a reason for hiding this comment

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

LGTM!

@tyralla
Copy link
Contributor Author

tyralla commented Oct 28, 2023

@KotlinIsland: Thanks for looking over it!

@KotlinIsland
Copy link
Contributor

KotlinIsland commented Oct 28, 2023

@KotlinIsland: Thanks for looking over it!

@tyralla All good, I had started implementing this in basedmypy, but now I don't have to, thanks!

@hauntsaninja
Copy link
Collaborator

Nice feature, thank you!

@hauntsaninja hauntsaninja merged commit c224da5 into python:master Dec 4, 2023
18 checks passed
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.

(🎁) Intersections with @final types should trigger unreachable errors
3 participants