py-mine / mcstatus

A Python library for checking the status of Minecraft servers
https://mcstatus.readthedocs.io
Apache License 2.0
472 stars 37 forks source link

Support mypy #699

Closed CoolCat467 closed 9 months ago

CoolCat467 commented 9 months ago

Officially supporting type checking with mypy as well would mean I don't get type errors in my project just because I don't use pyright and opens the door to using mypyc to compile the project into a C extension, which in some cases can make things run faster.

ItsDrike commented 9 months ago

The biggest reason as to why this project only supports pyright is that mypy often takes significantly longer than pyright to implement any new typing features.

In the time of adding pyright to the project, mypy still didn't even have support for typing.Self (even though pyright already had it for about a year by then), which made mypy a bad choice for us at the time.

Since then, I haven't really been keeping up to date with mypy's development, and it's entirely possible that they've got better at keeping up though. However if that's not the case, I'm not sure I'd like to bring it in, as it could potentially prevent us from being able to use some nice new typing features.


That said, if things are better now, I'm not necessarily against adding mypy alongside with pyright.

I don't think we would make any use of mypyc though, and I'd like to have a better reason to make this change than just:

would mean I don't get type errors in my project just because I don't use pyright

Contributors are expected to conform to the project's style guidelines, and to set up their development environment for the project they're working on accordingly to those guidelines, regardless of what they prefer.

(Hint: You don't need to have pyright in your editor if you really don't want to, just make sure to run pre-commit to ensure that pyright will pass, and not to introduce any mypy-specific stuff. Although adding pyright to your editor would probably result in a better experience. Most editors should be configurable in a way that lets you disable/enable various linters/type-checkers on a per-project basis, so it doesn't have to change your workflow on other projects.)

Even if mypy support would be added though, it won't mean removal of pyright, we would just be running both, so you'll probably want to set up pyright for mcstatus anyway.


I would like to ask all other contributors reading this to give their opinion on adding mypy, either by commenting, or at least by adding a :+1: or :-1: reaction to the issue, as it is a fairly significant change, and makes mypy knowledge yet another pre-requisite, alongside pyright and all of our other linters.

PerchunPak commented 9 months ago

would mean I don't get type errors in my project just because I don't use pyright

As I can understand, you get errors when using this project as library in another project? If so, could you please provide what errors you get? I used this library in projects powered by mypy a lot and didn't get any problems.

opens the door to using mypyc to compile the project into a C extension

I also ran this small script using mypyc just fine without any changes to the project code.

# mypyc_test.py
import time
import mcstatus

t0 = time.time()
server = mcstatus.JavaServer.lookup("play.hypixel.net")
status = server.status()
print(status.motd)
print(time.time() - t0)
~/dev/mcstatus master ❯ python mypyc_test.py
Motd(parsed=['                ', <MinecraftColor.GREEN: 'a'>, 'Hypixel Network ', <MinecraftColor.RED: 'c'>, '[1.8-1.20]\n         ', <MinecraftColor.DARK_AQUA: '3'>, '', <Formatting.BOLD: 'l'>, 'SKYBLOCK 0.19.9 ', <MinecraftColor.GRAY: '7'>, '| ', <MinecraftColor.AQUA: 'b'>, '', <Formatting.BOLD: 'l'>, 'BEDWARS v1.9'], raw='                §aHypixel Network §c[1.8-1.20]\n         §3§lSKYBLOCK 0.19.9 §7| §b§lBEDWARS v1.9', bedrock=False)
0.45096421241760254
~/dev/mcstatus master ❯ mypyc mypyc_test.py
running build_ext
building 'mypyc_test' extension
creating build/temp.linux-x86_64-cpython-312
creating build/temp.linux-x86_64-cpython-312/build
gcc -fno-strict-overflow -Wsign-compare -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -fcf-protection -fexceptions -fcf-protection -fexceptions -fcf-protection -fexceptions -fPIC -I/home/perchun/.cache/pypoetry/virtualenvs/mcstatus-Owe_Dc5Q-py3.12/lib64/python3.12/site-packages/mypyc/lib-rt -I/home/perchun/.cache/pypoetry/virtualenvs/mcstatus-Owe_Dc5Q-py3.12/include -I/usr/include/python3.12 -c build/__native.c -o build/temp.linux-x86_64-cpython-312/build/__native.o -O3 -g1 -Werror -Wno-unused-function -Wno-unused-label -Wno-unreachable-code -Wno-unused-variable -Wno-unused-command-line-argument -Wno-unknown-warning-option -Wno-unused-but-set-variable -Wno-ignored-optimization-argument -Wno-cpp
creating build/lib.linux-x86_64-cpython-312
gcc -shared build/temp.linux-x86_64-cpython-312/build/__native.o -L/usr/lib64 -o build/lib.linux-x86_64-cpython-312/mypyc_test.cpython-312-x86_64-linux-gnu.so
copying build/lib.linux-x86_64-cpython-312/mypyc_test.cpython-312-x86_64-linux-gnu.so -> 
~/dev/mcstatus master ❯ python -c "import mypyc_test"
Motd(parsed=['                ', <MinecraftColor.GREEN: 'a'>, 'Hypixel Network ', <MinecraftColor.RED: 'c'>, '[1.8-1.20]\n         ', <MinecraftColor.DARK_AQUA: '3'>, '', <Formatting.BOLD: 'l'>, 'SKYBLOCK 0.19.9 ', <MinecraftColor.GRAY: '7'>, '| ', <MinecraftColor.AQUA: 'b'>, '', <Formatting.BOLD: 'l'>, 'BEDWARS v1.9'], raw='                §aHypixel Network §c[1.8-1.20]\n         §3§lSKYBLOCK 0.19.9 §7| §b§lBEDWARS v1.9', bedrock=False)
0.3873469829559326

And to be fare:

Mypyc is currently alpha software. It’s only recommended for production use cases with careful testing, and if you are willing to contribute fixes or to work around issues you will encounter.

PerchunPak commented 9 months ago

That said, if things are better now, I'm not necessarily against adding mypy alongside with pyright.

Even if they are so, I would expect conflicts between those too (as mypy has warnings about unused type ignores, and pyright has more checks than mypy which may be false-positive/inaccurate sometimes)

So to summarize, I don't see any sense in keeping both, as they would likely conflict. If you get errors in your project, where mcstatus is used properly as library installed through pip (not like cloned with git in the same folder) - it is probably a bug.

ItsDrike commented 9 months ago

Even if they are so, I would expect conflicts between those too (as mypy has warnings about unused type ignores, and pyright has more checks than mypy which may be false-positive/inaccurate sometimes)

Those warnings can be disabled, also pyright has # pyright: ignore which we can make use of to prevent such conflicts, there are code-bases using both, so it is certainly doable. It's more a question of whether we're willing to put in the work to do such a migration, and whether we're then willing to work in such environment.

PerchunPak commented 9 months ago

Oh and here are errors raised by mypy now. Some of them are covered in newer version of pyright (I will do fixes later, but if someone would want to take it - I don't mind) and some of them are mypy-specific false-positives. Although there are errors, that weren't catched by pyright.

❯ mypy mcstatus/
mcstatus/motd/transformers.py:30: error: Incompatible types in assignment (expression has type "function", variable has type "Callable[[Formatting | MinecraftColor | WebColor | TranslationTag | str], _HOOK_RETURN_TYPE]")  [assignment]
mcstatus/motd/transformers.py:147: error: Need type annotation for "on_reset" (hint: "on_reset: List[<type>] = ...")  [var-annotated]
mcstatus/utils.py:147: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [overload-overlap]
mcstatus/utils.py:154: error: Overloaded function implementation does not accept all possible arguments of signature 2  [misc]
mcstatus/utils.py:154: error: Overloaded function implementation cannot produce return type of signature 2  [misc]
mcstatus/motd/__init__.py:52: error: Name "raw" already defined on line 40  [no-redef]
mcstatus/motd/__init__.py:186: error: Name "__class__" is not defined  [name-defined]
mcstatus/status_response.py:144: error: Argument "enforces_secure_chat" to "JavaStatusResponse" has incompatible type "object"; expected "bool | None"  [arg-type]
mcstatus/address.py:63: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
mcstatus/querier.py:70: error: Incompatible types in assignment (expression has type "UDPAsyncSocketConnection", base class "ServerQuerier" defined the type as "UDPSocketConnection")  [assignment]
mcstatus/querier.py:72: error: Return type "Coroutine[Any, Any, Connection]" of "_read_packet" incompatible with return type "Connection" in supertype "ServerQuerier"  [override]
mcstatus/querier.py:78: error: Return type "Coroutine[Any, Any, None]" of "handshake" incompatible with return type "None" in supertype "ServerQuerier"  [override]
mcstatus/querier.py:84: error: Return type "Coroutine[Any, Any, QueryResponse]" of "read_query" incompatible with return type "QueryResponse" in supertype "ServerQuerier"  [override]
mcstatus/pinger.py:90: error: Incompatible types in assignment (expression has type "TCPAsyncSocketConnection", base class "ServerPinger" defined the type as "TCPSocketConnection")  [assignment]
mcstatus/pinger.py:92: error: Return type "Coroutine[Any, Any, JavaStatusResponse]" of "read_status" incompatible with return type "JavaStatusResponse" in supertype "ServerPinger"  [override]
mcstatus/pinger.py:111: error: Return type "Coroutine[Any, Any, float]" of "test_ping" incompatible with return type "float" in supertype "ServerPinger"  [override]
mcstatus/__main__.py:32: error: Incompatible types in assignment (expression has type "str", target has type "bool")  [assignment]
mcstatus/__main__.py:33: error: Incompatible types in assignment (expression has type "int", target has type "bool")  [assignment]
mcstatus/__main__.py:34: error: Incompatible types in assignment (expression has type "Motd", target has type "bool")  [assignment]
mcstatus/__main__.py:35: error: Incompatible types in assignment (expression has type "int", target has type "bool")  [assignment]
mcstatus/__main__.py:36: error: Incompatible types in assignment (expression has type "int", target has type "bool")  [assignment]
mcstatus/__main__.py:37: error: Incompatible types in assignment (expression has type "list[Never]", target has type "bool")  [assignment]
mcstatus/__main__.py:39: error: Incompatible types in assignment (expression has type "list[dict[str, str]]", target has type "bool")  [assignment]
mcstatus/__main__.py:41: error: Incompatible types in assignment (expression has type "float", target has type "bool")  [assignment]
mcstatus/__main__.py:45: error: Incompatible types in assignment (expression has type "str", target has type "bool")  [assignment]
mcstatus/__main__.py:46: error: Incompatible types in assignment (expression has type "str", target has type "bool")  [assignment]
mcstatus/__main__.py:47: error: Incompatible types in assignment (expression has type "str", target has type "bool")  [assignment]
mcstatus/__main__.py:48: error: Incompatible types in assignment (expression has type "list[str]", target has type "bool")  [assignment]
Found 27 errors in 7 files (checked 16 source files)
ItsDrike commented 9 months ago

Many of the errors that weren't checked by pyright is because we didn't enable some optional extra checks that pyright has, but yeah nevertheless, mypy is generally more strict than pyright.

ItsDrike commented 9 months ago

There's a really nice document showing the differences between the two type-checkers here: https://github.com/microsoft/pyright/blob/main/docs/mypy-comparison.md. Could be useful when considering whether we'd like to add it.

kevinkjt2000 commented 9 months ago

Running both pyright and mypy would increase our maintenance burden for false positives. So, that leaves the option of selecting one over the other. Pyright has proven helpful in improving mcstatus and detecting some real issues. I would want to see proof of mistakes that mypy is catching that pyright cannot catch before considering a switch. Reading through that mypy output above shows me that mypy is catching a lot of false positives that we already handled with pyright. Until a worthwhile proof appears, pyright is handling the job.