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

CI: add mypy to strict type check #3165

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

beauxq
Copy link
Collaborator

@beauxq beauxq commented Apr 17, 2024

What is this fixing or adding?

This is an addition to #3121 with some previous discussion here #3107

There are some errors that mypy catches and pyright doesn't, and some errors that pyright catches and mypy doesn't.
So we can catch more problems if we check with both.

The one Python script .github/type_check.py will show output from both mypy and pyright.

One of the biggest issues we've found with mypy in AP is this bug: python/mypy#10506
After lots of battling with it in #2173 and #2899, I think we worked around it so it only needs a # type: ignore for default = "random"
I think that will only happen with a few options in worlds, and this check is opt-in from world maintainers.

How was this tested?

submitting this PR, because thar's the only way to test github actions

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Apr 17, 2024
@beauxq
Copy link
Collaborator Author

beauxq commented Apr 17, 2024

A minor issue I found is this:

def foo(option_class: Union[Type[NumericOption], Type[FreeText]]) -> None:
    print(option_class.name_lookup.values())

reports: "error: Access to generic class variables is ambiguous [misc]"
This should be pretty rare, because it only happens with the union of multiple options, and not with 1 base type Type[Option[int]]

.github/type_check.py Outdated Show resolved Hide resolved
.github/type_check.py Outdated Show resolved Hide resolved
.github/type_check.py Outdated Show resolved Hide resolved
.github/type_check.py Outdated Show resolved Hide resolved
Comment on lines +25 to +26
with open(config) as config_file:
config_data: Union[Dict[str, Any], Any] = json.load(config_file)
Copy link
Member

@black-sliver black-sliver Apr 20, 2024

Choose a reason for hiding this comment

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

Shouldn't mypy use a separate list of files, so as the maintainer of a file they can decide which one they think should be applied for their code (assuming they are not 1:1 compatible)?

Copy link
Collaborator Author

@beauxq beauxq Apr 20, 2024

Choose a reason for hiding this comment

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

We can't really isolate the checks to a specific list of files.
The list of files we give (to either mypy or pyright) is not a list of files to check, It's a list of files to report errors in. Both type checkers follow imports to get type information from other files. So the type information in any file is going to affect both checks.
If someone thinks they're only using mypy, because the file they're changing is only on the mypy list, that change will still affect the pyright check, even though the file is not on the pyright list.

Because we can't completely isolate them, I don't think it's worth it to maintain 2 separate lists.

The main focus of this is core, and worlds are opt-in.
If this PR is accepted (I don't consider it decided yet whether we want this), If a world maintainer wants to only use one, then they shouldn't opt in to this system, and should just check with their own workflow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You shouldn't think of this as a linter, checking stylistic things.
You should think of it as a static type checker, just like in a C++ or Rust compiler.

I think it wouldn't make sense to say:
"For this C++ file x.cpp I only care whether clang's type checker passes, but for y.cpp I only care whether gcc's type checker passes."
We would care about everything passing, in one compiler, or the other compiler, or we want to support both compilers.

I think it would be good to support multiple static type checkers, just like a C++ project that wants to support multiple compilers.

Copy link
Member

Choose a reason for hiding this comment

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

That argument makes sense, yeah.

That being said, I honestly don't know how we (or who) decide(s) if we want to merge this or not.

@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Apr 26, 2024
@black-sliver
Copy link
Member

I guess I would like for more people to chime in and say if they think this is good or bad. It's hard for me to decide if this is an upgrade or not.

In my mind, having both allows more proper placement of typing or # type: ignore to ensure compatibility with both mypy and pyright, however pinning both in CI will also give more opportunities for it to be out of line with what people use locally.

In other words,
if the CI succeeds we can be fairly certain that running any type checker locally will work, but
if we get a fix in either one, we still can not "use" it until it's fixed in both and the pins got updated.

@black-sliver
Copy link
Member

@NewSoupVi do you have an opinion on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants