Skip to content

Python: don't emit ClassVar for a file's top-level extensions - #29222

Open
folded wants to merge 1 commit into
protocolbuffers:mainfrom
folded:pyi-no-classvar-for-toplevel-extensions
Open

Python: don't emit ClassVar for a file's top-level extensions#29222
folded wants to merge 1 commit into
protocolbuffers:mainfrom
folded:pyi-no-classvar-for-toplevel-extensions

Conversation

@folded

@folded folded commented Aug 17, 2026

Copy link
Copy Markdown

Fixes #29221.

Problem

PyiGenerator::PrintExtensions is a template instantiated for two different scopes, and emits _ClassVar[int] for both:

call site descriptor output scope ClassVar valid
pyi_generator.cc:481 PrintExtensions(message_descriptor) Descriptor class body yes
pyi_generator.cc:656 PrintExtensions(*public_dep) FileDescriptor module no
pyi_generator.cc:669 PrintExtensions(*file_) FileDescriptor module no

PEP 526 restricts ClassVar to class bodies, so a proto declaring a file-level extension generates a .pyi that does not type-check.

Given:

syntax = "proto2";
package probe;
import "google/protobuf/descriptor.proto";

extend google.protobuf.FileOptions {
  optional bool top_level_ext = 50001;
}

message Holder {
  extend google.protobuf.MessageOptions {
    optional bool nested_ext = 50002;
  }
}

before:

DESCRIPTOR: _descriptor.FileDescriptor
TOP_LEVEL_EXT_FIELD_NUMBER: _ClassVar[int]      # module scope
top_level_ext: _descriptor.FieldDescriptor

class Holder(_message.Message):
    __slots__ = ()
    NESTED_EXT_FIELD_NUMBER: _ClassVar[int]     # class body
    nested_ext: _descriptor.FieldDescriptor
    def __init__(self) -> None: ...
ext_pb2.pyi:7:1 - error: "ClassVar" is not allowed in this context (reportInvalidTypeForm)

Only the module-scope constant is flagged; the nested one is correct as-is.

Change

PrintEnumValues in the same file already carries a bool is_classvar = false parameter for exactly this distinction. This gives PrintExtensions the same parameter and passes true only from the message call site, so the two FileDescriptor instantiations fall through to a plain int annotation. No template specialisation needed.

After:

TOP_LEVEL_EXT_FIELD_NUMBER: int

The nested case is unchanged.

Notes

  • Affects any proto with a top-level extend, not only extension-only files — the repro above also contains a message.
  • .pyi golden/expected outputs containing top-level extensions will need regenerating.
  • I have not built or run the test suite locally; relying on CI for that.

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
@folded
folded requested a review from a team as a code owner August 17, 2026 12:34
@folded
folded requested review from anandolee and removed request for a team August 17, 2026 12:34
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: --pyi_out emits ClassVar[int] at module scope for file-level extensions

1 participant