`Path` refactor #2959

etianen · 2024-02-17T16:09:42Z

This PR implements the proposed refactor in #2944. As well as fixing the Python 3.13 problems, it adds some cool new things! 😎

The big change 💥

trio.Path now subclasses pathlib.PurePath!

This means all the sync method forwarding can be completely removed!

Since it now fits into the pathlib class hierarchy, it can be used interchangeably with other pure paths and methods that take pure paths. This also has the nice effect of making the wrapped async methods just work with regard to input types.

It's also now a non-virtual subclass of os.PathLike[str], which might help some static analysis tools...?

Two concrete subclasses PosixPath and WindowsPath also now exist, matching how pathlib does things. Instantiating a Path actually gets you the appropriate one for the platform, just like pathlib.

The medium change

Async method wrapping is now explicit, fixing Python 3.13. There are three high-level wrapper helpers that work for most cases (_wrap_method, _wrap_method_path and _wrap_method_path_iterable). For methods that don't fit into these, a low-level (_wraps_async) is used.

Other changes

Path is now slotted, to match pathlib. A minor speed boost, with luck!

Fixing Python 3.13

Unlike #2955, this fix actually works! Check it out:

================================================= test session starts ==================================================
platform darwin -- Python 3.13.0a3+, pytest-8.0.1, pluggy-1.4.0
rootdir: /Users/dave.hall/Workspace/trio
configfile: pyproject.toml
collected 33 items                                                                                                     

src/trio/_tests/test_path.py .................................                                                   [100%]

================================================== 33 passed in 0.05s ==================================================

My battle with `pyright --verifytypes` 😭

The pyright --verifytypes check does not like this syntax:

stat = _wrap_method(pathlib.Path.stat)

It claims this is an ambiguous type, asking me to annotate it with something that can't be expressed with the Python type system except for a per-method unweildy protocol!

The workaround would be to put in a dummy method and use a decorator syntax, which pyright thinks is absolutely okay despite inferring to exactly the same thing! It's also pretty disgusting to look at, and bloats the implementation with boilerplate.

@_wrap_method(pathlib.Path.stat)
def stat(self) -> Any:
    raise AssertionError("unreachable!")

I can't see any way to disable this check for just this file, or just these lines. Any suggestions?! 😭

codecov · 2024-02-17T16:11:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (379d99b) to head (ba5b6c7).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2959      +/-   ##
==========================================
- Coverage   99.64%   99.64%   -0.01%     
==========================================
  Files         117      117              
  Lines       17643    17594      -49     
  Branches     3176     3171       -5     
==========================================
- Hits        17581    17532      -49     
  Misses         43       43              
  Partials       19       19

Files	Coverage Δ
src/trio/__init__.py	`100.00% <100.00%> (ø)`
src/trio/_path.py	`100.00% <100.00%> (ø)`
src/trio/_tests/test_exports.py	`99.61% <100.00%> (+<0.01%)`	⬆️
src/trio/_tests/test_path.py	`100.00% <100.00%> (ø)`

etianen · 2024-02-17T16:17:54Z

src/trio/_tests/test_path.py

-
-    assert not hasattr(MockWrapper, "_private")
-
-


All of this can be removed, now we're not using metaclasses.

etianen · 2024-02-17T16:18:43Z

src/trio/_tests/type_tests/path.py

@@ -39,7 +39,7 @@ def sync_attrs(path: trio.Path) -> None:
    assert_type(path.drive, str)
    assert_type(path.root, str)
    assert_type(path.anchor, str)
-    assert_type(path.parents[3], pathlib.Path)


I count this as a bugfix!

src/trio/_tests/test_exports.py

CoolCat467

Questions about a few minor things

src/trio/_path.py

jakkdl · 2024-02-18T10:07:12Z

The pyright --verifytypes check does not like this syntax:
stat = _wrap_method(pathlib.Path.stat)
It claims this is an ambiguous type, asking me to annotate it with something that can't be expressed with the Python type system except for a per-method unweildy protocol!
The workaround would be to put in a dummy method and use a decorator syntax, which pyright thinks is absolutely okay despite inferring to exactly the same thing! It's also pretty disgusting to look at, and bloats the implementation with boilerplate.

The error message is specifically about it being ambigious and could be inferred differently by other type checkers, so whether it's being inferred to the same thing by pyright is not really relevant. In most other cases we've resorted to doing the disgusting/bloated boilerplate to get around these errors, but I'll have a closer look later on whether I think it's worth it. We are after all running type checks on both mypy & pyright, so we can probably ignore it.

I can't see any way to disable this check for just this file, or just these lines. Any suggestions?! 😭

Maybe I should resuscitate #2910 and get the output-filtering script finished/merged.

etianen · 2024-02-18T11:55:05Z

The error message is specifically about it being ambiguous and could be inferred differently by other type checkers

I think I'm disagreeing here with pyright. I can write the same thing using a decorator syntax, which provides no additional typing information, and it's happy. It just seems to be complaining that I'm doing a variable assignment and not explicitly annotating the variable.

This is related to my point about the static visibility linter, really. This check seems to have a very uncompromising idea of what's ambiguous. The use of ParamSpec and Concatenate here has, AFAIK, absolutely only one possible meaning. That's not ambiguous! It's simply not explicit.

I'm a huge fan of static analysis tools in general, and favor very strict typing and linting. But to me, these checks feel overboard. I realize that this exactly the argument people make to me about strict mypy checks, so maybe it's a case of comfort levels? But at the point inheritance and strictly-typed metaprograming become forbidden, it feels too much to me.

I'm left in a bit of a quandry for this PR though. Hopefully you have a solution in mind with #2910.

etianen · 2024-02-18T12:13:02Z

Also, apologies if the tone of my messages comes over as overly critical! I'm a big fan of how welcoming this project is to new contributors, so this stems from a concern about placing too-high barriers in front of new contributions.

It's hard to express frustration without sounding... frustrated. 🥵

jakkdl · 2024-02-18T17:07:17Z

disagreeing with pyright seems very fair in this case, and given how complex the Path wrapping is already I'm open to just ignoring what --verifytypes says. I don't know for sure why it forbids assignment here.
Will look at stuff tomorrow :)

etianen · 2024-02-18T17:30:48Z

Thanks for the feedback! I've implemented the suggested changes. The new docs in particular look very nice! 💪

I'll await your findings on the --verifytypes problem. I couldn't find a way to selectively ignore this file, so hopefully you'll have a better time of it than me! 😅

CoolCat467 · 2024-02-18T18:10:17Z

One slightly interesting point https://trio--2959.org.readthedocs.build/en/2959/reference-io.html#trio.Path.link_to

etianen · 2024-02-18T19:19:18Z

One slightly interesting point https://trio--2959.org.readthedocs.build/en/2959/reference-io.html#trio.Path.link_to

I'm assuming these docs were built not on Python 3.12? The method is indeed gone in 3.12.

etianen · 2024-02-20T16:02:36Z

I've just had another look at this. Unfortunately, verifytypes does not respect:

exclude
ignore
defineConstant

The only solution I can see is the solution in #2910. Do we keep this open and wait for that to merge, or do we temporarily disable the check?

A5rocks

I forgot: could you add a newsfragment?

Also a couple comments, ranging from nitpicks to asking for a test or two

src/trio/_path.py

A5rocks · 2024-02-21T02:42:44Z

src/trio/_path.py

-os.PathLike.register(Path)
+    ) -> AsyncIOWrapper[IO[Any]]: ...
+
+    @_wraps_async(pathlib.Path.open)  # type: ignore[misc]  # Overload return mismatch.


If this is what I think it is (mismatch because we don't have overloads for "r" vs "w" vs whatever), then IMO this should have a TODO. I realize you didn't write this type ignore and just copied it over, but yeah.

This is probably actually because we return an AsyncIOWrapper, not the sync file.

It's neither! It's Type of decorated function contains type "Any" ("Callable[[Path, str, int, Optional[str], Optional[str], Optional[str]], Coroutine[Any, Any, AsyncIOWrapper[IO[Any]]]]")

For some reason, using any in a decorated function is naughty. It's not naughty if the function isn't decorated though. 🤷

src/trio/_tests/test_path.py

src/trio/_tests/type_tests/path.py

A5rocks · 2024-02-21T05:20:19Z

The only solution I can see is the solution in #2910. Do we keep this open and wait for that to merge, or do we temporarily disable the check?

I'm sure specifically filtering pyright errors should be a pretty simple thing to do; let's wait a few days and see if that happens. If it doesn't, then temporarily disabling the check sounds good. (I'm hoping we can get a release out in ~a week, so that this and a bunch of changes can go out)

TeamSpen210 · 2024-02-21T06:15:41Z

Perhaps we should open an issue for Pyright. Thinking about it, it seems like a bug that verifytypes doesn't pick up on inference being required for the decorator, that's inconsistent.

…implementation detail

etianen · 2024-02-21T20:01:04Z

Hopefully that's all points addressed! Some directly, others laterally.

A5rocks

Looks good, I like that this decreases the total amount of code!

(hopefully someone else can also give this a lookover because my passes might have missed something)

CoolCat467

Looks pretty good other than one thing I noticed

src/trio/_tests/type_tests/path.py

CoolCat467

Very nice!

A5rocks · 2024-03-11T04:30:29Z

pyright changes are merged, you should be able to python src/trio/_tests/check_type_completeness.py --overwrite-file (or something like that) and that should be it! i tried doing that but for some reason github codespaces doesn't recognize that i can push to the branch and i dont want to bother setting this up locally.

jakkdl · 2024-03-11T14:50:04Z

I added some logic to ignore everything instead of listing all the errors in the json. I feel like has_docstring_at_runtime should've been able to pick up the docstrings, but I haven't bothered figuring out why it doesn't.

etianen · 2024-03-11T21:36:52Z

It's in! Thanks all!

jakkdl · 2024-03-13T10:28:39Z

Thank you! And sorry for the delay with finishing up #2910 :)

A5rocks · 2024-03-17T03:01:51Z

fyi this went out in 0.25.0!

etianen force-pushed the dh/path-refactor branch from d540052 to 80229df Compare February 17, 2024 16:10

etianen force-pushed the dh/path-refactor branch from 80229df to 0a78304 Compare February 17, 2024 16:16

etianen commented Feb 17, 2024

View reviewed changes

etianen force-pushed the dh/path-refactor branch 4 times, most recently from 5776eaa to 0dbda03 Compare February 17, 2024 16:45

etianen commented Feb 17, 2024

View reviewed changes

src/trio/_tests/test_exports.py Show resolved Hide resolved

etianen force-pushed the dh/path-refactor branch 6 times, most recently from cb177d5 to c12689a Compare February 17, 2024 17:32

Path refactor

91bcaa6

etianen force-pushed the dh/path-refactor branch from c12689a to 91bcaa6 Compare February 17, 2024 17:37

etianen marked this pull request as ready for review February 17, 2024 17:45

etianen mentioned this pull request Feb 17, 2024

[Heads up] Test failures with Python 3.13.0a3 #2944

Closed

CoolCat467 reviewed Feb 17, 2024

View reviewed changes

src/trio/_path.py Outdated Show resolved Hide resolved

src/trio/_path.py Outdated Show resolved Hide resolved

etianen added 2 commits February 18, 2024 17:16

Renaming _path.Self to _path.PathT

3a4d24f

Including original documentation in wrapped methods

00099dc

A5rocks requested changes Feb 21, 2024

View reviewed changes

etianen added 2 commits February 21, 2024 07:33

Tweaking docscring grammar for trio.Path

2f628db

Losing unnecessary pyright: ignore

7d407e5

jakkdl mentioned this pull request Feb 21, 2024

bump pyright, introducing warnings for tons of missing docstrings #2910

Merged

etianen added 2 commits February 21, 2024 19:27

Added newsfragment for new implementation

dad750a

Removing mentions of subclassing trio.Path, keeping this a private …

45f2012

…implementation detail

etianen force-pushed the dh/path-refactor branch from d73c26a to 8b47648 Compare February 21, 2024 19:51

Added OS-specific tests for trio.Path

819ea33

etianen force-pushed the dh/path-refactor branch from 8b47648 to 819ea33 Compare February 21, 2024 19:52

A5rocks approved these changes Feb 21, 2024

View reviewed changes

CoolCat467 requested changes Feb 22, 2024

View reviewed changes

src/trio/_tests/type_tests/path.py Outdated Show resolved Hide resolved

Importing AsyncIOWrapper from trio._file_io in Path tests

dadd176

etianen requested a review from CoolCat467 February 22, 2024 22:43

CoolCat467 approved these changes Feb 23, 2024

View reviewed changes

etianen and others added 2 commits March 5, 2024 17:08

Merge branch 'master' into dh/path-refactor

cdc5953

Merge branch 'master' into dh/path-refactor

7ad7763

jakkdl added 2 commits March 11, 2024 15:47

ignore all current trio._path.Path errors

b512a40

Merge remote-tracking branch 'origin/master' into dh/path-refactor

ba5b6c7

etianen merged commit f890f8f into python-trio:master Mar 11, 2024
28 checks passed

etianen deleted the dh/path-refactor branch March 11, 2024 21:35

hroncok mentioned this pull request May 24, 2024

Remaining test issues with Python 3.13.0b1: not ki_protected, pathlib.Path.resolve siganture #3004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Path` refactor #2959

`Path` refactor #2959

etianen commented Feb 17, 2024 •

edited

Loading

codecov bot commented Feb 17, 2024 •

edited

Loading

etianen Feb 17, 2024

etianen Feb 17, 2024

CoolCat467 left a comment

jakkdl commented Feb 18, 2024

etianen commented Feb 18, 2024

etianen commented Feb 18, 2024

jakkdl commented Feb 18, 2024

etianen commented Feb 18, 2024

CoolCat467 commented Feb 18, 2024

etianen commented Feb 18, 2024

etianen commented Feb 20, 2024

A5rocks left a comment

A5rocks Feb 21, 2024

TeamSpen210 Feb 21, 2024

etianen Feb 21, 2024

A5rocks commented Feb 21, 2024

TeamSpen210 commented Feb 21, 2024

etianen commented Feb 21, 2024

A5rocks left a comment •

edited

Loading

CoolCat467 left a comment

CoolCat467 left a comment

A5rocks commented Mar 11, 2024 •

edited

Loading

jakkdl commented Mar 11, 2024

etianen commented Mar 11, 2024

jakkdl commented Mar 13, 2024

A5rocks commented Mar 17, 2024

Path refactor #2959

Path refactor #2959

Conversation

etianen commented Feb 17, 2024 • edited Loading

The big change 💥

The medium change

Other changes

Fixing Python 3.13

My battle with pyright --verifytypes 😭

codecov bot commented Feb 17, 2024 • edited Loading

Codecov Report

etianen Feb 17, 2024

Choose a reason for hiding this comment

etianen Feb 17, 2024

Choose a reason for hiding this comment

CoolCat467 left a comment

Choose a reason for hiding this comment

jakkdl commented Feb 18, 2024

etianen commented Feb 18, 2024

etianen commented Feb 18, 2024

jakkdl commented Feb 18, 2024

etianen commented Feb 18, 2024

CoolCat467 commented Feb 18, 2024

etianen commented Feb 18, 2024

etianen commented Feb 20, 2024

A5rocks left a comment

Choose a reason for hiding this comment

A5rocks Feb 21, 2024

Choose a reason for hiding this comment

TeamSpen210 Feb 21, 2024

Choose a reason for hiding this comment

etianen Feb 21, 2024

Choose a reason for hiding this comment

A5rocks commented Feb 21, 2024

TeamSpen210 commented Feb 21, 2024

etianen commented Feb 21, 2024

A5rocks left a comment • edited Loading

Choose a reason for hiding this comment

CoolCat467 left a comment

Choose a reason for hiding this comment

CoolCat467 left a comment

Choose a reason for hiding this comment

A5rocks commented Mar 11, 2024 • edited Loading

jakkdl commented Mar 11, 2024

etianen commented Mar 11, 2024

jakkdl commented Mar 13, 2024

A5rocks commented Mar 17, 2024

`Path` refactor #2959

`Path` refactor #2959

etianen commented Feb 17, 2024 •

edited

Loading

My battle with `pyright --verifytypes` 😭

codecov bot commented Feb 17, 2024 •

edited

Loading

A5rocks left a comment •

edited

Loading

A5rocks commented Mar 11, 2024 •

edited

Loading