Enforce some more pyright settings #133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As we didn't previously specify which python version should pyright use, it automatically defaulted to the latest one. This adds a setting to make pyright assume we're usng python 3.8 (lowest supported python version), allowing it to easily catch some mistakes, such as using a fucntion that's not yet available in older python versions when developing with a newer python version installed. Doing this actually uncovered some mistakes, namely: - `docs/conf.py` used `str.removesuffix` function was used, which is however a 3.9+ function. - `docs/conf.py` was missing a `__future__.annotations` import, causing annotations like `list[str]` to fail on 3.8 (3.9+) - `mcproto/types/chat.py` was importing `TypeAlias` from `typing`, however `TypeAlias` was only introduced into `typing` in 3.9, so to support 3.8, we should be importing from `typing_extensions`. - `mcproto/types/chat.py` was using `list[RawChatMessageDict]` in a type-alias constant definition, which however wouldn't work on 3.8 (list was made generic in 3.9). This was solved by making this annotation a string instead.
This enables various optional toggles, making pyright report some more errors that it would by default. Specifically, these additional diagnostics were enabled: - `reportUntypedFunctionDecorator`: Report function decorators that have no type annotations, obscuring the function return type and input parameters. - ` reportUntypedClassDecorator`: Similar to ` reportUntypedFunctionDecorator`, but for classes - `reportUntypedNamedTuple`: Report the use of `collections.namedtuple` instead of the typed alternative: `typing.NamedTuple`, which supports specifying the types of the values it holds. - `reportTypeCommentUsage`: Prior to Python 3.5, the parser did not support type annotations, so comments prefixed with "type" were used. While not yet deprecated, this feature is very old and there is no reason to use it instead of using regular type annotations. - `reportConstantRedefinition`: Attempts to redefine variables whose names are in all-caps will produce an error. - `reportDeprecated`: Any use of functions decorated with `typing_extensions.deprecated` (following draft PEP 702). - `reportOverlappingOverload`: Overloads that overlap in signature and obscure each other or have incompatible return types will produce errors. - `reportShadowedImports`: Presence of files that are overriding a module in stdlib (have the same name) will be marked as errors.
This makes pyright treat any unnecessary comparisons or similar checks, where the result is always known due to static analysis as errors. This is good, because it saves us some computation time, and it can sometimes catch actual mistakes is the code-base (which it actually did, see below). In our codebase, there were 2 instances where we did perform an unnecessary comparison: - In `mcproto/packets/handshaking/handshake.py`, there was an `isinstance` check in `__init__` of the `Handshake` packet class, where we check that the `next_state` argument, annotated to be a union of `NextState` (`IntEnum`) and `int`. This was a check of whether the `next_state` argument is a subclass of `int`, and if it is, it converted this integer to the appropriate state in the `NextState` enum. However since this enum is an `IntEnum`, it was itself actually a subclass of `int`, making it's instances always pass this check, leading to the logic of converting simple ints to enums also run when an actual enum was passed in. I have fixed this by moving to a isinstance check for the `NextState` enum, and running the logic if that's not the case. - In `mcproto/types/chat.py`, in the `as_dict` method, there is a sequence of `isinstance` checks, each of which narrows the type further and further, leaving the last check to always result in the same (`dict`) type, making this last isinstance unnecessary, as it couldd simply be an `else`. However in this case, there is actually an `else` below even this last check, which should statically never run, where an exception about invalid type gets raised. In this case, I do find this useful, as if the protocol changes for whatever reason, and the returned JSON would return something unexpected, we can immediately see an exception telling us exactly what's going on. Because of this, I have ignored this violation for this specific case.
ItsDrike
added
t: bug
In OOP programming, we should generally try to follow "LSP": Liskov Substitution Principle, which basically states that any class A, inheriting from another class B should be able to be treated as that class B, without issues. So as an example, if our parent class (B) has a method `foo`, which doesn't take any arguments, in the new class (A), we can override this method, but we can't now make `foo` take an additional argument, as that would mean this new class A can't be assumed to act like B, just with extra features. Hence this would be violating LSP. In the existing code-base, there were 4 violations like this found, 2 of which were resolved, and 2 ignored: - First 2 violations were found in the `tests/mcproto/protocol/helpers.py` file, the violation was about overriding Mock's `__call__`, which we don't care about, as in this case we break LSP intentionally, to make this class behave like the object we intent to mock. - Second 2 violations were found in `tests/mcproto/protocol/test_base_io.py` file, which were about our "concrete" writer classes, that directly inherit from the base writer classes, and override the abstract `write` function to make these classes initializable and testable. However this override actually mistakenly assumed that the `write` function was taking in a `bytearray`, when in fact the base class specifies that it takes `bytes`. This violation was fixed. Additionally, during fixing of the second 2 violations, I have noticed that in the `tests/mcproto/protocol/helpers.py` file (where the first 2 violations were), in the `WriteFunctionMock` class, the same mistake was made (having the write function take `bytearray` instead of `bytes`), which I fixed here as well.
ItsDrike
force-pushed
the
pyright-settings
branch
from
e40435e
to
b53c54c
Compare
sppookkyblake
approved these changes
Jun 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pyright is capable of enforcing various rules, which are disabled by default. This enables the enforcement of some of these, and fixes the issues uncovered by them.
Some of these issues are actually quite significant, turns out there were various instances where we used python 3.8 incompatible code, and needlessly tried to obtain an appropriate value from
NextState(IntEnum), even when the parameter was already an instance ofNextState(more info on this in the description of the commit that fixed this). There were also some other more minor issues, which should however also get addressed.I find most of these newly enabled rules relatively easy to follow, and I think we should enforce them as they can catch various, often pretty hard to find issues, and don't actually affect the contributing process too much (it is relatively hard to violate them, even if you don't know that you should follow these rules, this can be seen by how few violations there actually were after enabling these in the existing codebase).