search

nipunn1313 / mypy-protobuf

open source tools to generate mypy stubs from protobufs
Apache License 2.0
647 stars 80 forks source link

`google.protobuf.any_pb2.global__Any` is not marked as `TypeAlias` #600

Closed Avasam closed 6 months ago

Avasam commented 6 months ago

https://github.com/nipunn1313/mypy-protobuf/pull/321 added the TypeAlias annotation for class.V, but using stubs generated by typeshed, there's still a single Y026 (https://github.com/PyCQA/flake8-pyi/blob/main/ERRORCODES.md / https://docs.astral.sh/ruff/rules/type-alias-without-annotation/) violation in https://github.com/python/typeshed/blob/7d56cd9a6cf6e0a4ea89c68d0397e197aff32cbe/stubs/protobuf/google/protobuf/any_pb2.pyi#L177

nipunn1313 commented 6 months ago

yep! Would accept a fix here! I hadn't seen this lint error before. Is it relatively new?

nipunn1313 commented 6 months ago

I am curious why this line https://github.com/nipunn1313/mypy-protobuf/blob/7506de3f39469e1a6665f1bf388f506a40571996/test/generated/testproto/test3_pb2.pyi#L209

does not cause this lint to fire? It seems like it should also cause the lint to fire - and we would have caught that.

Avasam commented 6 months ago

I hadn't seen this lint error before. Is it relatively new?

It's existence in Flake8-pyi not new and has been enabled by default in early 2022: https://github.com/PyCQA/flake8-pyi/pull/101 It's been added to Ruff inv0.0.279 last year https://github.com/astral-sh/ruff/pull/5844

If you mean that it is raised in typeshed, also not new (same timeline as mentioned above), Y026 is just currently disabled in _pb2.pyi files (as with other non-autofixable lint issues for autogenerated files)[^1]

After bumping mypy-protobuf to v3.6.0 in typeshed I just noticed that in typeshed's usage, that was the last violation of that rule left.

I am curious why this line [...] does not cause this lint to fire? It seems like it should also cause the lint to fire - and we would have caught that.

This should hopefully answer that question: https://github.com/PyCQA/flake8-pyi/blob/f60d7e3cdc833dd358b493d2ca80fd187cc38cfb/pyi.py#L1202-L1216

[^1]: Ruff supports autofixes for PYI026, but I believe typeshed is waiting on https://github.com/python/mypy/issues/16581 or https://github.com/plinss/flake8-noqa/pull/30 to avoid conflicting error codes.

nipunn1313 commented 6 months ago

this seems relevant

        The only exceptions are the type aliases `X = (typing.)Any`
        (special-cased, because it is so common in a stub),
        and `X = None` (special-cased because it is so special).

I would argue that its actually false-positive in https://github.com/python/typeshed/blob/7d56cd9a6cf6e0a4ea89c68d0397e197aff32cbe/stubs/protobuf/google/protobuf/any_pb2.pyi#L177 - since that Any is not typing.Any, but rather a class Any.

But in any case, we can still aim to fix. Adding a TypeAlias annotation seems wholly reasonable.

Avasam commented 6 months ago

[...] - since that Any is not typing.Any, but rather a class Any.

You're right, I didn't look further to see it's not typing.Any. I guess the question is then, are global___* meant to be Type Alias or Variable Alias (you seem to be saying they should be type aliases anyway).

# global prefix globals are generated, but aren't used at runtime ._pb2.global___.

Output Format [...]

  • Only mangle-alias top-level identifiers w/ global___ to avoid conflict w/ fields of same name [previously was mangling inner messages as well]