chore(lint): static guards for the five recurring review-reject classes - #1830
Merged
Conversation
Turn five defect classes that reviewers keep rejecting by hand into static checks that run in the existing lint lane -- no new workflow, no new job. Measured on master first (graphistry/, tests excluded, matching mypy.ini): missing-annotations 1704 findings / 79 files explicit-any 1605 findings / 117 files explicit-cast 959 findings / 81 files bare-generic 209 findings / 48 files plottable-setattr 3 findings / 3 files plottable-attr-write 39 findings / 5 files vocab-str-param 42 findings / 21 files The first four are far too large to be errors today, so enforcement is a per-file count ratchet against bin/ci_type_hygiene_baseline.json: a file may not gain findings, and a file absent from the baseline must have zero. New and moved code is held to the rule; existing debt is grandfathered and shrinks. bin/ci_type_hygiene_guard.py is stdlib-only and runs from bin/lint.sh, so it executes on every interpreter in the python-lint-types matrix (3.8-3.14) in ~1.4s with byte-identical counts on all seven. Class 2 targets the hazard, not the syntax. A blanket getattr/setattr ban was measured and rejected -- 300 non-test getattr sites are overwhelmingly legitimate optional-attribute reads, and the id()-keyed cache leak of #1825 requires a *write*. So only writes onto a parameter annotated as a Plottable are flagged, in two forms: setattr() (3 sites, one of which is #1825 itself) and param.attr = ... (39 sites), the form the same hazard takes when nobody writes setattr. Ruff additionally gains B009/B010 as real errors, with today's 31 constant-name offenders across 14 files seeded into per-file-ignores. Class 5 ships the narrowest defensible rule rather than a plausible one. The general "this str should be a Literal" question was prototyped as a comparison heuristic, measured at ~80% precision over 44 hits, and deliberately not shipped -- a column name is legitimately str, and a rule needing manual triage every PR is worse than no rule. vocab-str-param instead knows exactly six parameter names (table/kind/direction/how/mode/engine) whose vocabularies this repo has already committed to as Literal aliases. Escape hatch is `# hygiene-ok: <check> -- <reason>` on the line, not raising a baseline cap. Conventions documented in DEVELOP.md "Type Hygiene Guard". No production code changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YYZRXegrALuXd3NHH5evqx
lmeyerov
added a commit
that referenced
this pull request
Jul 29, 2026
…giene baseline The per-file ratchet in bin/ci_type_hygiene_baseline.json is a SNAPSHOT, so it goes stale whenever a PR other than the one that owns it touches a baselined file. #1830 captured its baseline on a branch whose merge-base is b6181d3 -- before #1800, #1799 and #1816 landed -- and each of those three added cast() calls to a file #1830 had already pinned. All four were green on their own bases; the merged combination was not. The baseline is UNCHANGED. The findings are removed instead: - 3 of the 7 were never typing.cast. The guard matches any call named `cast`, including the attribute form, so pl.Expr.cast -- a polars RUNTIME dtype conversion -- counts as a typing finding. Those carry the documented `# hygiene-ok: explicit-cast` escape hatch with a reason. - The other 4 are real and are gone by DECLARATION rather than by call-site assertion. _two_hop_cached_equal_domain_degree_counts declares `counts: Tuple[DataFrameT, DataFrameT]` once, collapsing four casts into one localized `# type: ignore[assignment]` on the polars arm. _apply_connected_optional_match declares `seed_ids: SeriesT` / `node_ids: SeriesT`, since selecting one column off a frame is a Series on every engine. All three files now sit at or below baseline (18/18, 132/133, 41/42). Typing-only: typing.cast is the identity function at runtime, so every removal is provably value-preserving. The one restructured line binds an unchanged pl.Expr list to a name so the per-line suppression fits the 127-column limit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YYZRXegrALuXd3NHH5evqx
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.
What
Five defect classes that reviewers keep rejecting by hand are now checked statically, in the existing lint lane. No new workflow, no new job, no production code changed.
bin/ci_type_hygiene_guard.py— stdlib-only AST pass overgraphistry/(tests excluded, matchingmypy.ini), invoked frombin/lint.sh, so it runs inpython-lint-typeson py3.8–3.14. ~1.4 s, byte-identical counts on all seven interpreters (verified).bin/ci_type_hygiene_baseline.json— the per-file ratchet.pyproject.toml— ruffB009/B010promoted to errors, today's offenders seeded intoper-file-ignores.DEVELOP.md— conventions + escape hatch.CHANGELOG.md— entry.Measured first
Counts on
masterb6181d35,graphistry/excludinggraphistry/tests(same exclusionmypy.inialready uses; including tests would add ~5,000 missing-annotation findings from throwaway fixtures):missing-annotationsAnyexplicit-anycast()explicit-castbare-genericplottable-setattrplottable-attr-writestrvsLiteralvocab-str-paramB009/B010per-file-ignoresRatchet
Per-file count baseline: a file may not gain findings, and a file absent from the baseline must have zero. So new files are fully enforced, and new untyped functions inside existing files are caught too — which per-file-ignores alone would not do.
changed-files-onlywas considered and rejected:python-lint-typeschecks out atfetch-depth: 1, so no merge-base is available in that job, and I cannot author workflows to change it.Escape hatch is
# hygiene-ok: <check> -- <reason>on the line, not raising a cap via--update-baseline.Class 2 targets the hazard, not the syntax
A blanket
getattr/setattrban was measured and rejected: 300 non-testgetattrsites, overwhelmingly legitimate optional-attribute reads. The #1825 leak (setattrcache onto the caller'sPlottablekeyed byid(), returning a stale answer after in-place mutation) requires a write, so only writes onto a parameter annotated as aPlottableare flagged:plottable-setattr— 3 sites. One isgfql_fast_paths.py:514, i.e. GFQL: two-hop count returns a STALE answer after in-place frame mutation (degree-count memo setattrs onto the caller's Plottable, keyed by id() — the BLOCKER-1 pattern the same file forbids) #1825 itself; one isgfql_unified.py:1571; one iscluster.py:190(legitimate, dynamic name on a fresh copy).plottable-attr-write— 39 sites, theparam.attr = ...form the same hazard takes when nobody writessetattr. Without this thesetattrrule is one keystroke from being bypassed.Ruff
B009/B010additionally reject the constant-name forms outright; 31 offenders across 14 files are seeded and retire withruff check --fix --select B009,B010 <file>.Class 5 is deliberately narrow
There is no honest general rule for "this
strshould be aLiteral". I prototyped the obvious one — astrparameter compared against a small closed set of literals in its own body — and measured it: 44 hits at roughly 80% precision. The false positives are exactly the owner's carve-out (_alias_key(column: str),__getattr__(name: str),_is_bool_literal(text: str),_coerce_value(attr_type: str)reading arbitrary GEXF). Not shipped.What ships instead is a hard-coded registry of six parameter names whose vocabularies this repo has already committed to as
Literalaliases (table,kind,direction,how,mode,engine) — 42 sites, ratcheted. It is a floor, not a full check; reviewers still own the general case.What this will NOT catch
def f(x: str) -> strwherexis really a DataFrame is invisible.Anyreached through a type alias or an unparameterized import; only the literalAnytoken in an annotation is seen.h = g; h._x = 1) or a non-Plottable-annotated / unannotated parameter. The rule keys on the annotation, which is why it is precise; that precision is also its blind spot.--strictlists files that improved so caps can be retightened deliberately.Not done, on purpose
No
mypy.inistrictness flags.disallow_untyped_defs/disallow_any_explicit/disallow_any_genericsare per-module all-or-nothing, and with 79–117 dirty modules the exemption list would be ~the whole package, with no way to express "no worse than today". The AST guard gives per-file granularity in the same lane at ~1.4 s.Verification
./bin/lint.shgreen end to end (ruff + guard + relative-import check), exit 0.# hygiene-okverified to suppress.B009/B010verified to fire on new code despite the seeded ignores.Please do not merge without owner review — the baseline encodes a policy decision about what is grandfathered.