Python: don't emit ClassVar for a file's top-level extensions - #29222
Open
folded wants to merge 1 commit into
Open
Python: don't emit ClassVar for a file's top-level extensions#29222folded wants to merge 1 commit into
folded wants to merge 1 commit into
Conversation
PrintExtensions is a template instantiated for both a Descriptor (extensions
nested in a message, printed into the class body) and a FileDescriptor
(top-level extensions, printed at module scope), and emitted _ClassVar[int]
for both. ClassVar is only a valid annotation inside a class body, so a proto
declaring a file-level extension produced a .pyi that does not type-check:
ext_pb2.pyi:7:1 - error: "ClassVar" is not allowed in this context
Give it the is_classvar parameter PrintEnumValues already uses for the same
distinction, and pass it only from the message call site.
Fixes protocolbuffers#29221
themis-mirror Bot
pushed a commit
to populationgenomics/themis
that referenced
this pull request
Aug 18, 2026
`typing.override` on every method that overrides one, and `reportImplicitOverride = "error"` so omitting it is an error rather than a habit. 65 methods across ports, fixture backends and servicers. ### Why An override that does not declare itself is one a base-method rename or removal leaves behind silently, still present and implementing nothing. The decorator turns that into a pyright error at the implementation. ### Why repo-wide Pyright's per-directory scoping (`executionEnvironments`) resolves against a `root` that is also the import-resolution root, so a narrow rule costs an `extraPaths` workaround — and it still only covers the tree you named. Every abc implementation in this repo overrides the same way, so there is no principled boundary short of all of them. A glob does not work either: `root = "**/servicer.py"` silently matches nothing and the rule quietly never fires, which is worse than not having it. ### exclude → ignore The rule can only see that a method overrides when the base class is **analysed**. `exclude` drops a file from analysis outright, so the generated servicer bases were invisible and the check passed vacuously on exactly the case it exists for. `ignore` analyses the file and suppresses only its own diagnostics. Net effect: `themis/rpc`, `themis/litcache/models`, `themis/workbench/models`, `themis/workbench/rpc` and `themis/services/sandbox_worker/_generated.py` go from unanalysed to checked. They are clean — the old comment claiming otherwise is stale. Two generated files still need their own diagnostics suppressed, both upstream codegen defects rather than anything fixable here: - `**/*_pb2_grpc.py` — the plugin emits a channel-less "simple stubs" class calling `grpc.experimental.unary_unary` without importing it. Nothing in this repo calls that class, and grpc considers the `AttributeError` the intended opt-in gate ([grpc#39555](grpc/grpc#39555), closed as working-as-intended; [grpc#43233](grpc/grpc#43233) proposes a `TYPE_CHECKING` import that would retire this entry). The `.pyi` beside it stays fully checked — that is where the servicer base and stub types come from. - `themis/rpc/sandbox_options_pb2.pyi` — protoc's `--pyi_out` annotates a top-level extension's field-number constant `ClassVar[int]` at module scope, where `ClassVar` is not valid ([protobuf#29221](protocolbuffers/protobuf#29221), fix in [protobuf#29222](protocolbuffers/protobuf#29222)). ### Two lint exemptions retire `N802` and `ARG002` were carried per-path for the servicers' PascalCase rpc names and the generated `(request, context)` signature. Ruff exempts an `@override` method from both, so the ignore lists for `themis/services/*/servicer.py` and the `N802`/`ARG002` entries under `themis/clients/auth/tests/**` are gone and `ruff check` is clean without them. A machine-checked decorator replaced two hand-maintained lists. `ruff --fix` also dropped a now-dead `# noqa: ARG002 — port signature` in `fixture_work_queue.py` for the same reason. ### Checked locally pyright 0 errors, `ruff check` clean, `629 passed, 7 skipped`, regen output unchanged, compat gate reports no incompatibility. Verified the rule actually fires by removing a decorator from `hello/servicer.py` and getting the error back. Not capturable — no rendered surface.
This file contains hidden or 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
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.
Fixes #29221.
Problem
PyiGenerator::PrintExtensionsis a template instantiated for two different scopes, and emits_ClassVar[int]for both:ClassVarvalidpyi_generator.cc:481PrintExtensions(message_descriptor)Descriptorpyi_generator.cc:656PrintExtensions(*public_dep)FileDescriptorpyi_generator.cc:669PrintExtensions(*file_)FileDescriptorPEP 526 restricts
ClassVarto class bodies, so a proto declaring a file-level extension generates a.pyithat does not type-check.Given:
before:
Only the module-scope constant is flagged; the nested one is correct as-is.
Change
PrintEnumValuesin the same file already carries abool is_classvar = falseparameter for exactly this distinction. This givesPrintExtensionsthe same parameter and passestrueonly from the message call site, so the twoFileDescriptorinstantiations fall through to a plainintannotation. No template specialisation needed.After:
The nested case is unchanged.
Notes
extend, not only extension-only files — the repro above also contains a message..pyigolden/expected outputs containing top-level extensions will need regenerating.