bpo-36305: Fixes to path handling and parsing in pathlib #12361

kmaork · 2019-03-15T22:37:50Z

The bugs
This PR fixes three bugs with path parsing in pathlib.
The following examples show these bugs (when the cwd is C:\d):

WindowsPath('C:a').absolute() should return WindowsPath('C:\\d\\a') but returns WindowsPath('C:a').
This is caused by flawed logic in the parse_parts method of the _Flavour class.
WindowsPath('./b:a').absolute() should return WindowsPath('C:\\d\\b:a') but returns WindowsPath('b:a').
This is caused by the limited interface of parse_parts, and affects the Path.absolute, Path.expanduser and Path.__rtruediv__ methods.
WindowsPath('./b:a').resolve() should return WindowsPath('C:\\d\\b:a') but returns WindowsPath('b:a').
This is caused by missing logic in the resolve method and in Path.__str__

The fixes

To fix the first bug, I fixed a flaw in the parse_parts method.
The second one was more complicated, as with the current interface of parse_parts (called by _parse_args), the bug can't be fixed. Let's take a simple example: WindowsPath(WindowsPath('./a:b')) and WindowsPath('a:b') - before the bugfix, they are equal. That happens because in both cases, parse_parts is called with ['a:b']. This part can be interpreted in two ways - either as the relative path 'b' with the drive 'a:', or as a file 'a' with the NTFS data-stream 'b'.

That means that in some cases, passing the flattened _parts of a path to parse_parts is lossy. Therefore we have to a modify parse_parts's interface to enable passing more detailed information about the given parts. What I decided to do was allow passing tuples in addition to strings, thus supporting the old interface. The tuples would contain the drive, root and path parts, enough information to determine the correct parsing of path parts with data-streams, and maybe more future edge cases.

After modifying parse_parts's interface, I changed _parse_args to use it and made Path.absolute, Path.expanduser and Path.__rtruediv__ pass Path objects to _parse_args instead of path parts, to preserve the path information.
To solve the third bug I had to make small changes to both the resolve method and to Path.__str__.

Notes
In addition to the changes in the code, I've added regression tests and modified old incorrect tests.
Details about drive-relative paths can be found here.

https://bugs.python.org/issue36305

eryksun · 2019-03-16T07:49:10Z

Lib/pathlib.py

@@ -171,7 +171,7 @@ def splitroot(self, part, sep=sep):
                    else:
                        return part[:index2], sep, part[index2+1:]
        drv = root = ''
-        if second == ':' and first in self.drive_letters:
+        if second == ':' and first in self.drive_letters and third == sep:


The is not the solution. "C:" is a drive-relative path. Of course it has a drive. We need to fix absolute to properly support drive-relative paths. See the FIXME comment in the code. It's not necessarily as simple as calling ntpath._getfullpatname, as suggested in the comment, unless we give up on preserving ".." components in Windows. (That wouldn't be a problem really since preserving ".." components serves no point in Windows, unlike POSIX.)

You are right. I will modify the PR.

eryksun · 2019-03-16T07:55:01Z

WindowsPath('c:') / 'a' returns WindowsPath('c:a'), which is a bug.

This is not a bug. The resolved value of "c:" depends on its working directory, so joining it with "a" has to remain as "c:a". If we call absolute(), however, it should use the drive's current working directory to create a fully-qualified path.

eryksun · 2019-03-16T08:04:37Z

Lib/test/test_pathlib.py

+        # NTFS file-stream paths
+        check(['c:'],                   ('', '', ['c:']))
+        check(['c:a'],                  ('', '', ['c:a']))
+        check(['a', 'Z:b', 'c'],        ('', '', ['a', 'Z:b', 'c']))


Note that "c:" is not a file stream path, and neither is "c:a". The system interprets "[A-Za-z]:" as a drive. If we want to create a file stream named "a" in a file named "c" in the working directory, we have to use a qualified path such as "./c:a".

Which by the way is a separate bug in pathlib, if you want to look into it. For example, it's wrong for it to remove the explicit reference to the working directory in the following:

>>> p = Path('./c:a') >>> str(p) 'c:a'

We gave it the path of a file stream, but it mistakenly handles it as a drive-relative path when getting the path as a string.

... and yet,

>>> p = Path('c:a') >>> p WindowsPath('c:a') >>> Path('.') / p WindowsPath('c:a') >>> Path('./c:a') WindowsPath('c:a')

I don't think it's reasonable for Path('.') / Path('c:a') to give a different result than Path('./c:a'). I'm pretty sure I've seen it documented somewhere (although not in the official docs) that using Path('a/b/c') is a portable and shorter way of writing Path('a')/Path('b')/Path('c'), and the proposed behaviour violates that.

It does make sense, and does seem to have been the assumption in pathlib (seeing the implementation for parse_parts).

But this is a wrong assumption, as sometimes a path part can be interpreted in more than one way (for example, c:a that can either be a drive-relative path or a file with a stream). That means that separating a path to its parts may cause a loss of information about the original path and lead to incorrect results.

Assuming that this assumption can't be true, I think that the current behavior -
WindowsPath('a') / WindowsPath('./c:b') returning WindowsPath('c:b') is surely worse than
WindowsPath('a') / WindowsPath('./c:b') returning WindowsPath('a/c:b') (which would be the case if my fixes are applied)

csabella · 2020-01-16T12:08:51Z

Part of this was fixed with bpo-34775, causing the merge conflict.

pitrou · 2020-04-17T17:58:29Z

Hmm, I'm out of my depth here. @pfmoore @zooba Would you like to opine on the behaviour change in this PR? It's the first time I hear about NTFS data streams, myself...

@kmaork It's taken some time, but would you like to rebase / merge this PR with git master to have a fresh CI run?

kmaork · 2020-05-02T18:03:21Z

I think this PR has gotten stuck because the wanted behavior was not agreed upon. If we can settle it, someone (maybe me) will be able to implement it.
An possible way to resolve this is to say that pathlib should behave exactly like os.path in all cases. What do you think? @pitrou @pfmoore @zooba

barneygale · 2022-06-25T21:10:52Z

An possible way to resolve this is to say that pathlib should behave exactly like os.path in all cases. What do you think? @pitrou @pfmoore @zooba

Pathlib's internal representation of paths is more complex than a single string. This allows operations like .parent to be very fast, but it costs some flexibility in how path normalisation works. To make pathlib behave exactly like os.path, I think we'd need to perform serious surgery on pathlib's data model, which this PR doesn't do. I also reckon some of the original bugs have been fixed in recent versions.

barneygale

Once #101667 lands, you should only need to change _format_parsed_parts() to fix the original bug.

barneygale · 2023-02-17T23:51:38Z

Lib/test/test_pathlib.py

+        check(['a', 'Z:b', 'c'],        ('Z:', '', ['Z:', 'a', 'b', 'c']))
+        check(['Y:a', 'Z:b', 'c'],      ('Z:', '', ['Z:', 'a', 'b', 'c']))


This doesn't look right - if we switch drives mid-way through we should discard everything before. This is what os.path.join() does.

barneygale · 2023-02-17T23:52:58Z

Lib/test/test_pathlib.py

@@ -746,7 +756,9 @@ class PureWindowsPathTest(_BasePurePathTest, unittest.TestCase):

    equivalences = _BasePurePathTest.equivalences.copy()
    equivalences.update({
-        'c:a': [ ('c:', 'a'), ('c:', 'a/'), ('/', 'c:', 'a') ],
+        './a:b': [ ('./a:b',) ],
+        'a:b:c': [ ('./b:c', 'a:'), ('b:', 'a:b:c') ],


Similar to the above, I don't see why ('./b:c', 'a:') shouldn't resolve to a:.

barneygale · 2023-03-10T17:34:20Z

A refreshed version of this PR has been merged into main in #102454, so I'm going to close this PR. Thanks very much for working on this, @kmaork!

the-knights-who-say-ni added the CLA signed label Mar 15, 2019

bedevere-bot added the awaiting review label Mar 15, 2019

eryksun reviewed Mar 16, 2019

View reviewed changes

kmaork changed the title ~~bpo-36305: Fixed bug with colon-suffixed paths in pathlib~~ bpo-36305: Fixes to path handling and parsing in pathlib Mar 16, 2019

serhiy-storchaka mentioned this pull request May 28, 2019

Fix handling relative path in normalize_local_path_uri() on Windows. neuro-inc/neuro-cli#815

Merged

serhiy-storchaka self-requested a review May 28, 2019 14:43

Fixed bugs with drive-relative paths and NTFS ADS paths in pathlib

15aadf9

kmaork force-pushed the fix-issue-36305 branch from e254a4b to 15aadf9 Compare June 13, 2019 21:37

kmaork added 2 commits January 16, 2020 19:15

Merge branch 'master' into fix-issue-36305

c827f98

Merge branch 'master' into fix-issue-36305

64fc342

brettcannon added the topic-pathlib label Jun 21, 2022

kmaork mannequin mentioned this pull request Jun 21, 2022

Inconsistent behavior of pathlib.WindowsPath with drive paths #80486

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

barneygale requested changes Feb 17, 2023

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Feb 17, 2023

barneygale closed this Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-36305: Fixes to path handling and parsing in pathlib #12361

bpo-36305: Fixes to path handling and parsing in pathlib #12361

kmaork commented Mar 15, 2019 •

edited

Loading

eryksun Mar 16, 2019

kmaork Mar 16, 2019

eryksun commented Mar 16, 2019

eryksun Mar 16, 2019

eryksun Mar 16, 2019 •

edited

Loading

pfmoore Mar 23, 2019

kmaork Mar 23, 2019

csabella commented Jan 16, 2020 •

edited by bedevere-bot

Loading

pitrou commented Apr 17, 2020 •

edited

Loading

kmaork commented May 2, 2020 •

edited

Loading

barneygale commented Jun 25, 2022

barneygale left a comment •

edited

Loading

barneygale Feb 17, 2023

barneygale Feb 17, 2023

barneygale commented Mar 10, 2023

		check(['a', 'Z:b', 'c'], ('Z:', '', ['Z:', 'a', 'b', 'c']))
		check(['Y:a', 'Z:b', 'c'], ('Z:', '', ['Z:', 'a', 'b', 'c']))

bpo-36305: Fixes to path handling and parsing in pathlib #12361

bpo-36305: Fixes to path handling and parsing in pathlib #12361

Conversation

kmaork commented Mar 15, 2019 • edited Loading

eryksun Mar 16, 2019

Choose a reason for hiding this comment

kmaork Mar 16, 2019

Choose a reason for hiding this comment

eryksun commented Mar 16, 2019

eryksun Mar 16, 2019

Choose a reason for hiding this comment

eryksun Mar 16, 2019 • edited Loading

Choose a reason for hiding this comment

pfmoore Mar 23, 2019

Choose a reason for hiding this comment

kmaork Mar 23, 2019

Choose a reason for hiding this comment

csabella commented Jan 16, 2020 • edited by bedevere-bot Loading

pitrou commented Apr 17, 2020 • edited Loading

kmaork commented May 2, 2020 • edited Loading

barneygale commented Jun 25, 2022

barneygale left a comment • edited Loading

Choose a reason for hiding this comment

barneygale Feb 17, 2023

Choose a reason for hiding this comment

barneygale Feb 17, 2023

Choose a reason for hiding this comment

barneygale commented Mar 10, 2023

kmaork commented Mar 15, 2019 •

edited

Loading

eryksun Mar 16, 2019 •

edited

Loading

csabella commented Jan 16, 2020 •

edited by bedevere-bot

Loading

pitrou commented Apr 17, 2020 •

edited

Loading

kmaork commented May 2, 2020 •

edited

Loading

barneygale left a comment •

edited

Loading