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

Use TypeGuard for has in Python 3.10 and above #997

Merged
merged 8 commits into from Aug 16, 2022

Conversation

layday
Copy link
Contributor

@layday layday commented Aug 8, 2022

Summary

Conditionally defines has using TypeGuard in Python 3.10+ in the stub file.

xref: #987

Pull Request Check List

  • Added tests for changed code.
    Our CI fails if coverage is not 100%.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
    • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

@Tinche
Copy link
Member

Tinche commented Aug 8, 2022

I don't see a direct use-case for this (since attrs classes should be statically known now) but I don't see the harm either.

@layday
Copy link
Contributor Author

layday commented Aug 8, 2022

The class might not be statically known to be an attrs class (#987, #996) or it can appear in a union with other types.

@hynek
Copy link
Member

hynek commented Aug 10, 2022

So what exactly does this do?

@Tinche
Copy link
Member

Tinche commented Aug 10, 2022

It essentially makes has work at compile time too.

Let's say your function takes a class (not an instance, but an actual class). You will type your argument as type (or type[C]). You can check if it's an attrs class with has, and if that's true you can pass it to a different function that takes type[AttrsInstance]. But Mypy will still yell at you, since it doesn't know that has means it's an attrs class.

If we annotate has with TypeGuard, Mypy will know.

Code example:

def needs_attrs_class(cls: type[AttrsInstance]) -> None:
    pass

def my_function(cls: type) -> None:
    if has(cls):
        needs_attrs_class(cls)  # Mypy's gonna be unhappy, unless it knows `has` narrows the type of is argument

@layday
Copy link
Contributor Author

layday commented Aug 10, 2022

It tells the type checker that if the condition is true then cls is of type type[AttrsInstance]. Simplified example:

def is_dict_no_typeguard(value: object) -> bool:
    return isinstance(value, dict)

def is_dict_typeguard(value: object) -> TypeGuard[dict]:
    return isinstance(value, dict)

might_be_a_dict = ...

if is_dict_no_typeguard(might_be_a_dict):
    "type of might_be_a_dict is object"

if is_dict_typeguard(might_be_a_dict):
    "type of might_be_a_dict is dict"

@hynek
Copy link
Member

hynek commented Aug 10, 2022

Sounds cool! Any reason against it? If not: make it happen Tin. ;)

@@ -470,7 +470,15 @@ def astuple(
tuple_factory: Type[Sequence[Any]] = ...,
retain_collection_types: bool = ...,
) -> Tuple[Any, ...]: ...
def has(cls: type) -> bool: ...

if sys.version_info >= (3, 10):
Copy link
Member

Choose a reason for hiding this comment

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

Would it be feasible to add a conditional import from typing-extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can only use one of a small number of constants like TYPE_CHECKING and sys.version_info to define types conditionally, try/except won't work, so I don't think so.

Copy link
Member

Choose a reason for hiding this comment

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

In my very limited testing this seems to work, both in Mypy and Pyright:

Screenshot 2022-08-15 at 13 37 05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but typing_extensions is not a dependency of attrs, so that'll error if typing_extensions is not installed.

Copy link
Contributor Author

@layday layday Aug 15, 2022

Choose a reason for hiding this comment

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

Okay, it seems typeshed ships typing_extensions as part of the stdlib, so I guess we can actually do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in fd2c819.

@hynek hynek added this to the 22.2.0 milestone Aug 11, 2022
@hynek
Copy link
Member

hynek commented Aug 15, 2022

Would you mind coming up with a way to verify this works in https://github.com/python-attrs/attrs/blob/main/tests/typing_example.py ?

@layday
Copy link
Contributor Author

layday commented Aug 15, 2022

There's a test in test_mypy.yml. Should I move it over to typing_example.py?

@hynek
Copy link
Member

hynek commented Aug 15, 2022

No, both would be great, since they work somewhat differently. Maybe if has(c): print(c.__attrs_attrs__) or something like that? That should pass, right?

@hynek
Copy link
Member

hynek commented Aug 16, 2022

Just so I understand this correctly: the import from typing_extensions only works, because we have the stubs in pyi files that are only touched by mypy which in turn brings in typing_extensions? IOW: if we'd use inline type hints, this would fall apart?

@hynek hynek merged commit ca6ce8c into python-attrs:main Aug 16, 2022
hynek added a commit that referenced this pull request Aug 16, 2022
@layday
Copy link
Contributor Author

layday commented Aug 16, 2022

That's right. The typeshed has typing_extensions in the stdlib. I assume they did this so that they can use new typing constructs in the typeshed without tripping MyPy. At runtime, you'd have to do something like:

import typing

if typing.TYPE_CHECKING:
    from typing_extensions import TypeGuard

# Use TypeGuard either in quotes or with `from __future__ import annotations`

@hynek
Copy link
Member

hynek commented Aug 16, 2022

Thanks!

hauntsaninja pushed a commit to python/mypy that referenced this pull request Aug 30, 2023
Since python-attrs/attrs#890 (≥ 22.1.0)
`attrs.fields` is typed to accept a protocol.
Since python-attrs/attrs#997 (≥ 22.2.0)
`attrs.has` is a type-guard.

Support both by removing the explicit error reporting and letting it
fall through to the type stub.

Fixes #15980.
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.

None yet

3 participants