CI: strict mypy check in github actions

[beauxq]

What is this fixing or adding?

mypy_files.txt is a list of files that will fail the CI if mypy finds errors in them

The idea is that we gradually add files and directories to the list as we get them cleaned up.
Eventually it can include all core python source files/directories.

World files and directories could be opt-in by the world maintainer. Adding all worlds doesn't need to be a goal.

This will help keep typing issues from creeping back in after they have been cleaned up.

The check that is done by CI is in a bash script .github/mypy_check.sh

• Developers can run this script before committing/pushing to see whether it will pass.
• Windows users should be able to run it with Git Bash.

A few minor typing changes are included here that are needed to finish cleaning these starting files.

How was this tested?

running the .github/mypy_check.sh script

[NewSoupVi]

Ah hmmm, I have a PR open for a ruff.toml in the witness directory. I suppose I will have a look at mypy to see if I can switch over or have both

I suppose I can have a pyproject.toml and have directives for both, probably?

[beauxq]

There are some decisions here on mypy options that might be worth discussing.

• --no-warn-unused-ignore - don't report a problem if there's a # type: ignore where mypy doesn't see any problem with the code

  • The reason for this is that different type checkers get different results. If someone uses # type: ignore for a different type checker, mypy would complain about it.

• --no-warn-return-any - don't report a problem if something with the Any type is returned from a function (where the function is annotated to return something else)

  • arguments for using --no-warn-return-any:

    • Generally, Any is a way to silence the type-checker, when we can't specify the type, or it's not worth it to specify the type. If we don't use this, then Any isn't silencing the type-checker when we might want it to.
      The workaround for this would usually just be a cast or # type: ignore or assert isinstance right before returning.
      • assert isinstance can't be used for some types - cast doesn't fit in some complex expressions

  • arguments against using --no-warn-return-any:

    • We use a lot of argparse.Namespace
      If we have code like return args.the thing, that's dangerous, because it's not doing anything to make sure we return the correct type. The type checker doesn't warn us because it's Any

    • The assert isinstance would help catch things in unit tests.

    • cast or # type: ignore could serve as a reminder that humans need to pay more attention to it.

I'm still kind of on the fence about --no-warn-return-any

[NewSoupVi]

So idk if there's anything you can do about this, but when I wanted to run this locally, I had the following problem

I ran the command on Windows in Git Bash.
It would keep telling me invalid argument for each line.

This was because of CRLF line endings. I needed to convert the .txt file to LF first, and then it worked: dos2unix ./.github/mypy_files.txt
This might be because my Git is configured in such a way that it auto replaces LF with CRLF on clone/pull, and then converts back on commit/push.

Maybe worth making a note about somewhere, not sure

[NewSoupVi]

If I do import schema in an options.py file, I get this:

worlds\witness\options.py:3: error: Skipping analyzing "schema": module is installed, but missing library stubs or py.typed marker [import-untyped]

That probably needs to be supressed? Not sure

[beauxq]

We could add --disable-error-code=import-untyped
But another solution, that is more type safe, but also takes more work, is to make our own type stubs for libraries we use that don't have type information.

That's what the typings directory is for in Archipelago. I've been maintaining some kivy type stubs there for a while.
I improve them once in a while for the things I work on. We don't have typing for all of kivy, because that would be a ton of work. So I just add typing for the parts I work with.

[beauxq]

@NewSoupVi I'd like you to comment on the --no-warn-return-any comments here #3107 (comment)
because I noticed it looking at worlds/messenger/subclasses.py

From this line

world = self.parent_region.multiworld.worlds[self.player]

The type checker doesn't know that world is a MessengerWorld, so it doesn't know about world.shop_prices below.

return world.shop_prices[name] + prereq_cost

It's kind of by chance that you don't get a type error rather than Any, because of the __getattr__ defined in World

Archipelago/worlds/AutoWorld.py

Line 306 in 569c37c

def __getattr__(self, item: str) -> Any:

If it weren't for that, you'd need to deal with this even with --no-warn-return-any, but other relevant situations will be similar to this.

The way I would resolve this is with

assert isinstance(world, MessengerWorld)

(importing MessengerWorld locally in that function, because a global import is likely to move towards circular import issues.)

---

Do you think situations like this are worth using assert isinstance and the like, so that we can get more type safety without --no-warn-return-any

[NewSoupVi]

There are definitely some justifications for "Any", and I think forcing compliant worlds to use typing.cast or assert isinstance all the time would probably be bad.

Here is a situation I ran into:

some_dict: Dict[int, Union[str, List[str]] = {
    1: "String"
    2: ["List", "of", "strings"]
}

dict[2].append("another string")

-> mypy gives an error for "str doesn't have append"

So I would have to do this:

cast(List[str], dict[2]).append("another string")

But, because "Any" is allowed, I was able to just do this instead:

some_dict: Dict[int, Any]

which was nice.

I can also see arguments against allowing "Any", but I feel like if we make it super hard for worlds to comply with this, it's probably less likely that people are going to jump on board. I feel like I already have a high tolerance for a Python dev for putting casts and if isinstance and if foo is None everywhere, and if I had to work around Any as well, I'm not sure I would've wanted to continue refactoring

[NewSoupVi]

I tried to refactor Witness to be mypy compliant.

It exposed some genuine bugs, which is cool. It also shows some huge structural problems and oddities of my implementation that will now stare me in the face until I fix them, which is great as well.

The only annoying thing is that since core is not mypy compliant, I can't fix two of the errors. I would have to go into core myself and make PRs to make those files mypy compliant.

I think potentially disabling [import-untyped] and [no-untyped-call] might be good? I don't see a situation in which a world would be doing those things in a bad way if all of its files pass mypy individually - If their own functions are all passing mypy, then it by definition must be one from an outside package, so it can't be their fault, right?

Edit: I can also think of a case here where for example an ap randomizer wraps and existing randomizerthat is also written in Python (OoT is an example), and in that case I would also think it's unfair to expect "import-untyped" to be complied with

#3112

[beauxq]

This particular option isn't about any usage of Any, it's about whether we're allowed to return Any from a function that's supposed to return something else.

Even without allowing the return of Any you'd still be allowed to use it within one function for things like that.

If you remove the --no-warn-return-any and then check worlds/messenger/subclasses.py, you'll see it's fine with using Any inside the function, but it doesn't like you returning Any from the function.

The purpose of this feature is to stop problems from escaping the function and spreading.

[NewSoupVi]

This particular option isn't about any usage of Any, it's about whether we're allowed to return Any from a function that's supposed to return something else.

Oh, that's my bad (I'm completely new to the world of mypy haha). In that case, I think I might be on board? In the example you gave, I absolutely think the world should be cast to MessengerWorld.

[beauxq]

I think potentially disabling [import-untyped] and [no-untyped-call] might be good? I don't see a situation in which a world would be doing those things in a bad way if all of its files pass mypy individually - If their own functions are all passing mypy, then it by definition must be one from an outside package, so it can't be their fault, right?

I think I want to keep [import-untyped] and [no-untyped-call] for a few reasons.

• Core is the focus here. The ability for worlds to opt in is just a side-effect feature.
  • (If this is forcing you to make typing fixes in core, that's a good thing, because core is the focus of this check.)
    • (These kinds of small typing fix PRs usually don't take as long as other PRs.)
• These particular diagnostics help with the gradual nature of this endeavor.
  • Example: Because Baseclasses.py is 1500 lines, that would make it more difficult to add strict checking to it all at once, but with [no-untyped-call], other smaller files that import from Baseclasses.py will be added to the list and get some checking in Baseclasses.py before we're able to add the entire Baseclasses.py to the list.
    (That's the reason this PR fixes typing in a few files that are not in the list. So this PR, as it is, gets to check more than just those 2 files. If we disable these checks, it would only check those 2 files, and be a lot less useful, and more difficult to spread to the rest of core.)

Edit: I can also think of a case here where for example an ap randomizer wraps and existing randomizerthat is also written in Python (OoT is an example), and in that case I would also think it's unfair to expect "import-untyped" to be complied with

This is exactly why I said "Adding all worlds doesn't need to be a goal", and should be opt-in.
Note that core should not be importing directly anything specific from worlds, so those worlds shouldn't be able to infect the typing of core.

• (Of course we know core does import directly from LTTP, but that's a problem we all know needs to be fixed.)

[NewSoupVi]

• (If this is forcing you to make typing fixes in core, that's a good thing, because core is the focus of this check.)

Interesting. I don't know if I'd personally be motivated to go into core to add typing because I can't put my world in otherwise, so I might just, like, run the mypy locally and be happy that it's still returning 2 errors because of core.
But it might work on other people, so I can't say it's a bad strategy :D

[Silvris]

Something I've noticed while working on cleaning up my world with this, and will undoubtedly be a common question should anyone try to add their own world to this:

for i in range(1, 11):
    set_rule(world.get_location(f"Location {i}"), lambda state, x=i: state.has("Item", x, world.player))

This will throw the following mypy error, while currently being the most accepted way of solving this problem.
error: Cannot infer type of lambda [misc]

Unsure of what solutions are available, if any. It seems this error cannot be disabled without disabling all misc errors.

[beauxq]

for i in range(1, 11):
    set_rule(world.get_location(f"Location {i}"), lambda state, x=i: state.has("Item", x, world.player))

This will throw the following mypy error, while currently being the most accepted way of solving this problem. error: Cannot infer type of lambda [misc]

Unsure of what solutions are available, if any. It seems this error cannot be disabled without disabling all misc errors.

This is an issue reported in the mypy issue tracker: python/mypy#15459

In Zillion, I use functools.partial to get around this.

This brings up another possible point of discussion for this:
We could choose to use a different type checker for this.
Generally, mypy has more bugs than pyright.
pyright correctly handles the type of this lambda
The reason that I implemented this PR with mypy was just because I found it easier to set up.
(command line options and choosing which files to analyze - I think pyright requires a configuration file for non-default options, and I don't even know if we can specify which files as cleanly.)

[beauxq]

It wasn't as hard as I thought it would be: #3121

[beauxq]

We might add mypy to the existing pyright check.