Enable reporting unknown types #333

ItsDrike · 2024-07-13T10:29:08Z

This PR is an experiment, just to see how the code-base would look like if we enabled reporting unknown types for basedpyright.

It's more of a prompt for discussion on whether or not this is worth it, I do not intend to merge this, at least not before this discussion takes place.

The issue with these rules is that they can sometimes be incredibly annoying to satisfy, it often means having to use a bunch of type-casts or just ignoring them. However, in many cases, they can also be incredibly useful, as it's good to know when basedpyright can't recognize some type and needs some help in understanding it. Overall, it would definitely improve the type safety, but the question is whether it's even worth it.

LiteApplication · 2024-07-15T08:15:59Z

docs/conf.py

@@ -110,7 +110,7 @@
 autodoc_member_order = "bysource"

 # Default options for all autodoc directives
-autodoc_default_options = {
+autodoc_default_options: dict[str, Any] = {


I don't see how these kind of type annotations are better

Yeah, the thing is, pyright would otherwise mark this as dict[str, Unknown] and with Unknown being banned, if a wildcard type is indeed what we want, it must be declared explicitly like this. That's also one of the reasons why these rules might be a bit too strict.

LiteApplication · 2024-07-15T08:16:26Z

docs/extensions/attributetable.py

@@ -153,7 +153,7 @@ def run(self) -> list[AttributeTablePlaceholder]:
 def build_lookup_table(env: BuildEnvironment) -> dict[str, list[str]]:
    # Given an environment, load up a lookup table of
    # full-class-name: objects
-    result = {}
+    result: dict[str, list[str]] = {}


But for these ones it's interesting to have type checking

I agree, in some cases, being forced to explicitly declare the annotations can be very helpful to the type checker, it is nice that this would now be a requirement, where types can't be easily inferred.

LiteApplication · 2024-07-15T08:17:22Z

mcproto/auth/microsoft/xbox.py

@@ -66,7 +66,7 @@ def __init__(self, exc: httpx.HTTPStatusError):
    @property
    def msg(self) -> str:
        """Produce a message for this error."""
-        msg_parts = []
+        msg_parts: list[str] = []


I already have things like that all over the place in more-packets

LiteApplication · 2024-07-15T08:19:35Z

tests/mcproto/types/test_nbt.py

@@ -477,7 +479,7 @@ def test_nbt_bigfile():
    data = bytes.fromhex(data)
    buffer = Buffer(data)

-    expected_object = {  # Name ! Level
+    expected_object: FromObjectType = {  # Name ! Level


This I find useful

LiteApplication · 2024-07-15T09:56:09Z

I'm not against these new rules as I feel they are not the most annoying of the bunch, but I don't really know if the extra effort is worth the while.

ItsDrike · 2024-07-15T16:25:42Z

The worst / most annoying stuff are recursive types, like those in nbt.py, since pyright can only understand them up to 1 level deep. After that, they become Unknown and casts are necessary. That was definitely the most annoying part of this.

LiteApplication · 2024-07-15T21:10:48Z

Type checking is useful when it uses the computer's power to mitigate our forgetfulness, but when it's going the opposite way I find it annoying 😆 .

Since we have the reportUnknownParameterType for basedpyright enabled, we can't leave `*args` or `**kwargs` unannotated anyways, so we might as well enforce it properly.

ItsDrike · 2025-01-08T01:45:38Z

The worst / most annoying stuff are recursive types, like those in nbt.py, since pyright can only understand them up to 1 level deep. After that, they become Unknown and casts are necessary. That was definitely the most annoying part of this.

Seems that basedpyright now supports recursive types properly, so the latest rebase here removes all of those workaround casts for recursive types, making this a lot more pleasant rule.

I'm still slightly worried about enabling it though, as it also means that any untyped libs that we may depend on will cause these warnings every time they're used, and these would be pretty annoying to remove, as simply declaring the type e.g. x: int = untyped_func() isn't enough to suppress this, because pyright doesn't let you ignore the internal Unknown type quite so easily. Unknown would actually propagate here, making x a union of int | Unknown, hence triggering reportUnknownVariableType. Just a pyright ignore isn't enough either, because the type would remain a union of those 2, triggering more issues when you work with that variable in pretty much any way.

Instead, the only real way to resolve this would be to use an explicit cast e.g. x = typing.cast(int, untyped_func()). This isn't horrible, but can be annoying, especially for heavy use of untyped libraries. If we can't figure out the type of the return value easily, it might even lead to cast(Any, x) situations, just to convert the implicit any type (Unknown) into an explicit Any type, which isn't banned.

That said, these rules really are handy to have enabled and do improve the typing experience significantly in some cases. Mcproto also doesn't currently rely heavily on any such untyped libraries, and most of the popular libraries would remain usable without issues, as they are usually typed. Also, mcproto is actually fairly unlikely to pick up a lot of runtime dependencies with what it's purpose is, so it might be worth it.

Note that there is a way to compromise, and get the "best of both worlds", where instead of enabling these rules outright, making them required and checked in the validation CI & pre-commit, we could use the "hint" level for these rules, which would make basepyright report these diagnostics in the IDE, while editing, but not following them wouldn't be considered a full-on failure that prevents merging.

That said, we shouldn't just jump to this without thinking, this could still produce a lot of clutter diagnostics when opening up a file with a bunch of such untyped calls and it's also losing on the main benefit of type-checking, being the checking part. If this isn't actually enforced, we can still end up with unknown types in various places, leading to worse type-checking.

However, if we do decide against a fully enforced approach, this is definitely something to consider before throwing this PR away entirely.

ItsDrike added t: enhancement Improvement to an existing feature p: 3 - low This can wait do-not-merge This PR can be reviewed, but cannot be merged now a: internal Related to internal API of the project labels Jul 13, 2024

ItsDrike marked this pull request as ready for review July 13, 2024 10:32

ItsDrike mentioned this pull request Jul 13, 2024

Use a stricter configuration for basedpyright #332

Merged

ItsDrike force-pushed the enable-unknown-type-reporting branch from 6cbd87e to e434651 Compare July 13, 2024 10:34

LiteApplication reviewed Jul 15, 2024

View reviewed changes

py-mine-ci-bot bot added the s: stale Has had no activity for a while (will be closed for inactivity/marked up for grabs soon) label Oct 27, 2024

Base automatically changed from strict-basedpyright to main January 7, 2025 22:08

ItsDrike added 3 commits January 7, 2025 23:19

Enable reporting unknown types

e563d56

Enforce annotating args & kwargs with ruff

5d5eb64

Since we have the reportUnknownParameterType for basedpyright enabled, we can't leave `*args` or `**kwargs` unannotated anyways, so we might as well enforce it properly.

Enable reportUnknownLambdaType

fd4a475

ItsDrike force-pushed the enable-unknown-type-reporting branch from 7fa69fc to fd4a475 Compare January 7, 2025 22:20

py-mine-ci-bot bot removed the s: stale Has had no activity for a while (will be closed for inactivity/marked up for grabs soon) label Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable reporting unknown types #333

Enable reporting unknown types #333

ItsDrike commented Jul 13, 2024 •

edited

Loading

LiteApplication Jul 15, 2024

ItsDrike Jul 15, 2024

LiteApplication Jul 15, 2024

ItsDrike Jul 15, 2024

LiteApplication Jul 15, 2024

LiteApplication Jul 15, 2024

LiteApplication commented Jul 15, 2024

ItsDrike commented Jul 15, 2024

LiteApplication commented Jul 15, 2024

ItsDrike commented Jan 8, 2025 •

edited

Loading

Enable reporting unknown types #333

Are you sure you want to change the base?

Enable reporting unknown types #333

Conversation

ItsDrike commented Jul 13, 2024 • edited Loading

LiteApplication Jul 15, 2024

Choose a reason for hiding this comment

ItsDrike Jul 15, 2024

Choose a reason for hiding this comment

LiteApplication Jul 15, 2024

Choose a reason for hiding this comment

ItsDrike Jul 15, 2024

Choose a reason for hiding this comment

LiteApplication Jul 15, 2024

Choose a reason for hiding this comment

LiteApplication Jul 15, 2024

Choose a reason for hiding this comment

LiteApplication commented Jul 15, 2024

ItsDrike commented Jul 15, 2024

LiteApplication commented Jul 15, 2024

ItsDrike commented Jan 8, 2025 • edited Loading

ItsDrike commented Jul 13, 2024 •

edited

Loading

ItsDrike commented Jan 8, 2025 •

edited

Loading