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

More helpful type guards #14238

Merged
merged 9 commits into from
Jan 31, 2023
Merged

More helpful type guards #14238

merged 9 commits into from
Jan 31, 2023

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Dec 3, 2022

A TODO I found while looking at something else got me down this rabbit hole, and I noticed mypy's subpar performance in these two cases.

Short descriptions from before I did this, so may be technically inaccurate:

  1. check when defining a type guard whether the function is right or not
  2. follow kw args if they go to a regular positional first arg

This may fix an issue but I have not conducted any searches.
Fixes #13199
Refs #14425

> Type checkers should assume that type narrowing should be applied to the expression that is passed as the first positional argument to a user-defined type guard. [...]
>
> If a type guard function is implemented as an instance method or class method, the first positional argument maps to the second parameter (after “self” or “cls”).
@github-actions

This comment has been minimized.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 3, 2022

mypy primer results seem to be narrowing on self:

    @abstractmethod
    def is_error(self) -> TypeGuard[Error[_TSource, _TError]]:
        """Returns `True` if the result is an `Error` value."""

        raise NotImplementedError

This seems... wrong? Firstly, the PEP says this (as mentioned in one of my commit messages):

If a type guard function is implemented as an instance method or class method, the first positional argument maps to the second parameter (after “self” or “cls”).

This was even explicitly rejected! https://peps.python.org/pep-0647/#narrowing-of-implicit-self-and-cls-parameters

Secondly, mypy doesn't even support this!! At the moment:

from typing import TypeGuard

class A:
  def n(self) -> TypeGuard[int]:
    return False

a = A()
if a.n():
  reveal_type(a)  # N: Revealed type is "__main__.A"

However, I realize I missed staticmethod special casing. Will fix.

from typing_extensions import TypeGuard

class Z:
    @staticmethod
    def typeguard(h: object) -> TypeGuard[int]:  # E: TypeGuard functions must have a positional argument
        return isinstance(h, int)

x: object
if Z().typeguard(x):
    reveal_type(x)
if Z.typeguard(x):
    reveal_type(x)

@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.

Thanks! I have a couple of questions, this is not a full review.

mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Show resolved Hide resolved
mypy/semanal.py Show resolved Hide resolved
@A5rocks A5rocks requested a review from sobolevn December 5, 2022 14:08
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 10, 2022

OK with that tests should be added as requested. I've got one additional question that I've moved over to #14273 because it may cause more debate. Hopefully this as it stands is alright!

@github-actions

This comment has been minimized.

@A5rocks A5rocks requested a review from sobolevn December 11, 2022 06:33
@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member

sobolevn commented Dec 11, 2022

Cases that error:

   
    @abstractmethod
    def is_error(self) -> TypeGuard[Error[_TSource, _TError]]:
        """Returns `True` if the result is an `Error` value."""

        raise NotImplementedError

    @abstractmethod
    def is_ok(self) -> TypeGuard[Ok[_TSource, _TError]]:
        """Returns `True` if the result is an `Ok` value."""

        raise NotImplementedError

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 11, 2022

Yep! As mentioned before, that is invalid as according to the PEP and mypy doesn't support it anyways. Having an error at definition time is likely beneficial.

@AlexWaygood AlexWaygood added the topic-typeguard TypeGuard / PEP 647 label Dec 11, 2022
@A5rocks
Copy link
Contributor Author

A5rocks commented Jan 12, 2023

Someone ran into this in the issue tracker; I have updated the "fixes" section.

@hauntsaninja hauntsaninja mentioned this pull request Jan 26, 2023
17 tasks
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

def typeguard(x: object, y: int) -> TypeGuard[int]:
...

def typeguard(x: object, y: Union[int, str]) -> Union[TypeGuard[int], TypeGuard[str]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this return type makes sense. But can add an error in another PR

@github-actions
Copy link
Contributor

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

Tanjun (https://github.com/FasterSpeeding/Tanjun)
- version: 1.0.0+dev.7c14ebae6ca5d6ec39600e122b58a62afaa3ab02
+ version: 1.0.0+dev.d490f40dcc4f6244b2c486e9f6b6a7234763310f
-   File "/semanal.py", line 6235, in accept
+   File "/semanal.py", line 6249, in accept
-   File "/semanal.py", line 5909, in defer
+   File "/semanal.py", line 5923, in defer

Expression (https://github.com/cognitedata/Expression)
+ expression/core/result.py:125: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/result.py:131: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/result.py:212: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/result.py:217: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/result.py:323: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/result.py:327: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/option.py:139: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/option.py:144: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/option.py:214: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/option.py:218: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/option.py:330: error: TypeGuard functions must have a positional argument  [valid-type]
+ expression/core/option.py:334: error: TypeGuard functions must have a positional argument  [valid-type]

@hauntsaninja
Copy link
Collaborator

Oh hmm, the crash on FasterSpeeding/Tanjun is new (not to this PR, just new in general)

@hauntsaninja hauntsaninja merged commit 28c67cb into python:master Jan 31, 2023
@A5rocks A5rocks deleted the typeguard-ocd branch March 24, 2023 02:24
hauntsaninja pushed a commit that referenced this pull request Mar 26, 2023
I was checking through my previous contributions and noticed I messed up
the logic here. I added a test case that currently is wrong.

Followup to #14238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-typeguard TypeGuard / PEP 647 upnext
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no error when no argument in TypeGuard function
4 participants