ci: reject field names that generated code cannot address - #1740
Open
u9g wants to merge 2 commits into
Open
Conversation
A field named `from` reached main and only surfaced days later as a red mypy gate in python-sdks, on an unrelated PR: protoc's --pyi_out cannot emit a Python keyword as a parameter name, so it drops the attribute and falls back to an untyped **kwargs. Nothing in this repo's CI could see it — protocol generates Go and JS, never Python. Two checks, because they answer to different authorities. api-linter's core::0140::reserved-words matches a maintained list of Java, JavaScript and Python 3 keywords; the stub check runs the generator and looks for the **kwargs it emits when it gives up on a name. api-linter's default rule set reports 413 problems on one of these files, so the config enables that single rule. Four fields predate the check and are suppressed by path in the config rather than with the in-proto comment, which protoc copies into every language's generated code.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flags a job that leaves GITHUB_TOKEN at its default permissions. The job reads the repo and uses the token only for setup-protoc's release downloads.
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
2 participants
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.
Follows #1738, which is already merged — this branch is rebased onto it, so the gate is green on
mainas it stands.fromreachedmainand surfaced days later as a red mypy gate in python-sdks, on a PR that had only bumped the submodule. Nothing here could have caught it: protocol generates Go and JS in CI, never Python, and Go is immune because its capitalization rules dodge the collision.Two checks
core::0140::reserved-words(api-linter) matches field names against a maintained list of Java, JavaScript and Python 3 keywords. Verified against the pre-rename proto — it flags both fields and exits 1:Only that one rule is enabled. api-linter's defaults encode Google's whole API style guide, which these protos don't follow — 413 problems on that one file. Note the flags
--disable-rule core --enable-rule core::0140::reserved-wordsdo not work, disable wins; it takes a config file.The Python stub check generates the stubs and greps for
**kwargs, which is what protoc emits when it cannot name a field's parameter. It answers to the generator instead of a list — worth having separately, because the generator's behaviour is subtler than any keyword list suggests. Probing it:from/finallydegrade to**kwargs; the soft keywordsmatch/typeare fine; and a field namedselfis handled by renaming the receiver toself_.The four fields that predate the check
Suppressed by path in
.api-linter.yaml, with reasons, rather than with the in-proto(-- api-linter: ... --)comment — protoc copies that comment into the generated code of every language, so four suppressions would have landed in the public API docs of every SDK:TrickleRequest.final,TranscriptionSegment.finalfinalFunctionCall.argumentsargumentsSIPCall.from(rpc/io.proto)fromFromheader.rpc/is Go-only, so it never reaches a Python stub — a rename is a prerequisite if that changes.The path suppression is file-scoped, so a new keyword field in one of those four files would slip past api-linter. Their Python side stays covered: the stub check takes no suppressions.
Both steps were run locally, green on this tree and exit 1 when
start_timeis reverted tofrom. No proto or generated-code changes — the diff is the config and the CI job.