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

fix types in os.path #1363

Merged
merged 3 commits into from
May 31, 2017
Merged

fix types in os.path #1363

merged 3 commits into from
May 31, 2017

Conversation

matthiaskramm
Copy link
Contributor

In Python 2, doing

    os.path.join(u"foo", "bar")

is actually legal, and returns a unicode string.
Also os.path.relpath always returns the type of its first argument.

In Python 2, doing
    os.path.join(u"foo", "bar")
is actually legal, and returns a unicode string.
Also os.path.relpath always returns the type of its first argument.
@JelleZijlstra
Copy link
Member

Currently the os.path stubs are identical for Python 2 and 3, and I was planning to merge them after #1324 is merged. Could you apply the same change to the Python 3 path.pyi stub, perhaps behind a sys.version_info check?

@matthiaskramm
Copy link
Contributor Author

Happy to rebase on top of your refactoring. In the mean time, any clue what to do about

stdlib/2/os/path.pyi:59: error: Overloaded function signatures 1 and 2 overlap with incompatible return types

Should I add a type: ignore?

@gvanrossum
Copy link
Member

The error is because technically (bytes)->bytes overlaps with (Union[bytes, Text])->Text in the arguments but not in the result. I don't know that adding an ignore is going to fix this -- we'd have to test it with some code that actually calls this function with various combinations of args and reveals the return type. (This would be a good use of the proposed runtime tests (#1339).

@matthiaskramm
Copy link
Contributor Author

Good idea. I changed this to

@overload
def join(path: bytes, *paths: bytes) -> bytes: ...  # type: ignore
@overload
def join(path: _PathType, *paths: _PathType) -> Text: ...  # type: ignore

and ran the following code (which we can also add to the planned test suite).

import os
reveal_type(os.path.join("foo", "bar"))  # -> Any
reveal_type(os.path.join("foo", u"bar"))  # -> builtins.unicode
reveal_type(os.path.join(u"foo", "bar"))  # -> builtins.unicode
reveal_type(os.path.join(u"foo", u"bar"))  # -> builtins.unicode
reveal_type(os.path.join("foo", "bar", u"baz"))  # -> builtins.unicode

Seems mypy is happy enough, but the Any for the default case looks a bit concerning.

What's a good way forward with this?

  1. Keep the AnyStr. This seems to make mypy happy, but pytype is currently broken for us, since we actually do have code that calls os.path.join with a mixture of unicode and str.
  2. Change to @overload, with a # type: ignore. This has the disadvantage that mypy seems to assume Any for the default case.
  3. Change to def join(...) -> _PathType. This means that type checkers always have to consider a return type of unicode, even if the input is bytes.
  4. Change the return type to Any in all cases, with a similar argument as in pow (pow function return type overly broad #285). This is similar to 2., but at least we make all type checkers do the same thing.

@JelleZijlstra
Copy link
Member

What about changing the return type to always be Text? That should usually work, but maybe some code does rely on os.path.join returning str to type check.

I think the best solution is to go with your PR, then add a small mypy plugin that corrects the return type for os.path.join if it gets passed both bytes and unicode in py2 mode.

@gvanrossum
Copy link
Member

I wonder if we could do something like this? No ignores needed.

@overload
def join(path: bytes, *paths: bytes) -> bytes: ...
@overload
def join(path: Text, *paths: _PathType) -> Text: ...
@overload
def join(path: bytes, __p2: Text, *paths: _PathType) -> Text: ...
@overload
def join(path: bytes, __p2: bytes, __p3: Text, *paths: _PathType) -> Text: ...

It does the right thing for up to three arguments; it flags join(b"", b"", b"", u"") as an error, but perhaps that's better.

@matthiaskramm
Copy link
Contributor Author

Followed Guido's suggestion. Some grepping through our codebase indicated that we also need a __p4.

@JelleZijlstra: How do you want to merge this? Should I rebase onto your refactoring, or do you want to merge this right now?

@JelleZijlstra
Copy link
Member

I'm fine with merging this as is. I'll just have to put a few if sys.version_info checks when I merge path.pyi for 2 and 3.

@matthiaskramm
Copy link
Contributor Author

Ok, feel free to merge.

Looks like travis is broken though. (mypy can't find typing?)

@JelleZijlstra
Copy link
Member

The Travis issue should be fixed in master. (And one of the two Travis builds succeeded; not sure how they differ.)

@gvanrossum gvanrossum merged commit 6f07424 into master May 31, 2017
@gvanrossum gvanrossum deleted the os_path branch May 31, 2017 17:43
@gvanrossum
Copy link
Member

Fingers crossed. :-)

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.

3 participants