Skip to content

os.path.{abspath, basename, join} should accept _PathType #1960

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

Closed
wants to merge 2 commits into from

Conversation

sid-kap
Copy link
Contributor

@sid-kap sid-kap commented Mar 13, 2018

No description provided.

@sid-kap sid-kap changed the title abspath and basename should accept _PathType os.path.{abspath, basename, join} should accept _PathType Mar 13, 2018
@eric-wieser
Copy link
Contributor

eric-wieser commented Mar 13, 2018

I think this needs to become _PathType = AnyStr and _PathType = Union[_PathLike, AnyStr]

@sid-kap
Copy link
Contributor Author

sid-kap commented Mar 13, 2018

@eric-wieser What do you mean by that?
Also, how do I figure out which test is failing?

@JelleZijlstra
Copy link
Member

You have to click through a few times. The error is:

FAILURE  #115 check stdlibsamples (3.2)
glob.py:44: error: Incompatible types in "yield" (actual type "str", expected type "bytes")
SUMMARY  1/132 tasks and 1/132 tests failed

in mypy's selftest, which among others runs mypy on some sample code.

I haven't looked in detail, but the types here are complicated because we both have to support Paths and we have to make it so that if the argument is bytes, we return bytes (that's what the previous AnyStrs were for). @eric-wieser's suggestion (changing the way _PathType is defined) may work, but may also cause more problems.

@eric-wieser
Copy link
Contributor

eric-wieser commented Mar 13, 2018

I mean change

 if sys.version_info >= (3, 6):
     from builtins import _PathLike
-    _PathType = Union[bytes, Text, _PathLike]
+    _PathType = Union[AnyStr, _PathLike]
 else:
-    _PathType = Union[bytes, Text]
+    _PathType = AnyStr

or if that doesn't work

 if sys.version_info >= (3, 6):
     from builtins import _PathLike
-    _PathType = Union[bytes, Text, _PathLike[AnyStr]]
+    _PathType = Union[AnyStr, _PathLike[AnyStr]]
 else:
-    _PathType = Union[bytes, Text]
+    _PathType = AnyStr

@JelleZijlstra
Copy link
Member

That's very risky since we import and reuse os.path. _PathType in several other stubs.

@eric-wieser
Copy link
Contributor

I suspect this is the correct definition in all the other cases though, and if that causes failures there are probably mistakes in those stubs.

@JelleZijlstra
Copy link
Member

It's not, since your definition includes a TypeVar. It is correct only in cases where there are multiple types that should match (e.g., an argument and a return type). It is wrong when _PathType is used only as an argument type.

@eric-wieser
Copy link
Contributor

eric-wieser commented Mar 13, 2018

I don't understand the scoping rules of type vars yet.

Perhaps we want instead

 if sys.version_info >= (3, 6):
     from builtins import _PathLike
-    _PathType = Union[bytes, Text, _PathLike[AnyStr]]
+    class _PathType(Generic[AnyStr], Union[AnyStr, _PathLike[AnyStr]]): pass
 else:
-    _PathType = Union[bytes, Text]
+    class _PathType(Generic[AnyStr], AnyStr): pass

What I'm trying to spell is the C++

template<typename U, typename = enable_if_t<is_str<U>>>
using _PathType = Union<U, PathLike<U>>

but perhaps that's not supported

@eric-wieser
Copy link
Contributor

eric-wieser commented Mar 13, 2018

Ah, perhaps

 if sys.version_info >= (3, 6):
     from builtins import _PathLike
-    _PathType = Union[bytes, Text, _PathLike[AnyStr]]
+    T = TypeVar('T', str, bytes)
+    _PathType = Union[T, _PathLike[T]]  # use elsewhere as _PathType[AnyStr]
 else:
-    _PathType = Union[bytes, Text]
+    T = TypeVar('T', str, bytes)
+    _PathType = ???

It seems to be that T[AnyStr] should be AnyStr, rather than an error

@sid-kap
Copy link
Contributor Author

sid-kap commented Mar 13, 2018

@eric-wieser @JelleZijlstra I'm pretty new to mypy/typeshed so I don't know enough to decide which one seems correct. If you come to a conclusion about how to do it correctly, feel free to open up a new PR.

@eric-wieser
Copy link
Contributor

Opened python/typing#543 about how to write this

@JelleZijlstra
Copy link
Member

I reimplemented this in a way I thought should work (https://github.com/JelleZijlstra/typeshed/tree/ospathlike), but that unfortunately triggers python/mypy#3644. Let me try to think of some other way to handle it.

@JelleZijlstra
Copy link
Member

Oh actually, it works with overloads (JelleZijlstra@f4c10cb).

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Apr 14, 2018
Fixes python#1997.

This is tricky because we need to get the return values right (see python#1960 for
prior attempts) and we often run into python/mypy#3644. I found that I
could express most signatures correctly using a series of overloads.

A few other changes in here:
- Added splitunc, which according to https://docs.python.org/3/library/os.path.html
  should exist in both Unix and Windows.
- Made the second argument to os.path.curdir Optional to match the implementation.
- Fixed os.path.split, whose previous Path-aware signature triggered python/mypy#3644.

I used the following test program to make sure mypy accepted the signatures used here:
@FichteFoll
Copy link
Contributor

See also #1841 for a previous attempt.

JelleZijlstra added a commit that referenced this pull request May 15, 2018
Fixes #1997, #2068.

This is tricky because we need to get the return values right (see #1960 for
prior attempts) and we often run into python/mypy#3644. I found that I
could express most signatures correctly using a series of overloads.

A few other changes in here:
- Added splitunc, which according to https://docs.python.org/3/library/os.path.html
  should exist in both Unix and Windows.
- Made the second argument to os.path.curdir Optional to match the implementation.
- Fixed os.path.split, whose previous Path-aware signature triggered python/mypy#3644.
@JelleZijlstra
Copy link
Member

Superseded by #2053.

gwk pushed a commit to gwk/typeshed that referenced this pull request May 29, 2018
Fixes python#1997, python#2068.

This is tricky because we need to get the return values right (see python#1960 for
prior attempts) and we often run into python/mypy#3644. I found that I
could express most signatures correctly using a series of overloads.

A few other changes in here:
- Added splitunc, which according to https://docs.python.org/3/library/os.path.html
  should exist in both Unix and Windows.
- Made the second argument to os.path.curdir Optional to match the implementation.
- Fixed os.path.split, whose previous Path-aware signature triggered python/mypy#3644.
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
Fixes python#1997, python#2068.

This is tricky because we need to get the return values right (see python#1960 for
prior attempts) and we often run into python/mypy#3644. I found that I
could express most signatures correctly using a series of overloads.

A few other changes in here:
- Added splitunc, which according to https://docs.python.org/3/library/os.path.html
  should exist in both Unix and Windows.
- Made the second argument to os.path.curdir Optional to match the implementation.
- Fixed os.path.split, whose previous Path-aware signature triggered python/mypy#3644.
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.

4 participants