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

Add type hints for @typechecked #96

Merged
merged 1 commit into from
Aug 19, 2020
Merged

Add type hints for @typechecked #96

merged 1 commit into from
Aug 19, 2020

Conversation

dbarnett
Copy link
Contributor

@dbarnett dbarnett commented Aug 3, 2020

Adds a py.typed marker file to indicate pytypes has type hints (see PEP 561), and adds type hints for @typechecked. These type hints are added in a separate pyi file to help with compatibility for python 3.3 & 3.4, since inline type annotations are a syntax error for those versions.

The fixes in typechecker.py were for issues flagged by the typechecker and shouldn't change the behavior of the code at all. They may not be strictly needed since type hint resolution order is supposed to check *.pyi files before their associated *.py files.
EDIT: Dropped the type checker fixes. They were more trouble than I expected and probably not needed.

Fixes #74.

pytypes/typechecker.py Outdated Show resolved Hide resolved
pytypes/typechecker.py Outdated Show resolved Hide resolved
@Stewori
Copy link
Owner

Stewori commented Aug 3, 2020

This breaks too many tests. Please try if reverting the lines I commented fixes this. One would think it cannot be caused by anything else as the asserts etc are not invasive. But then there was the recent incident with the blank line breaking a test...

@dbarnett dbarnett changed the title Add type hints for @typechecked and fix some pytype errors Add type hints for @typechecked Aug 3, 2020
@dbarnett
Copy link
Contributor Author

dbarnett commented Aug 3, 2020

This breaks too many tests. Please try if reverting the lines I commented fixes this. One would think it cannot be caused by anything else as the asserts etc are not invasive. But then there was the recent incident with the blank line breaking a test...

Yeah, you're right, sorry for the noise. I think I'm going to drop all the little fixes from this PR since any remaining improvements are pretty minor by themselves.

Besides that, I'm also updating the pyi file to be more complete and include types for no_type_check. I don't understand from the docs I've read what behavior things will give if something's defined in .py but not in .pyi. I worry that it'll start complaining "module 'pytypes.typechecker' has no attribute 'foo'" where it originally had more self-explanatory "No such module pytypes". I'm not sure if that's going to be a pain to keep up-to-date, but I suspect it won't matter much either way, if you're game to push to pypi and see if it's an improvement in practice.

@Stewori
Copy link
Owner

Stewori commented Aug 4, 2020

Would it work to make this a pytypes.pyi rather than a typechecker.pyi? pytypes imports all relevant bits and makes them available as pytypes.xxx, e.g. pytypes.typechecked. This would allow for a clean distinction:
everything directly available in pytypes would have type-annotations (ideally, one day) while other stuff is considered internal.
Also, this would allow to bundle all type info in one place, notably also for pytypes.is_subtype and pytypes.is_of_type.

Or does the static typechecker strictly require the pyi to target the location where the functions are actually implemented?

@dbarnett
Copy link
Contributor Author

dbarnett commented Aug 4, 2020

Yeah, that's fair, I agree that's probably the way to go. Let me look into that and update you when it's ready for another look.

@dbarnett dbarnett requested a review from Stewori August 4, 2020 17:46
@dbarnett dbarnett marked this pull request as draft August 4, 2020 17:46
@Stewori
Copy link
Owner

Stewori commented Aug 4, 2020

I have second thoughts on the pytypes.pyi idea. IIRC stubfiles are supposed to match the functions and classes as implemented in a module, not imported stuff. Stubfiles are really meant to augment code in static sense while imports and aliases are runtime stuff. So, probably it's best to proceed according to your initial proposal.

At first glance I thought this could solve the issue with annotating functions from type_util, e.g. is_subtype (alias for type_util._issubclass). Traditionally, type info for these functions causes infinite runtime typecheck recursion in profiler mode.
I guess the best solution would be to go from
from type_util import _issubclass as is_subtype
to

from type_util import _issubclass

def is_subtype(...): #with type annotation
    return _issubclass(...)

but anyway, that's beyond this PR. Just explaining my reasoning that led to the idea with pytypes.pyi.

pytypes/typechecker.pyi Outdated Show resolved Hide resolved
pytypes/typechecker.pyi Outdated Show resolved Hide resolved
pytypes/typechecker.pyi Show resolved Hide resolved
pytypes/typechecker.pyi Outdated Show resolved Hide resolved
@Stewori
Copy link
Owner

Stewori commented Aug 5, 2020

I suppose there is a way to let mypy validate whether the pyi file still fits to the corresponding py file. E.g. whether all functions and classes are present, and if the signatures match. If you have the right command at hand, I would like to note it down for pre-release checks to avoid a change in the py file was forgotten to be applied in the pyi.

@dbarnett
Copy link
Contributor Author

dbarnett commented Aug 6, 2020

I suppose there is a way to let mypy validate whether the pyi file still fits to the corresponding py file.

Looks like no per python/mypy#5028. What I'd done is used stubgen to create a skeleton pyi file and then modified it to add the desired type hints. I could probably formalize it with a quick script to generate and patch a fresh pyi file based on the latest .py file changes. There doesn't seem to be a good automated way to detect discrepancies, though.

@Stewori
Copy link
Owner

Stewori commented Aug 6, 2020

Isn't there a (simple) way to get mypy in case of an invalid stubfile produce these little "no such attribute" errors you mentioned?
I assume this would not catch every possible incompatibility but it would be a handy consistency check.

There doesn't seem to be a good automated way to detect discrepancies, though.

Maybe we should consider to add such kind of thing to pytypes. It's intended to host useful utilities for typing, actually.
I think stubfile_manager already does some portion of this. I think it would error if the pyi defines something that is not in the py file (including methods and other OOP constructs and nested classes which were especially painful to get right I remember). However, it does not check the "totality" aspect ATM. Also, I am not fully sure what could and should be done in terms of signature checking. I guess the result of inspect.get(full)argspec should be similar between every py and pyi defined function and method. Or is that too restrictive?

@dbarnett
Copy link
Contributor Author

dbarnett commented Aug 8, 2020

Isn't there a (simple) way to get mypy in case of an invalid stubfile produce these little "no such attribute" errors you mentioned?

The "no such attribute" errors I mentioned were from calling code referencing pytypes code that wasn't in the pyi. The unsolved problem is comparing the .py to the .pyi and noticing discrepancies like missing functions, wrong number of params, etc.

Maybe we should consider to add such kind of thing to pytypes. It's intended to host useful utilities for typing, actually.
I think stubfile_manager already does some portion of this.

Good idea! I filed issue #100 for this. I hadn't seen anything about stubfile_manager yet and can't find any usage notes on it. Sounds like I should look into what it can do.

@dbarnett dbarnett force-pushed the pytype branch 2 times, most recently from 96bf2cb to 1c67978 Compare August 8, 2020 19:17
@Stewori
Copy link
Owner

Stewori commented Aug 8, 2020

I hadn't seen anything about stubfile_manager yet and can't find any usage notes on it. Sounds like I should look into what it can do.

It's just internal and hardly documented. It locates stubfiles and attaches the types therein to the members of the original module. It is the motor of @annotations.
Unfortunately it predates PEP 561 and only locates stubfiles "naively" by the logic and heuristics that were common before PEP 561. It's a long-standing todo to implement PEP 561 here properly.

@dbarnett dbarnett marked this pull request as ready for review August 8, 2020 20:39
@dbarnett dbarnett requested a review from Stewori August 8, 2020 21:00
pytypes/typechecker.pyi Outdated Show resolved Hide resolved
pytypes/typechecker.pyi Outdated Show resolved Hide resolved
@dbarnett dbarnett force-pushed the pytype branch 2 times, most recently from 4805cdd to fb132eb Compare August 10, 2020 00:16
pytypes/typechecker.pyi Outdated Show resolved Hide resolved
pytypes/typechecker.pyi Outdated Show resolved Hide resolved
@Stewori
Copy link
Owner

Stewori commented Aug 11, 2020

Sorry to bother again. I already thought it could be merged, then the two cleanup comments occurred to me...

@dbarnett
Copy link
Contributor Author

K, should be 100% good to go now. Take a look.

@Stewori Stewori merged commit 95d58a3 into Stewori:master Aug 19, 2020
@Stewori
Copy link
Owner

Stewori commented Aug 19, 2020

Thanks for working this through!

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.

pytypes code is not completely annotated
2 participants