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

GH-106037: Disarm os.PathLike foot-shotgun in pathlib.PurePath user subclasses #106043

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Jun 23, 2023

We made it possible to subclass pathlib.PurePath in a68e585, which landed in 3.12. However, user subclasses automatically inherit an __fspath__() method, which may not be appropriate. For example, a user subclass may implement a "virtual" filesystem to provide access to a .zip file or FTP server. But it would be highly surprising if open(FTPPath(...)) attempted to open a local file.

This patch makes the os.PathLike interface opt-in for pathlib.PurePath subclasses. In pathlib itself, we opt into the os.PathLike interface for PurePosixPath, PureWindowsPath and Path. As PurePath is not instantiable (you always get a PurePosixPath or PureWindowsPath object back), this is backwards compatible with 3.11.

…os.PathLike`

We made it possible to subclass `pathlib.PurePath` in a68e585, which landed
in 3.12. However, user subclasses automatically inherit an `__fspath__()`
method, which may not be appropriate. For example, a user subclass may
implement a "virtual" filesystem to provide access to a `.zip` file or FTP
server. But it would be highly surprising if `open(FTPPath(...))` attempted
to open a *local* file.

This patch makes the `os.PathLike` interface opt-in. In pathlib itself, we
opt into the `os.PathLike` interface for `PurePosixPath`, `PureWindowsPath`
and `Path`. As `PurePath` is not instantiable (you always get a
`PurePosixPath` or `PureWindowsPath` object back), this is backwards
compatible with 3.11, but not with earlier 3.12 betas.
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea. Are we sure it's backwards compatible to remove __fspath__ from PurePath? Looks like there are a few user subclasses of it in the wild: https://github.com/pytest-dev/pyfakefs/blob/b576d65bbd9a7508c90e6b00aa3b9eccf4a86fa1/pyfakefs/fake_pathlib.py#L796-L805

One possible alternative solution might be to implement #106046, and then just document that users should set __fspath__ to None if they don't want their path subclasses to be considered subtypes of os.PathLike...?

@barneygale
Copy link
Contributor Author

barneygale commented Jun 23, 2023

That would at least allow me to set __fspath__ = None in AbstractPath, which would lessen the impact significantly.

Looks like there are a few user subclasses of it in the wild

That example you gave was only recently updated for 3.12 beta 1. There shouldn't be many more out there, and as I consider this a bugfix, it's reasonable to change behaviour anyway I think

@barneygale
Copy link
Contributor Author

Alex, I've played around with just setting __fspath__ = None where I need, and it works nicely. I mean, the exception message is bogus, but that's still a simpler fix than this PR or the previous _BasePurePath PR. Thanks so much for the pointer. I'll close this but keep the bug open (it's technically valid, but I have a workaround for AbstractPath so less pressing)

@barneygale barneygale closed this Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants