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

Make mypy test suite pass mypy type-check on Windows #11895

Closed
wants to merge 3 commits into from

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jan 3, 2022

Description

Mypy's test suite fails mypy on Windows. This PR fixes that. (This is my first non-docs contribution to mypy, so let me know if I'm doing anything wrong in this PR!)

Here is the error I get if I try running the mypy test suite on my Windows machine:

(venv) C:\Users\Alex\Desktop\Code dump\mypy>python runtests.py
run self: ['C:\\Users\\Alex\\Desktop\\Code dump\\mypy\\venv\\Scripts\\python.exe', '-m', 'mypy', '--config-file', 'mypy_self_check.ini', '-p', 'mypy']
mypy\test\data.py:41: error: Incompatible types in assignment
(expression has type overloaded function, variable has type overloaded
function)  [assignment]
            join = posixpath.join
                   ^
Found 1 error in 1 file (checked 163 source files)

FAILED: self

This error is due to the following lines in mypy/test/data.py:

mypy/mypy/test/data.py

Lines 38 to 41 in 2676cd8

if case.suite.native_sep:
join = os.path.join
else:
join = posixpath.join

On linux, os.path.join and posixpath.join have the same signature. However, on Windows, os.path.join is a reexport of ntpath.join, which has a different signature to posixpath.join due to the fact that the first parameter has a different name.

Here is how posixpath.join is defined in typeshed:

@overload
def join(a: StrPath, *paths: StrPath) -> str: ...
@overload
def join(a: BytesPath, *paths: BytesPath) -> bytes: ...

Here is how ntpath.join is defined in typeshed:

@overload
def join(path: StrPath, *paths: StrPath) -> str: ...
@overload
def join(path: BytesPath, *paths: BytesPath) -> bytes: ...

Test plan

Not sure. Guidance on this would be appreciated.

@AlexWaygood AlexWaygood changed the title Make mypy test suite pass mypy on Windows Make mypy test suite pass mypy type-check on Windows Jan 3, 2022
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 3, 2022

Excellent, CI errors in the one test I'm trying to fix. Not sure I really understand them.

@AlexWaygood
Copy link
Member Author

This PR isn't a good solution to this problem. I have an idea for a better solution to the problem, which would involve making changes to typeshed instead.

@AlexWaygood AlexWaygood closed this Jan 3, 2022
@AlexWaygood AlexWaygood deleted the fix-windows-typecheck branch January 3, 2022 22:36
@pranavrajpal
Copy link
Contributor

I think this PR is a good idea, but we can probably simplify the type of join. We only seem to use join with exactly 2 str arguments, so I think declaring the type of join as Callable[[str, str], str] and removing the protocol should make this simpler (and hopefully also fixes the CI errors).

@AlexWaygood AlexWaygood restored the fix-windows-typecheck branch January 4, 2022 08:24
@AlexWaygood AlexWaygood reopened this Jan 4, 2022
@AlexWaygood
Copy link
Member Author

I think this PR is a good idea, but we can probably simplify the type of join. We only seem to use join with exactly 2 str arguments, so I think declaring the type of join as Callable[[str, str], str] and removing the protocol should make this simpler (and hopefully also fixes the CI errors).

Worth a shot, let's see what happens :)

@AlexWaygood
Copy link
Member Author

I think the only way of creating a regression test for this kind of thing would be to run the self-check on Windows as well as Linux in the CI. Is that a change worth making?

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 4, 2022

This PR isn't a good solution to this problem. I have an idea for a better solution to the problem, which would involve making changes to typeshed instead.

FWIW, these are the changes to typeshed, which I think might be a better solution to this problem: python/typeshed#6812. (EDIT: These typeshed changes have now been merged, so the problem that this PR is solving will go away as soon as mypy's copy of typeshed is synced.)

@hauntsaninja
Copy link
Collaborator

Thanks for looking into this! I'd accept a PR that runs self check on Windows. Self check is pretty fast + hopefully helps keep contributor experience good for Windows users.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 5, 2022

Thanks for looking into this! I'd accept a PR that runs self check on Windows. Self check is pretty fast + hopefully helps keep contributor experience good for Windows users.

Great! I'll close this and open a new PR for that, then, as soon as typeshed is resynced :)

@AlexWaygood AlexWaygood closed this Jan 5, 2022
@AlexWaygood AlexWaygood deleted the fix-windows-typecheck branch January 5, 2022 07:54
AlexWaygood added a commit to AlexWaygood/mypy that referenced this pull request Jan 5, 2022
As discussed in python#11895, mypy's test suite currently does not pass a mypy self-check when run on Windows. This should hopefully be fixed by python/typeshed#6812, but running a self-check on Windows as part of the CI tests should help avoid this issue in the future.

This new CI test should hopefully fail until python#11905 is merged, confirming the diagnosis of the problem that I made in python#11895. Once python#11905 is merged, it should hopefully then pass, allowing this PR to be merged.
@AlexWaygood
Copy link
Member Author

Promised PR to run a Windows self-check is here: #11909 :)

Once #11905 is merged, I'll merge mypy/master into that branch, and the new test should hopefully pass, making the PR mergeable.

JelleZijlstra pushed a commit that referenced this pull request Jan 28, 2022
As discussed in #11895, mypy's test suite currently does not pass a mypy self-check when run on Windows. This should hopefully be fixed by python/typeshed#6812, but running a self-check on Windows as part of the CI tests should help avoid this issue in the future.
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