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

Type annotations? #599

Closed
AustinScola opened this issue May 16, 2021 · 27 comments
Closed

Type annotations? #599

AustinScola opened this issue May 16, 2021 · 27 comments

Comments

@AustinScola
Copy link
Contributor

AustinScola commented May 16, 2021

I'm trying to use pyfakefs on a project that uses mypy for static type checking. It appears that pyfakefs does not have type annotations. I'm wondering if there are any plans for this? Also, I would be happy to help contribute!

@mrbean-bremen
Copy link
Member

I have been thinking about this after dropping support for 2.7, but didn't get to it yet.
If you want to have a go, please go ahead - we always appreciate contributions!

mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue May 23, 2021
github-actions bot pushed a commit that referenced this issue May 23, 2021
mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue Jul 5, 2021
- includes FakeFileOpen and all file wrapper classes
- see pytest-dev#599
mrbean-bremen added a commit that referenced this issue Jul 6, 2021
- includes FakeFileOpen and all file wrapper classes
- see #599
github-actions bot pushed a commit that referenced this issue Jul 6, 2021
@AustinScola
Copy link
Contributor Author

@mrbean-bremen looks like you have added more type annotations! At what point do you think that it makes sense to include a py.typed file?

@mrbean-bremen
Copy link
Member

If we puy that in, it would just check the annotated modules, correct? In this case we could do it now - I have to admit that I wasn't aware that this is needed for mypy.
I have annotated the fake_filesystem module, and certainly will add fake_filesystem_unittests as it contains part of the API. I'm not sure about the other modules which just implement the respective Python modules (pathlib, shutil, and os.scandir), and I will probably not touch the tests. Also I want to see if I can remove some of the type:ignore statements.
You are always welcome to contribute, of course :)

@AustinScola
Copy link
Contributor Author

If we puy that in, it would just check the annotated modules, correct?

Yes, I think it would treat anything that is not annotated as Any. The downside of this is that users might think that everything is typed, and then be surprised that some type errors are not caught. The upside though is that there is at least some level of safety for what currently is typed.

@mrbean-bremen
Copy link
Member

The downside of this is that users might think that everything is typed

Agreed. I think we will not publish a new release until the relevant modules are typed, so this will probably not be a problem. This may take a while, as I'm working on this only every now and then, but I think there is no rush.

@davidlbaird
Copy link
Collaborator

I imported pyfakefs 5.0.0 to our source base at Google, and had difficulties with the new typing annotations when running pytype (https://pypi.org/project/pytype/). In particular, I made the following local edit:

# BEGIN GOOGLE_EDIT

# Alias is not working with pytest.
# AnyFileWrapper = Union[
#     "FakeFileWrapper", "FakeDirWrapper",
#     "StandardStreamWrapper", "FakePipeWrapper"
# ]

AnyFileWrapper = Any

# END GOOGLE_EDIT

Have the changes been tested on pytype in addition to mypy? This is a critical issue for our code base, as we have required pytype for all of our new code since 2021.

I also noticed issues running pytype on the unit tests in the tests/ directory.

Thanks!

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Feb 8, 2023

Thanks - no, so far we haven't used pytype, though it could easily get integrated into the CI.
I have to admit that I have neglected typing - I got stuck on some problems with path-like objects which I couldn't get to behave as I wanted (including the mentioned AnyFileWrapper), and it is sitting on my To Do list since then...

I will have a look at pytype tonight or later this week and see if I can have another go. As always, any help is appreciated :)

@mrbean-bremen
Copy link
Member

I haven't used pytype before (unsurprisingly, given that I'm working under Windows, where it is not supported). I can confirm the errors (if running pytype in CI), mostly in tests and other untyped packages, which mypy does not check, but also in pyfakefs.helpers, which is typed.
I will have a closer look somewhere in the next days, probably not before the weekend.

And good to know that pyfakefs is still (or again?) used at Google - I had always wondered if that line in the Readme:

At last count, pyfakefs was used in over 2,000 Python tests at Google.

is still valid... I guess that you haven't used a current version until now, as the typing stuff is in there for some time. The 5.0.0 release was mostly related to the move to pytest-dev and the removal of the old (non-PEP8) API, but did not really have many changes.

@davidlbaird
Copy link
Collaborator

Thanks for taking a look at pytype. Yes, pyfakefs is still very much a part of Google's code base. "Over 2,000" is an accurate count. As Google's pyfakefs maintainer, I've had to do some interesting large scale code updates to keep up with pyfakefs changes, and only recently decided to surface this challenge and one I created another issue for. Google still standardizes on unittest rather than pytest, though we do support running pytest, especially for PyPi packages.

@davidlbaird
Copy link
Collaborator

By the way, I also really appreciate and give kudos for the continued support and added features of pyfakefs from the community, especially in keeping up with new Python versions and new features like type annotations!

@mrbean-bremen
Copy link
Member

Glad to hear that pyfakefs is still used, and that you continue to maintain it at Google! Interesting - by which changes in pyfakefs have these large scale code updates been triggered?

Regarding unittest vs pytest: Out of interest, I recently checked how pyfakefs is used in the repos depending on it (I checked only ones these that have at least 100 stars). Of these (54 repos), 24 use it with unittest, 23 with pytest, and the rest with Patcher or by manually patching the classes. So it is still a tie between unittest and pytest, though pytest is on the rise...

@davidlbaird
Copy link
Collaborator

Requiring unit tests based on unittest predates my joining Google 12 year ago. I'm still leading an effort to deprecate the pyfakefs PascalCase function names. Two thousand unit test files is a low figure.

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Feb 9, 2023

Requiring unit tests based on unittest predates my joining Google 12 year ago

I guess that was in large part due to Mike Bland and Co.

I'm still leading an effort to deprecate the pyfakefs PascalCase function names.

Ok... I removed the PascalCase in version 5 because I haven't seen it used anywhere, and it had been deprecated for almost 5 years now, so I thought that we are on the save side here. Alas...

Two thousand unit test files is a low figure.

It is relatively, and replacing the API is probably mostly search and replace, but it is still quite some work...

@mrbean-bremen
Copy link
Member

This may take a while...
My Linux installation on my notebook has been wracked (again, probably by a Windows update - sigh), so I now try to use WSL instead, though that seems to be quite slow.
And it looks like this is not an easy fix, I may have to rethink the typing or str/bytes/path-like objects.

@davidlbaird
Copy link
Collaborator

Thanks for looking deeper into the problem. I provided my workaround, though admittedly, it is just turning off type checking for particular signatures to allow pytest to pass. This helps our code base, so that we can apply pytest checks to our code. I saw some other functions that expect differing types for args, and I found that the @typing.overload decorator helps in those situations:.
https://docs.python.org/3.10/library/typing.html?highlight=overload#typing.overload.

@mrbean-bremen
Copy link
Member

Thank you - yes, I have been using overloads in some places. Maybe I have to add more of them, though I remember that I had tried this at the time and didn't get far. I will see what I can do, maybe I have to relax some checks a bit.

@mrbean-bremen
Copy link
Member

Just to let you know: I have been tinkering with this issue (e.g. the pytype support) for some time, and given up for now - both because I have real problems to run pytype on my local PC with better than abysmal performance (the splitting up of fake_filesystem took the run time down from 15 minutes to 5 minutes, which is still not workable), and because I have problems to consolidate pytype with mypy. The changes I made to satisfy pytype broke the mypy runs, and I just lack experience with pytype to go on on this pace.

We are happy to accept PRs that fix the problem, or add a workaround for pytypes (like the one mentioned above), if it does not hamper the mypy checks, or add any improvement to the typing, which is currently not ideal.

@davidlbaird
Copy link
Collaborator

Hi, thanks for trying to figure out the problem with the annotations. Could you please add the details to reproduce the error in pyfakefs at https://github.com/google/pytype/issues/new, so that the pytype owners can look at why the annotations won't work in both tools? Thanks.

@mrbean-bremen
Copy link
Member

Will do this, though I probably need to first boil this down to some reproducible case. This may actually help me to understand this better myself. I will try to do this over the weekend.

@mrbean-bremen
Copy link
Member

I'm still not sure if I just using this wrong. Here is boiled down example code from helpers.py:

import os
from typing import Union, Optional, AnyStr, overload, cast

AnyString = Union[str, bytes]
AnyPath = Union[AnyString, os.PathLike]

@overload
def make_string_path(dir_name: AnyStr) -> AnyStr:
    ...

@overload
def make_string_path(dir_name: os.PathLike) -> str:
    ...

# @overload
def make_string_path(dir_name: AnyPath) -> AnyString:
    return cast(AnyString, os.fspath(dir_name))

As it is, it works with mypy, but shows an error with pytype:

File "helpers.py", line 19, in make_string_path: bad option 'bytes' in return type [bad-return-type]
           Expected: str
  Actually returned: Union[bytes, str]

This can be fixed by commenting in the @overload line before the function implementation, but this is not legal in mypy. I'm quite sure that I'm just doing something wrong here, and I'm somewhat reluctant to write an issue against pytype, before I better understand the problem. Maybe someone with knowledge of pytype has an idea?

@davidlbaird
Copy link
Collaborator

davidlbaird commented Mar 20, 2023

My read of @typing.overload is that the function signatures will annotate args and return values, while the final non-decorated function declaration is not annotated.
https://docs.python.org/3/library/typing.html#typing.overload

@davidlbaird
Copy link
Collaborator

Also, I have not had to make use of typing.cast with pytype. It is telling the typing tool to consider the variable as a certain type, it does not change the type.

@mrbean-bremen
Copy link
Member

My read of @typing.overload is that the function signatures will annotate args and return values, while the final non-decorated function declaration is not annotated.

Agreed. I had just been playing around and noticed that setting that overload seems to satisfy pytype - maybe this is just a missing check in pytype. The real solution shall be something else.

Also, I have not had to make use of typing.cast with pytype. It is telling the typing tool to consider the variable as a certain type, it does not change the type.

Yes, I'm aware of this. I sometimes need this because the return type of a Python library function is incorrectly inferred.

Anyway, I will probably make a restart with this some time later - I think I got stuck in the details and lost the overall picture, and I may have to read up on typing.
Before I do that, I wanted to handle other issues like the currently broken compatibility with Python 3.12 - the typing issue is inconvenient, but it has a workaround for the time being.

@davidlbaird
Copy link
Collaborator

As an update to pyfakefs 5.2.2 with the improvements to handle pytype errors. I tested the changes on our internal code base. The behavior I see now are related to those projects that run pytype on their unit test modules results in error conditions like No attribute 'write' on pyfakefs.fake_file.FakeDirWrapper [attribute-error] and No attribute 'read' on pyfakefs.fake_file.FakeDirWrapper [attribute-error].

The workaround of replacing the AnyFileWrapper alias to AnyFileWrapper = Any in fake_file.py resolves these attribute errors.

@davidlbaird
Copy link
Collaborator

Another error found, but can be resolved with AnyFileWrapper = Any:

File "xxx.py", line 42, in create: Function Xxx.__init__ was called with the wrong arguments [wrong-arg-types]
         Expected: (self, session, open_function: Callable[..., IO] = ..., ...)
  Actually passed: (self, session, open_function: pyfakefs.fake_open.FakeFileOpen, ...)
  
  Method __call__ of protocol Callable[..., IO] has the wrong signature in pyfakefs.fake_open.FakeFileOpen:
  
  >> Callable[..., IO] expects:
  @abstractmethod
  def __call__(self: Callable[..., _RET], *args, **kwargs) -> _RET: ...
  
  >> pyfakefs.fake_open.FakeFileOpen defines:
  def __call__(self: pyfakefs.fake_open.FakeFileOpen, *args, **kwargs) -> Union[pyfakefs.fake_file.FakeDirWrapper, pyfakefs.fake_file.FakeFileWrapper, pyfakefs.fake_file.FakePipeWrapper, pyfakefs.fake_file.StandardStreamWrapper]: ...

@mrbean-bremen
Copy link
Member

Thanks - I will check this.

@mrbean-bremen
Copy link
Member

I'm going to close this now that the py.typed file is included, and type annotations with mypy working (which was the aim of this issue). There are some things that I'm not happy with, but we have to make some compromises anyway, and I would prefer to handle remaining problems in separate issues as they appear.

As for pytype support: If the current state (which has a lot of excluded files and suppressed warnings) is not sufficient, I also propose to handle this in separate issues instead of this catch-all issue. As far as I am aware, pyptype is mainly used by Google itself, so I would rely on @davidlbaird here as to what is really needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants