Skip to content

Handles type checks for Type[Protocol] - #251

Merged
agronholm merged 2 commits into
agronholm:masterfrom
williamlw999-fb:fix_type_protocol
May 8, 2022
Merged

Handles type checks for Type[Protocol]#251
agronholm merged 2 commits into
agronholm:masterfrom
williamlw999-fb:fix_type_protocol

Conversation

@williamlw999-fb

Copy link
Copy Markdown
Contributor

I noticed that typeguard wasn't able to correctly type check passed in classes when the type hint was a Type[Protocol]. You would receive an error like: TypeError: Protocols with non-method members don't support issubclass(). Using isinstance() in these situations would be able to correctly type check the passed in class. I also added a warning when a Protocol wasn't annotated with runtime_checkable to make it more explicit when typeguard was unable to perform type checking.

Tests:
Added a data protocol to improve coverage
Added a test for checking Type[Protocol]

@deathowl

deathowl commented Apr 27, 2022

Copy link
Copy Markdown
Contributor

Hey @agronholm , could you take a look at this PR? As this would be needed in testslide facebook/TestSlide#333.

@williamlw999-fb
williamlw999-fb force-pushed the fix_type_protocol branch 2 times, most recently from 4be6e31 to 4da33fb Compare April 27, 2022 16:00
@agronholm

Copy link
Copy Markdown
Owner

The 2.x series (current master) has been frozen for new features; you should rebase this against the 3.0 branch.

@agronholm

Copy link
Copy Markdown
Owner

I've merged the 3.0 branch to master now to prevent any further confusion.

@coveralls

coveralls commented May 2, 2022

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.04%) to 88.642% when pulling bd2982b on williamlw999-fb:fix_type_protocol into a298d6c on agronholm:master.

Handles type checks for Type[Protocol]
@williamlw999-fb

Copy link
Copy Markdown
Contributor Author

@agronholm Thanks for helping out with the 3.0 rebase! I've updated my PR and made sure all signals are green. Let me know if there's anything else you'd like me to address for this PR

@agronholm

Copy link
Copy Markdown
Owner

Looks good at a glance; I will do a more thorough review later.

@agronholm

Copy link
Copy Markdown
Owner

This PR confuses me. You seem to be dropping type checking support for static protocols. Why?

@williamlw999-fb

williamlw999-fb commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

Static protocols were never supported by Typeguard. See line 501 of _checkers.py: if getattr(origin_type, "_is_runtime_protocol", False):.

StaticProtocol is defined without @runtime_checkable, so it does not pass that if statement in check_protocol(). This means typeguard doesn't type check StaticProtocol and always returns None. (See python docs on the runtime_checkable decorator https://docs.python.org/3/library/typing.html#typing.runtime_checkable). I've added a warning to make sure that users are aware when their protocol is not being type checked, and I updated the test to check for that warning.

The main fix in the PR is the addition of check_protocol in the check_class function to handle cases where you have a type hint like Type[Protocol].

@williamlw999-fb

Copy link
Copy Markdown
Contributor Author

To be more specific, I've made sure that the the type checker can detect not only that the instanceFoo() is a RuntimeProtocol, but also that the class Foo is a Type[RuntimeProtocol].

There were no changes made to the how Typeguard handles StaticProtocol aside from warning the user that Python (and subsequently Typeguard) can't check protocols not decorated with @runtime_checkable at runtime.

@williamlw999-fb

Copy link
Copy Markdown
Contributor Author

Does this help clarify the PR?

@agronholm agronholm left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good, just one tiny nit.

Comment thread src/typeguard/_checkers.py Outdated
@agronholm
agronholm merged commit 17416ca into agronholm:master May 8, 2022
@agronholm

Copy link
Copy Markdown
Owner

Thanks!

@williamlw999-fb

Copy link
Copy Markdown
Contributor Author

Thanks for updating the lint and accepting the PR!

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.

4 participants