From 6849a69aa3ce09b57881f9c8798985f30f563a7f Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 12 Apr 2023 14:44:37 +0200 Subject: [PATCH 1/3] Make methods in trio.Path visible to static tools, add tests for mypy seeing exported symbols, and tests for seeing class members. --- trio/_path.py | 77 +++++++++++++- trio/_path.pyi | 1 - trio/tests/test_exports.py | 206 +++++++++++++++++++++++++++++++++++-- 3 files changed, 271 insertions(+), 13 deletions(-) delete mode 100644 trio/_path.pyi diff --git a/trio/_path.py b/trio/_path.py index ea8cf98c34..254d965866 100644 --- a/trio/_path.py +++ b/trio/_path.py @@ -1,12 +1,12 @@ -# type: ignore - -from functools import wraps, partial import os -import types import pathlib +import sys +import types +from functools import partial, wraps +from typing import TYPE_CHECKING, Awaitable, Callable, TypeVar, Any import trio -from trio._util import async_wraps, Final +from trio._util import Final, async_wraps # re-wrap return value from methods that return new instances of pathlib.Path @@ -182,6 +182,73 @@ async def open(self, *args, **kwargs): value = await trio.to_thread.run_sync(func) return trio.wrap_file(value) + if TYPE_CHECKING: + # the dunders listed in _forward_magic that aren't seen otherwise + __bytes__ = pathlib.Path.__bytes__ + __truediv__ = pathlib.Path.__truediv__ + __rtruediv__ = pathlib.Path.__rtruediv__ + + # Note: these are not parsed properly by jedi. + # It might be superior to just manually implement all the methods and get rid + # of all the magic wrapping stuff. + + # wrapped methods handled by __getattr__ + absolute: Any + as_posix: Any + as_uri: Any + chmod: Any + cwd: Any + exists: Any + expanduser: Any + glob: Any + home: Any + is_absolute: Any + is_block_device: Any + is_char_device: Any + is_dir: Any + is_fifo: Any + is_file: Any + is_reserved: Any + is_socket: Any + is_symlink: Any + iterdir: Any + joinpath: Any + lchmod: Any + lstat: Any + match: Any + mkdir: Any + read_bytes: Any + read_text: Any + relative_to: Any + rename: Any + replace: Any + resolve: Any + rglob: Any + rmdir: Any + samefile: Any + stat: Any + symlink_to: Any + touch: Any + unlink: Any + with_name: Any + with_suffix: Any + write_bytes: Any + write_text: Any + + if sys.platform != "win32": + group: Any + is_mount: Any + owner: Any + + if sys.version_info >= (3, 8): + link_to: Any + if sys.version_info >= (3, 9): + is_relative_to: Any + with_stem: Any + readlink: Any + if sys.version_info >= (3, 10): + hardlink_to: Any + Path.iterdir.__doc__ = """ Like :meth:`pathlib.Path.iterdir`, but async. diff --git a/trio/_path.pyi b/trio/_path.pyi deleted file mode 100644 index 85a8e1f960..0000000000 --- a/trio/_path.pyi +++ /dev/null @@ -1 +0,0 @@ -class Path: ... diff --git a/trio/tests/test_exports.py b/trio/tests/test_exports.py index 026d6f5efa..e6da224bc0 100644 --- a/trio/tests/test_exports.py +++ b/trio/tests/test_exports.py @@ -1,17 +1,22 @@ +import enum +import importlib +import inspect import re +import socket as stdlib_socket import sys -import importlib import types -import inspect -import enum +from pathlib import Path +from types import ModuleType +from typing import Any, Iterable import pytest import trio import trio.testing +from trio.tests.conftest import RUN_SLOW -from .. import _core -from .. import _util +from .. import _core, _util +from .._core.tests.tutil import slow def test_core_is_properly_reexported(): @@ -65,12 +70,12 @@ def public_modules(module): reason="skip static introspection tools on Python dev/alpha releases", ) @pytest.mark.parametrize("modname", PUBLIC_MODULE_NAMES) -@pytest.mark.parametrize("tool", ["pylint", "jedi"]) +@pytest.mark.parametrize("tool", ["pylint", "jedi", "mypy"]) @pytest.mark.filterwarnings( # https://github.com/pypa/setuptools/issues/3274 "ignore:module 'sre_constants' is deprecated:DeprecationWarning", ) -def test_static_tool_sees_all_symbols(tool, modname): +def test_static_tool_sees_all_symbols(tool, modname, tmpdir): module = importlib.import_module(modname) def no_underscores(symbols): @@ -96,6 +101,30 @@ def no_underscores(symbols): script = jedi.Script(f"import {modname}; {modname}.") completions = script.complete() static_names = no_underscores(c.name for c in completions) + elif tool == "mypy": + if not RUN_SLOW: + pytest.skip("use --run-slow to check against mypy") + if sys.implementation.name != "cpython": + pytest.skip("mypy not installed in tests on pypy") + + # create py.typed file + (Path(trio.__file__).parent / "py.typed").write_text("") + + # mypy behaves strangely when passed a huge semicolon-separated line with `-c` + # so we use a tmpfile + tmpfile = tmpdir / "check_mypy.py" + tmpfile.write_text( + f"import {modname}\n" + + "".join(f"{modname}.{name}\n" for name in runtime_names), + encoding="utf8", + ) + from mypy.api import run + + res = run(["--config-file=", "--follow-imports=silent", str(tmpfile)]) + + # check that there were no errors (exit code 0), otherwise print the errors + assert res[2] == 0, res[0] + return else: # pragma: no cover assert False @@ -114,6 +143,169 @@ def no_underscores(symbols): assert False +# this could be sped up by only invoking mypy once per module, or even once for all +# modules, instead of once per class. +@slow +# see commend on test_static_tool_sees_all_symbols +@pytest.mark.redistributors_should_skip +# pylint/jedi often have trouble with alpha releases, where Python's internals +# are in flux, grammar may not have settled down, etc. +@pytest.mark.skipif( + sys.version_info.releaselevel == "alpha", + reason="skip static introspection tools on Python dev/alpha releases", +) +@pytest.mark.parametrize("module_name", PUBLIC_MODULE_NAMES) +@pytest.mark.parametrize("tool", ["jedi", "mypy"]) +def test_static_tool_sees_class_members(tool, module_name, tmpdir) -> None: + module = PUBLIC_MODULES[PUBLIC_MODULE_NAMES.index(module_name)] + + # ignore hidden, but not dunder, symbols + def no_hidden(symbols): + return { + symbol + for symbol in symbols + if (not symbol.startswith("_")) or symbol.startswith("__") + } + + if tool == "mypy": + if sys.implementation.name != "cpython": + pytest.skip("mypy not installed in tests on pypy") + # create py.typed file + (Path(trio.__file__).parent / "py.typed").write_text("") + + errors: dict[str, Any] = {} + if module_name == "trio.tests": + return + for class_name, class_ in module.__dict__.items(): + if not isinstance(class_, type): + continue + if module_name == "trio.socket" and class_name in dir(stdlib_socket): + continue + # Deprecated classes are exported with a leading underscore + if class_name.startswith("_"): # pragma: no cover + continue + + # dir() and inspect.getmembers doesn't display properties from the metaclass + # also ignore some dunder methods that tend to differ but are of no consequence + ignore_names = set(dir(type(class_))) | { + "__annotations__", + "__attrs_attrs__", + "__attrs_own_setattr__", + "__class_getitem__", + "__getstate__", + "__match_args__", + "__order__", + "__orig_bases__", + "__parameters__", + "__setstate__", + "__slots__", + "__weakref__", + } + + # pypy seems to have some additional dunders that differ + if sys.implementation.name == "pypy": + ignore_names |= { + "__basicsize__", + "__dictoffset__", + "__itemsize__", + "__sizeof__", + "__weakrefoffset__", + "__unicode__", + } + + # inspect.getmembers sees `name` and `value` in Enums, otherwise + # it behaves the same way as `dir` + # runtime_names = no_underscores(dir(class_)) + runtime_names = ( + no_hidden(x[0] for x in inspect.getmembers(class_)) - ignore_names + ) + + if tool == "jedi": + import jedi + + script = jedi.Script( + f"from {module_name} import {class_name}; {class_name}." + ) + completions = script.complete() + static_names = no_hidden(c.name for c in completions) - ignore_names + + missing = runtime_names - static_names + extra = static_names - runtime_names + if BaseException in class_.__mro__ and sys.version_info > (3, 11): + missing.remove("add_note") + + # TODO: why is this? Is it a problem? + if class_ == trio.StapledStream: + extra.remove("receive_stream") + extra.remove("send_stream") + + if missing | extra: + errors[f"{module_name}.{class_name}"] = { + "missing": missing, + "extra": extra, + } + elif tool == "mypy": + tmpfile = tmpdir / "check_mypy.py" + sorted_runtime_names = list(sorted(runtime_names)) + content = f"from {module_name} import {class_name}\n" + "".join( + f"{class_name}.{name}\n" for name in sorted_runtime_names + ) + tmpfile.write_text(content, encoding="utf8") + from mypy.api import run + + res = run( + [ + "--config-file=", + "--follow-imports=silent", + "--disable-error-code=operator", + str(tmpfile), + ] + ) + if res[2] != 0: + print(res) + it = iter(res[0].split("\n")[:-2]) + for output_line in it: + # hack to get around windows path starting with : + output_line = output_line[2:] + + _, line, error_type, message = output_line.split(":") + + # -2 due to lines being 1-indexed and to skip the import line + symbol = ( + f"{module_name}.{class_name}." + + sorted_runtime_names[int(line) - 2] + ) + + # The POSIX-only attributes get listed in `dir(trio.Path)` since + # they're in `dir(pathlib.Path)` on win32 cpython. This should *maybe* + # be fixed in the future, but for now we ignore it. + if ( + symbol + in ("trio.Path.group", "trio.Path.owner", "trio.Path.is_mount") + and sys.platform == "win32" + and sys.implementation.name == "cpython" + ): + continue + + # a bunch of symbols have this error, e.g. trio.lowlevel.Task.context + # but I don't think that's a problem - and if it is it would likely + # be picked up by "proper" mypy tests elsewhere + if "conflicts with class variable access" in message: + continue + + errors[symbol] = error_type + ":" + message + + else: # pragma: no cover + assert False + + if errors: # pragma: no cover + from pprint import pprint + + print(f"\n{tool} can't see the following symbols in {module_name}:") + pprint(errors) + assert not errors + + def test_classes_are_final(): for module in PUBLIC_MODULES: for name, class_ in module.__dict__.items(): From d01f42e87ea343de013620c20e991e46562bd658 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 5 May 2023 14:02:43 +0200 Subject: [PATCH 2/3] fixes after review feedback --- trio/_path.py | 16 ++++-- trio/tests/test_exports.py | 113 ++++++++++++++++++++++++------------- 2 files changed, 84 insertions(+), 45 deletions(-) diff --git a/trio/_path.py b/trio/_path.py index 254d965866..18405c59ce 100644 --- a/trio/_path.py +++ b/trio/_path.py @@ -156,11 +156,16 @@ class Path(metaclass=AsyncAutoWrapperType): def __init__(self, *args): self._wrapped = pathlib.Path(*args) - def __getattr__(self, name): - if name in self._forward: - value = getattr(self._wrapped, name) - return rewrap_path(value) - raise AttributeError(name) + # type checkers allow accessing any attributes on class instances with `__getattr__` + # so we hide it behind a type guard forcing it to rely on the hardcoded attribute + # list below. + if not TYPE_CHECKING: + + def __getattr__(self, name): + if name in self._forward: + value = getattr(self._wrapped, name) + return rewrap_path(value) + raise AttributeError(name) def __dir__(self): return super().__dir__() + self._forward @@ -188,7 +193,6 @@ async def open(self, *args, **kwargs): __truediv__ = pathlib.Path.__truediv__ __rtruediv__ = pathlib.Path.__rtruediv__ - # Note: these are not parsed properly by jedi. # It might be superior to just manually implement all the methods and get rid # of all the magic wrapping stuff. diff --git a/trio/tests/test_exports.py b/trio/tests/test_exports.py index e6da224bc0..74700220c4 100644 --- a/trio/tests/test_exports.py +++ b/trio/tests/test_exports.py @@ -108,7 +108,10 @@ def no_underscores(symbols): pytest.skip("mypy not installed in tests on pypy") # create py.typed file - (Path(trio.__file__).parent / "py.typed").write_text("") + py_typed_path = Path(trio.__file__).parent / "py.typed" + py_typed_exists = py_typed_path.exists() + if not py_typed_exists: + py_typed_path.write_text("") # mypy behaves strangely when passed a huge semicolon-separated line with `-c` # so we use a tmpfile @@ -122,6 +125,10 @@ def no_underscores(symbols): res = run(["--config-file=", "--follow-imports=silent", str(tmpfile)]) + # clean up created py.typed file + if not py_typed_exists: + py_typed_path.unlink() + # check that there were no errors (exit code 0), otherwise print the errors assert res[2] == 0, res[0] return @@ -146,7 +153,7 @@ def no_underscores(symbols): # this could be sped up by only invoking mypy once per module, or even once for all # modules, instead of once per class. @slow -# see commend on test_static_tool_sees_all_symbols +# see comment on test_static_tool_sees_all_symbols @pytest.mark.redistributors_should_skip # pylint/jedi often have trouble with alpha releases, where Python's internals # are in flux, grammar may not have settled down, etc. @@ -167,13 +174,19 @@ def no_hidden(symbols): if (not symbol.startswith("_")) or symbol.startswith("__") } + py_typed_path = Path(trio.__file__).parent / "py.typed" + py_typed_exists = py_typed_path.exists() + if tool == "mypy": if sys.implementation.name != "cpython": pytest.skip("mypy not installed in tests on pypy") # create py.typed file - (Path(trio.__file__).parent / "py.typed").write_text("") + # not marked with no-cover pragma, remove this logic when trio is marked + # with py.typed proper + if not py_typed_exists: + py_typed_path.write_text("") - errors: dict[str, Any] = {} + errors: dict[str, object] = {} if module_name == "trio.tests": return for class_name, class_ in module.__dict__.items(): @@ -182,6 +195,7 @@ def no_hidden(symbols): if module_name == "trio.socket" and class_name in dir(stdlib_socket): continue # Deprecated classes are exported with a leading underscore + # We don't care about errors in _MultiError as that's on its way out anyway if class_name.startswith("_"): # pragma: no cover continue @@ -239,7 +253,11 @@ def no_hidden(symbols): extra.remove("receive_stream") extra.remove("send_stream") - if missing | extra: + # intentionally hidden behind type guard + if class_ == trio.Path: + missing.remove("__getattr__") + + if missing or extra: errors[f"{module_name}.{class_name}"] = { "missing": missing, "extra": extra, @@ -258,46 +276,63 @@ def no_hidden(symbols): "--config-file=", "--follow-imports=silent", "--disable-error-code=operator", + "--soft-error-limit=-1", + "--no-error-summary", str(tmpfile), ] ) - if res[2] != 0: - print(res) - it = iter(res[0].split("\n")[:-2]) - for output_line in it: - # hack to get around windows path starting with : - output_line = output_line[2:] - - _, line, error_type, message = output_line.split(":") - - # -2 due to lines being 1-indexed and to skip the import line - symbol = ( - f"{module_name}.{class_name}." - + sorted_runtime_names[int(line) - 2] - ) - - # The POSIX-only attributes get listed in `dir(trio.Path)` since - # they're in `dir(pathlib.Path)` on win32 cpython. This should *maybe* - # be fixed in the future, but for now we ignore it. - if ( - symbol - in ("trio.Path.group", "trio.Path.owner", "trio.Path.is_mount") - and sys.platform == "win32" - and sys.implementation.name == "cpython" - ): - continue - - # a bunch of symbols have this error, e.g. trio.lowlevel.Task.context - # but I don't think that's a problem - and if it is it would likely - # be picked up by "proper" mypy tests elsewhere - if "conflicts with class variable access" in message: - continue - - errors[symbol] = error_type + ":" + message + # no errors + if res[2] == 0: + continue + + # get each line of output, containing an error for a symbol, + # stripping of trailing newline + it = iter(res[0].split("\n")[:-1]) + for output_line in it: + # split out the three last fields to not have problems with windows + # drives or other paths with any `:` + _, line, error_type, message = output_line.rsplit(":", 3) + + # -2 due to lines being 1-indexed and to skip the import line + symbol = ( + f"{module_name}.{class_name}." + sorted_runtime_names[int(line) - 2] + ) + + # The POSIX-only attributes get listed in `dir(trio.Path)` since + # they're in `dir(pathlib.Path)` on win32 cpython. This should *maybe* + # be fixed in the future, but for now we ignore it. + if ( + symbol + in ("trio.Path.group", "trio.Path.owner", "trio.Path.is_mount") + and sys.platform == "win32" + and sys.implementation.name == "cpython" + ): + continue + + # intentionally hidden from type checkers, lest they accept any attribute + if symbol == "trio.Path.__getattr__": + continue + + # a bunch of symbols have this error, e.g. trio.lowlevel.Task.context + # It's not a problem: it's just complaining we're accessing + # instance-only attributes on a class! + # See this test for a minimized version that causes this error: + # https://github.com/python/mypy/blob/c517b86b9ba7487e7758f187cf31478e7aeaad47/test-data/unit/check-slots.test#L515-L523. + + if "conflicts with class variable access" in message: + continue + + errors[symbol] = error_type + ":" + message else: # pragma: no cover - assert False + assert False, "unknown tool" + + # clean up created py.typed file + if tool == "mypy" and not py_typed_exists: + py_typed_path.unlink() + # `assert not errors` will not print the full content of errors, even with + # `--verbose`, so we manually print it if errors: # pragma: no cover from pprint import pprint From c6ac4603b241509ae248a6e3f612a3153abd3df5 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 16 May 2023 13:00:10 +0200 Subject: [PATCH 3/3] fix codecov --- trio/_path.py | 8 ++++++-- trio/tests/test_exports.py | 15 +++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/trio/_path.py b/trio/_path.py index 18405c59ce..7c338dbc97 100644 --- a/trio/_path.py +++ b/trio/_path.py @@ -193,8 +193,10 @@ async def open(self, *args, **kwargs): __truediv__ = pathlib.Path.__truediv__ __rtruediv__ = pathlib.Path.__rtruediv__ - # It might be superior to just manually implement all the methods and get rid - # of all the magic wrapping stuff. + # These should be fully typed, either manually or with some magic wrapper + # function that copies the type of pathlib.Path except sticking an async in + # front of all of them. The latter is unfortunately not trivial, see attempts in + # https://github.com/python-trio/trio/issues/2630 # wrapped methods handled by __getattr__ absolute: Any @@ -274,4 +276,6 @@ async def open(self, *args, **kwargs): # sense than inventing our own special docstring for this. del Path.absolute.__doc__ +# TODO: This is likely not supported by all the static tools out there, see discussion in +# https://github.com/python-trio/trio/pull/2631#discussion_r1185612528 os.PathLike.register(Path) diff --git a/trio/tests/test_exports.py b/trio/tests/test_exports.py index 74700220c4..8eb1131ee4 100644 --- a/trio/tests/test_exports.py +++ b/trio/tests/test_exports.py @@ -45,7 +45,7 @@ def public_modules(module): continue if not class_.__name__.startswith(module.__name__): # pragma: no cover continue - if class_ is module: + if class_ is module: # pragma: no cover continue # We should rename the trio.tests module (#274), but until then we use # a special-case hack: @@ -102,7 +102,7 @@ def no_underscores(symbols): completions = script.complete() static_names = no_underscores(c.name for c in completions) elif tool == "mypy": - if not RUN_SLOW: + if not RUN_SLOW: # pragma: no cover pytest.skip("use --run-slow to check against mypy") if sys.implementation.name != "cpython": pytest.skip("mypy not installed in tests on pypy") @@ -110,7 +110,7 @@ def no_underscores(symbols): # create py.typed file py_typed_path = Path(trio.__file__).parent / "py.typed" py_typed_exists = py_typed_path.exists() - if not py_typed_exists: + if not py_typed_exists: # pragma: no cover py_typed_path.write_text("") # mypy behaves strangely when passed a huge semicolon-separated line with `-c` @@ -126,7 +126,7 @@ def no_underscores(symbols): res = run(["--config-file=", "--follow-imports=silent", str(tmpfile)]) # clean up created py.typed file - if not py_typed_exists: + if not py_typed_exists: # pragma: no cover py_typed_path.unlink() # check that there were no errors (exit code 0), otherwise print the errors @@ -187,8 +187,6 @@ def no_hidden(symbols): py_typed_path.write_text("") errors: dict[str, object] = {} - if module_name == "trio.tests": - return for class_name, class_ in module.__dict__.items(): if not isinstance(class_, type): continue @@ -249,6 +247,7 @@ def no_hidden(symbols): missing.remove("add_note") # TODO: why is this? Is it a problem? + # see https://github.com/python-trio/trio/pull/2631#discussion_r1185615916 if class_ == trio.StapledStream: extra.remove("receive_stream") extra.remove("send_stream") @@ -257,7 +256,7 @@ def no_hidden(symbols): if class_ == trio.Path: missing.remove("__getattr__") - if missing or extra: + if missing or extra: # pragma: no cover errors[f"{module_name}.{class_name}"] = { "missing": missing, "extra": extra, @@ -322,7 +321,7 @@ def no_hidden(symbols): if "conflicts with class variable access" in message: continue - errors[symbol] = error_type + ":" + message + errors[symbol] = error_type + ":" + message # pragma: no cover else: # pragma: no cover assert False, "unknown tool"