Skip to content

Fix TypeGuard Explicit Any Check #12050

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

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

Conversation

hmc-cs-mdrissi
Copy link
Contributor

@hmc-cs-mdrissi hmc-cs-mdrissi commented Jan 24, 2022

Description

Fixes #12049. This pr is stacked on top of this pr. I would only review the last commit for this pr and ignored required portion of it.

This adds an explicit Any check for TypeGuard argument. This requires adding an accept method to TypeGuard. To avoid new type classes not defining accept I marked accept abstract method.

Two open questions,

  1. The way I handled explicit Any check was by adding it to the argument analysis. Ideally function def analysis for explicit Any would take care of this but it doesn't work because the return type is transformed to bool for typeguards. Removing that transformation looks like a harder change.
  2. My accept implementation just delegates to the inner type for typeguards. Maybe type visitor should have a new method added of visit_type_guard(...) with the default being visit the inner type but allow other visitors to do something else. Maybe this issue can be avoided until a visitor that cares about typeguardness appears.

Test Plan

I ran local tests and checked that my test case in the bug report is fixed. The error message produced for this code,

from typing import Any, Union

from typing_extensions import TypeGuard


def test_any_typeguard(_mypy_marker: Union[int, str]) -> TypeGuard[Any]:
    return isinstance(_mypy_marker, int)
typeguard_any.py:6: error: Explicit "Any" is not allowed
Found 1 error in 1 file (checked 1 source file)

I'll add a test case after agreement this approach is reasonable.

@hmc-cs-mdrissi hmc-cs-mdrissi changed the title Fix Fix TypeGuard Explicit Any Check Jan 24, 2022
@hmc-cs-mdrissi
Copy link
Contributor Author

Hmm I don't think my current approach makes much sense for other type visitors. Delegating visitor to the inner type works for explicit any check, but x: T and x: TypeGuard[T] are very different with the latter not even being valid as TypeGuard only works as a return type. And the argument doesn't correspond to the actual return type.

I'm currently thinking I should instead add a visit_type_guard function to TypeVisitor that other visitor classes would need to override to pick a reasonable way of handling typeguards.

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!

@@ -171,7 +171,7 @@ def deserialize_type(data: Union[JsonDict, str]) -> 'Type':
raise NotImplementedError('unexpected .class {}'.format(classname))


class Type(mypy.nodes.Context):
class Type(mypy.nodes.Context, ABC):
Copy link
Member

Choose a reason for hiding this comment

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

This is not required, we can catch missing @abstractmethods with mypy itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, interesting this is one place mypy/pyright differs microsoft/pyright#2547. Pyright requires ABC as during runtime the error is only raised for ABC.

I'm fine dropping ABC, just wanted to note if you want run other type checkers on mypy's codebase this will make a difference.

@@ -43,7 +43,8 @@ class Iterator(Iterable[T_co], Protocol):
def __next__(self) -> T_co: pass

class Sequence(Iterable[T_co]):
def __getitem__(self, n: Any) -> T_co: pass
# misc is for explicit Any.
def __getitem__(self, n: Any) -> T_co: pass # type: ignore[misc]
Copy link
Member

Choose a reason for hiding this comment

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

n: int?

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.

Explicit Any check for TypeGuard
2 participants