Skip to content

chore(lint): static guards for the five recurring review-reject classes - #1830

Merged
lmeyerov merged 1 commit into
masterfrom
chore/type-hygiene-static-guards
Jul 29, 2026
Merged

chore(lint): static guards for the five recurring review-reject classes#1830
lmeyerov merged 1 commit into
masterfrom
chore/type-hygiene-static-guards

Conversation

@lmeyerov

Copy link
Copy Markdown
Contributor

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 over graphistry/ (tests excluded, matching mypy.ini), invoked from bin/lint.sh, so it runs in python-lint-types on 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 — ruff B009/B010 promoted to errors, today's offenders seeded into per-file-ignores.
  • DEVELOP.md — conventions + escape hatch. CHANGELOG.md — entry.

Measured first

Counts on master b6181d35, graphistry/ excluding graphistry/tests (same exclusion mypy.ini already uses; including tests would add ~5,000 missing-annotation findings from throwaway fixtures):

Class Check Findings Files Enforcement
1 missing contracts missing-annotations 1704 79 ratchet
3a Any explicit-any 1605 117 ratchet
3b cast() explicit-cast 959 81 ratchet
4 bare generics bare-generic 209 48 ratchet
2 dynamic attr write plottable-setattr 3 3 effectively a hard rule
2 direct attr write plottable-attr-write 39 5 effectively a hard rule
5 str vs Literal vocab-str-param 42 21 narrow ratchet
2 constant-name attr ruff B009/B010 31 14 error today, seeded per-file-ignores

Ratchet

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-only was considered and rejected: python-lint-types checks out at fetch-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/setattr ban was measured and rejected: 300 non-test getattr sites, overwhelmingly legitimate optional-attribute reads. The #1825 leak (setattr cache onto the caller's Plottable keyed by id(), returning a stale answer after in-place mutation) requires a write, so only writes onto a parameter annotated as a Plottable are flagged:

Ruff B009/B010 additionally reject the constant-name forms outright; 31 offenders across 14 files are seeded and retire with ruff check --fix --select B009,B010 <file>.

Class 5 is deliberately narrow

There is no honest general rule for "this str should be a Literal". I prototyped the obvious one — a str parameter 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 Literal aliases (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

  • Wrong annotations. Every check is presence/shape only — def f(x: str) -> str where x is really a DataFrame is invisible.
  • Annotation churn inside a baselined file: delete one untyped function, add another, count unchanged.
  • Any reached through a type alias or an unparameterized import; only the literal Any token in an annotation is seen.
  • Plottable writes through a local alias (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.
  • Class 5 at large — see above.
  • The baseline can rot: shrinkage only warns by default. --strict lists files that improved so caps can be retightened deliberately.

Not done, on purpose

No mypy.ini strictness flags. disallow_untyped_defs / disallow_any_explicit / disallow_any_generics are 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.sh green end to end (ruff + guard + relative-import check), exit 0.
  • Guard verified to fire on a synthetic file for all 7 checks, and # hygiene-ok verified to suppress.
  • B009/B010 verified to fire on new code despite the seeded ignores.
  • Counts confirmed identical on py3.8, 3.9, 3.10, 3.11, 3.12, 3.13, 3.14.

Please do not merge without owner review — the baseline encodes a policy decision about what is grandfathered.

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
lmeyerov merged commit 6863879 into master Jul 29, 2026
77 checks passed
@lmeyerov
lmeyerov deleted the chore/type-hygiene-static-guards branch July 29, 2026 00:12
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
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.

1 participant