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

pathlib.Path objects can be used as context managers #83863

Closed
barneygale mannequin opened this issue Feb 19, 2020 · 16 comments
Closed

pathlib.Path objects can be used as context managers #83863

barneygale mannequin opened this issue Feb 19, 2020 · 16 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@barneygale
Copy link
Mannequin

barneygale mannequin commented Feb 19, 2020

BPO 39682
Nosy @brettcannon, @pitrou, @barneygale
PRs
  • bpo-39682: make pathlib.Path immutable by removing (undocumented) support for "closing" a path by using it as a context manager #18846
  • bpo-46556: emit DeprecationWarning from pathlib.Path.__enter__() #30971
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-04-01.14:11:09.381>
    created_at = <Date 2020-02-19.02:10:35.842>
    labels = ['type-bug', 'library', '3.9']
    title = 'pathlib.Path objects can be used as context managers'
    updated_at = <Date 2022-01-27.23:22:28.820>
    user = 'https://github.com/barneygale'

    bugs.python.org fields:

    activity = <Date 2022-01-27.23:22:28.820>
    actor = 'barneygale'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-04-01.14:11:09.381>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2020-02-19.02:10:35.842>
    creator = 'barneygale'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39682
    keywords = ['patch']
    message_count = 14.0
    messages = ['362244', '362840', '362934', '363206', '363209', '363218', '363260', '363261', '363285', '363298', '363383', '363468', '363469', '365473']
    nosy_count = 4.0
    nosy_names = ['brett.cannon', 'pitrou', 'Isaac Muse', 'barneygale']
    pr_nums = ['18846', '30971']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue39682'
    versions = ['Python 3.9']

    Linked PRs

    @barneygale
    Copy link
    Mannequin Author

    barneygale mannequin commented Feb 19, 2020

    pathlib.Path objects can be used as context managers, but this functionality is undocumented and makes little sense. Example:

    >>> import pathlib
    >>> root = pathlib.Path("/")
    >>> with root:
    ...     print(1)
    ... 
    1
    >>> with root:
    ...     print(2)
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/barney/.pyenv/versions/3.7.3/lib/python3.7/pathlib.py", line 1028, in __enter__
        self._raise_closed()
      File "/home/barney/.pyenv/versions/3.7.3/lib/python3.7/pathlib.py", line 1035, in _raise_closed
        raise ValueError("I/O operation on closed path")
    ValueError: I/O operation on closed path

    Path objects don't acquire any resources on new/init/enter, nor do they release any resources on exit. The whole notion of the path being _closed seems to exist purely to make impure Path methods unusable after exiting from the context manager. I can't personally think of a compelling use case for this, and suggest that it be removed.

    @barneygale barneygale mannequin added 3.8 only security fixes stdlib Python modules in the Lib dir labels Feb 19, 2020
    @brettcannon
    Copy link
    Member

    A use-case of "closing" a path is to cheaply flag that a path object is no longer valid, e.g. you deleted the underlying file and so you don't want others using the object anymore without paying the penalty of having to do the I/O to trigger the failure due to the path no longer existing.

    Now whether anyone is using this functionality I don't know, but removing it will inevitably break code for someone, so someone will need to make at least some form of an attempt to see if there's any use in the wild to justify simplifying the code by removing the functionality.

    @anntzer
    Copy link
    Mannequin

    anntzer mannequin commented Feb 28, 2020

    A problem of having this bit of state in paths is that it violates assumptions based on immutability/hashability, e.g. with

        from pathlib import Path
    
        p = Path("foo")
        q = Path("foo")
    
        with p: pass
    
        unique_paths = {p, q}
    
        print(len(unique_paths))  # == 1, as p == q
        for path in unique_paths:
            print(path.resolve())  # Fails if the path is closed.

    The last line fails with unique_paths = {p, q} as p has been closed (and apparently sets keep the first element when multiple "equal" elements are passed in), but not with unique_paths = {q, p}.

    It would also prevent optimizations based on immutability as proposed in https://bugs.python.org/issue39783.

    @brettcannon
    Copy link
    Member

    I guess a question is whether we want immutability guarantees (I for one didn't even know you could hash Path objects until now).

    I'm personally fine with that idea as it mentally makes sense to not need paths to be mutable. But as I said, someone needs to take at least some stab at seeing if any preexisting code out there will break if the _closed flag is removed before we can continue this discussion.

    @anntzer
    Copy link
    Mannequin

    anntzer mannequin commented Mar 2, 2020

    Immutability and hashability are listed first among "general properties" of paths (https://docs.python.org/3/library/pathlib.html#general-properties), and in the PEP proposing pathlib (https://www.python.org/dev/peps/pep-0428/#immutability). Looking at it again I *guess* the docs version could be read as only guaranteeing this for PurePaths (because that's in the PurePath section) rather than Path, but the PEP version clearly implies that this is true for both PurePaths and concrete Paths.

    It may be tricky to check usage in third-party packages, given that one would need to look for with <path-object>: ... rather than, say, a method name which can be grepped. Do you have any suggestions here?

    @pitrou
    Copy link
    Member

    pitrou commented Mar 2, 2020

    As with the Accessor abstraction, the original idea was to support Path objects backed by a directory file descriptor (for use with openat() and friends). That idea was abandoned but it looks like the context manager stayed. It's certainly not useful currently.

    @barneygale
    Copy link
    Mannequin Author

    barneygale mannequin commented Mar 3, 2020

    Can I ask what sort of backwards-compatibility guarantees Python provides for these cases? In the case of using a Path object as a context manager, I think we can say:

    • It's easy to do - there's no need to call any underscore-prefixed methods for example
    • It's undocumented
    • It's pretty hard to determine existing usage statically - grepping for `with p:` is years of work.

    @barneygale
    Copy link
    Mannequin Author

    barneygale mannequin commented Mar 3, 2020

    Also, thank you Antoine for your explanation :-)

    @brettcannon
    Copy link
    Member

    Can I ask what sort of backwards-compatibility guarantees Python provides for these cases?

    A deprecation warning for two releases (i.e. two years). Then it can be removed. So if people want to move forward with removing this then a DeprecationWarning would need to be raised in all places where _closed is set with a message stating the functionality is deprecated in 3.9 and slated for removal in 3.11 (along with appropriate doc updates, both for the module and What's New).

    @pitrou
    Copy link
    Member

    pitrou commented Mar 3, 2020

    Note you could simply remove the "closed" flag and the context manager live. That way, you don't have to deprecate anything.

    @brettcannon
    Copy link
    Member

    @antoine I just quite follow what you mean. Are you saying ditch _closed and just leave the context manager to be a no-op?

    @IsaacMuse
    Copy link
    Mannequin

    IsaacMuse mannequin commented Mar 6, 2020

    Brace expansion does not currently exist in Python's glob. You'd have to use a third party module to expand the braces and then run glob on each returned pattern, or use a third party module that implements a glob that does it for you.

    Shameless plug:

    Brace expansion: https://github.com/facelessuser/bracex

    Glob that does it for you (when the feature is enabled): https://github.com/facelessuser/wcmatch

    Now whether Python should integrate such behavior by default is another question.

    @IsaacMuse
    Copy link
    Mannequin

    IsaacMuse mannequin commented Mar 6, 2020

    Wrong thread sorry

    @pitrou
    Copy link
    Member

    pitrou commented Apr 1, 2020

    New changeset 00002e6 by Barney Gale in branch 'master':
    bpo-39682: make pathlib.Path immutable by removing (undocumented) support for "closing" a path by using it as a context manager (GH-18846)
    00002e6

    @pitrou pitrou closed this as completed Apr 1, 2020
    @pitrou pitrou added the type-bug An unexpected behavior, bug, or error label Apr 1, 2020
    @pitrou pitrou closed this as completed Apr 1, 2020
    @pitrou pitrou added type-bug An unexpected behavior, bug, or error 3.9 only security fixes and removed 3.8 only security fixes labels Apr 1, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    skshetry added a commit to skshetry/hydra that referenced this issue Oct 15, 2022
    Path as a contextmanager is deprecated from 3.11 onwards, and was a
    no-op since Python 3.11. Also this functionality was never documented.
    
    See [cpython/Lib/pathlib.py#L810-L816][1], python/cpython#83863 and, python/cpython#90714.
    
    [1]: https://github.com/python/cpython/blob/120b4ab2b68aebf96ce0de243eab89a25fc2d282/Lib/pathlib.py#L810-L816
    skshetry added a commit to skshetry/hydra that referenced this issue Oct 15, 2022
    Path as a contextmanager is deprecated from 3.11 onwards, and was a
    no-op since Python 3.9. Also this functionality was never documented.
    
    See [cpython/Lib/pathlib.py#L810-L816][1], python/cpython#83863 and, python/cpython#90714.
    
    [1]: https://github.com/python/cpython/blob/120b4ab2b68aebf96ce0de243eab89a25fc2d282/Lib/pathlib.py#L810-L816
    pixelb pushed a commit to facebookresearch/hydra that referenced this issue Oct 17, 2022
    Path as a contextmanager is deprecated from 3.11 onwards, and was a
    no-op since Python 3.9. Also this functionality was never documented.
    
    See [cpython/Lib/pathlib.py#L810-L816][1], python/cpython#83863 and, python/cpython#90714.
    
    [1]: https://github.com/python/cpython/blob/120b4ab2b68aebf96ce0de243eab89a25fc2d282/Lib/pathlib.py#L810-L816
    barneygale added a commit to barneygale/cpython that referenced this issue May 23, 2023
    …ext managers
    
    In Python 3.8 and prior, `pathlib.Path.__exit__()` marked a path as closed;
    some subsequent attempts to perform I/O would raise an IOError. This
    functionality was never documented, and had the effect of making `Path`
    objects mutable, contrary to PEP 428. In Python 3.9 we made `__exit__()` a
    no-op, and in 3.11 `__enter__()` began raising deprecation warnings. Here
    we remove both methods.
    barneygale added a commit that referenced this issue May 23, 2023
    …nagers (GH-104807)
    
    In Python 3.8 and prior, `pathlib.Path.__exit__()` marked a path as closed;
    some subsequent attempts to perform I/O would raise an IOError. This
    functionality was never documented, and had the effect of making `Path`
    objects mutable, contrary to PEP 428. In Python 3.9 we made `__exit__()` a
    no-op, and in 3.11 `__enter__()` began raising deprecation warnings. Here
    we remove both methods.
    @andrewshadura
    Copy link

    Oh well. I was actually using this functionality.
    It was super convenient to do something like:

    with Path('debian') / 'fill.copyright.blanks.yml' as debian_copyright_overrides:
        if debian_copyright_overrides.is_file():
            ...
        else:
            ...

    Basically it was a very visual way to structure code that worked with different subpaths. So no matter that it didn’t "close" path, it allowed to temporarily assign a shortcut to that path, do some work, and then forget about it.

    @barneygale
    Copy link
    Contributor

    Perhaps contextlib.nullcontext() would help?

    But context managers are all about acquiring and releasing resources. It could be considered misleading to use a context manager only for code formating purposes. I would write:

    debian_copyright_overrides = Path('debian') / 'fill.copyright.blanks.yml'
    if debian_copyright_overrides.is_file():
        ...
    else:
        ...

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants