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

Remove pathlib.PurePath.__eq__ #10662

Merged
merged 4 commits into from
Sep 7, 2023

Conversation

hauntsaninja
Copy link
Collaborator

Fixes #10661

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator Author

I think the primer hits here are real issues

@github-actions

This comment has been minimized.

@hauntsaninja hauntsaninja marked this pull request as ready for review September 4, 2023 23:58
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
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.

It still feels like we're sorta working around shortcomings in the heuristics mypy/pyright use here. (As you said in the issue, this kind of thing is going to affect anybody who writes a __eq__ method in a py.typed runtime library, and __eq__ methods are pretty common!)

But I agree that this is a pretty useful error for mypy/pyright to emit, so I'm happy for this change to be made on the basis that practicality beats purity :-)

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator Author

Sounds good, I'll leave open for a bit in case anyone else chimes in!

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 5, 2023

Here is a small regression that this PR causes, FYI: currently mypy will not complain about this code (on my Windows machine, the expression evaluates to True):

from pathlib import *
PureWindowsPath("foo/bar") == Path("foo/bar")

But with this PR, it will:

$ mypy -c "from pathlib import *; PureWindowsPath('foo/bar') == Path('foo/bar')" --strict-equality --custom-typeshed-dir .
<string>:1: error: Non-overlapping equality check (left operand type: "PureWindowsPath", right operand type: "Path")  [comparison-overlap]
Found 1 error in 1 file (checked 1 source file)

Is the regression that important? Probably not particularly; I don't know how many people are doing this kind of comparison. But, worth noting, I think.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Sep 5, 2023

@AlexWaygood oh, interesting example! Definitely a false positive in terms of what the type checker thinks it's doing, but that code is pretty suspect and I'm okay with it triggering a warning of some kind — depending on your platform, it could be true that those could never be equal!

So I think I'm still fine with the PR (though I'll certainly add test case and comments). We should experiment with making Path.__new__ more accurate in a follow up. I'm trying to teach mypy about more kinds of __new__ over here: python/mypy#16020

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Diff from mypy_primer, showing the effect of this PR on open source code:

mkosi (https://github.com/systemd/mkosi)
+ mkosi/kmod.py:32:16: error: Non-overlapping container check (element type: "str", container item type: "Path")  [comparison-overlap]
+ mkosi/config.py:2012:8: error: Non-overlapping equality check (left operand type: "Optional[Path]", right operand type: "Literal['']")  [comparison-overlap]

@hauntsaninja hauntsaninja merged commit 4df9634 into python:main Sep 7, 2023
59 checks passed
@hauntsaninja hauntsaninja deleted the pathlib-strict-eq branch September 7, 2023 07:40
@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 15, 2023

This change caught a real bug in stubtest (that was introduced by, er, me)! https://github.com/python/mypy/actions/runs/6202155467/job/16840319289?pr=16121

The first CI run on python/mypy#16121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

comparison-overlap not triggered for pathlib.Path
3 participants