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 dmypy run on Windows #15429

Merged
merged 3 commits into from
Jun 14, 2023
Merged

Fix dmypy run on Windows #15429

merged 3 commits into from
Jun 14, 2023

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Jun 13, 2023

Fixes #10709

Two changes here:

  • Add equality methods to ErrorCode because they may appear in options snapshots
  • Use stable comparison for option snapshots, since Options.apply_changes() does some unrelated things with flags

Both are only relevant on Windows where options may roundtrip as: options -> snapshot -> pickle -> base64 -> unpickle -> apply_changes.

@github-actions

This comment has been minimized.

@Avasam
Copy link
Contributor

Avasam commented Jun 13, 2023

I pip install git+https://github.com/ilevkivskyi/mypy.git@fix-dmypy-run#egg=mypy into my typeshed environment, re-enabled --ignore-missing-imports, and it seems to be working correctly now.

The image below shows that missing imports are successfully ignored, and that mypy does show errors where it should
image

@ilevkivskyi
Copy link
Member Author

OK, great, thanks for confirming!

@Avasam
Copy link
Contributor

Avasam commented Jun 13, 2023

I'd like if someone else can confirm too, since my usage is going through a 3rd party extension. Just to make sure it wasn't a fluke.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 13, 2023

I can also repro the issue on a Windows machine, and can also confirm that this patch appears to fix it! (I'm running dmypy from the command line.)

Copy link
Collaborator

@svalentin svalentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it and seems to work.

before:

PS C:\src\mypy-play> python -m mypy.dmypy run .\test.py
Restarting: configuration changed
Daemon stopped
Daemon started
Response: {'restart': 'configuration changed', 'stdout': '', 'stderr': '', 'platform': 'win32', 'python_version': '3_11', 'roundtrip_time': 0.488344669342041}

after applying PR:

PS C:\src\mypy-play> python -m mypy.dmypy run .\test.py
Restarting: configuration changed
Daemon stopped
Daemon started
←[1m←[92mSuccess: no issues found in 42 source files←[0m
PS C:\src\mypy-play> python -m mypy.dmypy run .\test.py
←[1m←[92mSuccess: no issues found in 1 source file←[0m
PS C:\src\mypy-play>

options: Options,
status_file: str,
timeout: int | None = None,
options_snapshot: dict[str, object] | None = None,
Copy link
Collaborator

@svalentin svalentin Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accepted it since it work, but thinking out loud, it feels like it shouldn't be necessary to pass both options and options_snapshot. One or the other should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but then we will need something like "stable compare", which will be a tiny bit slower (this is btw the original way I did it). OTOH, then motivation is much more obvious, so let's go that way indeed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svalentin Could you please test again just in case?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Still works:

PS C:\src\mypy-play> python -m mypy.dmypy run .\test.py
Success: no issues found in 1 source file

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I think this is a much more elegant fix

@ilevkivskyi
Copy link
Member Author

@hauntsaninja It looks like mypy_primer is broken because AutoSplit fails to install. I tried re-running but it didn't help: https://github.com/python/mypy/actions/runs/5265892753/jobs/9519524776?pr=15429 Could you please take a look?

AlexWaygood added a commit to AlexWaygood/mypy_primer that referenced this pull request Jun 14, 2023
@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 14, 2023

@hauntsaninja It looks like mypy_primer is broken because AutoSplit fails to install. I tried re-running but it didn't help: https://github.com/python/mypy/actions/runs/5265892753/jobs/9519524776?pr=15429 Could you please take a look?

It looks like the issue is that mypy-primer can't install winsdk (maybe it's a Windows-only package?), which is specified as one of the packages needed for typechecking Autosplit. If that is indeed the issue, hauntsaninja/mypy_primer#89 should fix it.

winsdk was only added to the mypy-primer recipe for Autosplit yesterday, in hauntsaninja/mypy_primer#87

JelleZijlstra pushed a commit to hauntsaninja/mypy_primer that referenced this pull request Jun 14, 2023
@JelleZijlstra JelleZijlstra reopened this Jun 14, 2023
@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@ilevkivskyi ilevkivskyi merged commit cdddc50 into python:master Jun 14, 2023
@ilevkivskyi ilevkivskyi deleted the fix-dmypy-run branch June 14, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants