GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109413: Add a custom script for running mypy on libregrtest by AlexWaygood · Pull Request #109464 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Sep 15, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Currently, the only way to run mypy on Lib/test/libregrtest is to cd into Lib/test and then run mypy --config-file libregrtest/mypy.ini. This isn't ideal, as invoking mypy like this means that all things imported from test.support are inferred by mypy as being of type Any. That usually just means that you get some "false negatives" when it comes to type checking, but for us, it also means that we get a false-positive error:

libregrtest\single.py:57: error: Expected type in class pattern; found "Any"  [misc]
            case TestStats():
                 ^~~~~~~~~~~

This PR adds a custom script for invoking mypy on libregrtest, so that mypy can see the classes and functions inside Lib/test/support/ when it's run on libregrtest. A custom script is needed, as mypy gets very confused generally about the Lib/ directory -- it thinks the whole directory is "shadowing the stdlib". This means that mypy can only be invoked from very specific locations in the CPython repo, or it "sees" everything that's in the Lib/ directory, panics at all the things it doesn't understand, and refuses to do any type-checking at all.

The workaround this script uses is to copy Lib/test/support into a temporary directory and teach mypy about the existence of test.support (without revealing the existence of the rest of the Lib/ directory) using the MYPYPATH environment variable.

  • Issue: libregrtest: Improve type hints; explore running mypy in CI #109413

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AlexWaygood
Add a custom script for running mypy on libregrtest
770e39f
@AlexWaygood
AlexWaygood requested a review from vstinner September 15, 2023 19:02
@bedevere-app bedevere-app Bot added the awaiting review label Sep 15, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Sep 15, 2023
libregrtest: Improve type hints; explore running mypy in CI #109413
Closed
@AlexWaygood AlexWaygood added the skip news label Sep 15, 2023
AlexWaygood
AlexWaygood commented Sep 15, 2023
View reviewed changes
Comment thread Lib/test/support/__init__.py

@AlexWaygood AlexWaygood Sep 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Revealing the existence of this module to mypy necessitated fixing a few typing-related things to do with this module

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/support/__init__.py
Comment on lines +1329 to +1340
class WarningsPrinter:
def __init__(self, func: Callable[[str], None]) -> None:
self.func = func
# bpo-39983: Store the original sys.stderr at Python startup to be able to
# log warnings even if sys.stderr is captured temporarily by a test.
self.orig_stderr = sys.stderr

def __call__(self, msg: str) -> None:
return self.func(msg)


@WarningsPrinter

@AlexWaygood AlexWaygood Sep 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Mypy doesn't like assigning attributes to functions, unfortunately; it much prefers the idea of a custom callable class

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AlexWaygood AlexWaygood Sep 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

(This is the solution you said you preferred in #109382 (comment) :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/scripts/run_mypy_on_regrtest.py

@AlexWaygood AlexWaygood Sep 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@vstinner: where do you think this script should go?

  1. Where I have it now?
  2. Inside Lib/test/libregrtest?
  3. Somewhere else?

Also: thoughts on the name? It's a bit long and clunky right now...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 16, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would prefer to put it in Tools/build/. I would be fine with Lib/test/libregrtest.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@hauntsaninja

hauntsaninja commented Sep 16, 2023

Copy link
Copy Markdown
Contributor

(for what it's worth, I think we fixed the Expected type in class pattern; found "Any" diagnostic in python/mypy#14479 )
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented Sep 16, 2023

Copy link
Copy Markdown
Member Author

(for what it's worth, I think we fixed the Expected type in class pattern; found "Any" diagnostic in python/mypy#14479 )

Ah, thank you! I thought I remembered this coming up recently, but I couldn't find the reference when I searched for it.

Hmm, @vstinner, that makes me even less enthused about this approach. I know you said you'd prefer a custom script so that mypy would be able to see the types inside test.support, but to me, this script feels like a hack that's really awkward to invoke. Having everything from test.support being inferred as Any isn't ideal, but it's not the end of the world either, and the only false positive we'd be fixing here is going to go away in an upcoming mypy release anyway. I think the cure might be worse than the disease in this case.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Sep 16, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry @AlexWaygood, I'm not sure that I understood the alternative that you are proposing. No script? Can you maybe propose a PR implementating the alternative?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Tools/scripts/run_mypy_on_regrtest.py

@vstinner vstinner Sep 16, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would prefer to put it in Tools/build/. I would be fine with Lib/test/libregrtest.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AlexWaygood

AlexWaygood commented Sep 17, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Sorry @AlexWaygood, I'm not sure that I understood the alternative that you are proposing. No script? Can you maybe propose a PR implementating the alternative?

Yeah, the alternative would be to just invoke mypy directly by cding into Lib/test and running mypy --config-file libregrtest/mypy.ini -- you can do that now, although there are still some mypy errors. And if #109518 is merged, we won't even need to cd into Lib/test before invoking mypy -- we'll just be able to run mypy --config-file Lib/test/libregrtest/mypy.ini from the repo root to invoke mypy. It feels like a simpler and less hacky approach to me than a custom script.

The only disadvantage from that approach is that everything imported from test.support will be inferred by mypy as having type Any. But that's honestly not the worst thing in the world; I can live with that. There's a bug in mypy 1.5 that means that we would get a single false positive as a result of everything from test.support being inferred as Any. But the bug will be fixed in mypy 1.7 (#109464 (comment)), and in the meantime, we could just # type: ignore it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood added 2 commits September 18, 2023 12:51
@AlexWaygood
Merge branch 'main' into mypy-regrtest-script
50df501
@AlexWaygood
Updates now python#109518 is merged
1806074
@vstinner

vstinner commented Sep 18, 2023

Copy link
Copy Markdown
Member

And if #109518 is merged, we won't even need to cd into Lib/test before invoking mypy

It got merged. Do you want to propose an alternative PR which doesn't use temporary directory and copytree()?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented Nov 30, 2023

Copy link
Copy Markdown
Member Author

And if #109518 is merged, we won't even need to cd into Lib/test before invoking mypy

It got merged. Do you want to propose an alternative PR which doesn't use temporary directory and copytree()?

yes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood closed this Nov 30, 2023
@AlexWaygood
AlexWaygood deleted the mypy-regrtest-script branch November 30, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@AlexWaygood @hauntsaninja @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.