lattice-system

Refactoring conventions and review criteria

This document captures conventions and review criteria distilled from the project’s refactoring rounds. It is applied as the review checklist for every pull request, not only refactor PRs.

The knowledge is grouped by phase / category so that reviewers can locate the relevant criteria quickly.

1. Test methods (per refactor plan v4 §2.1)

The following table is the canonical menu of test idioms used in this project. Each PR that adds or changes tests should be checkable against it.

Method Code shape Use
A. decide-based universal example : ∀ x : Fin n, P x := by decide Pin the meaning of a finite, Decidable predicate. Most refactor-resistant.
B. matrix entry-wise on small Fin n ext i j; fin_cases i <;> fin_cases j <;> simp [defn_apply] Cross-check small matrix-valued definitions. Catches meaning changes immediately.
C. bridge identity example : defn_A = defn_B := rfl Pin the consistency of two definitions of the same object. Refactor detector.
D. signature-preservation shim example : <type> := named_thm args Limited to public API freeze. Survives rename only of the body, not of the name.
E. Plausible (POC only) Not primary. Generator integration cost is high for our / matrix setting.
F. #guard_msgs /-- expected msg -/ #guard_msgs in <command> Pin diagnostic output: @[deprecated] warnings, intended failures, linter behaviour. Not for theorem regression.
G. small exhaustive (Fin n; fin_cases) example : ∀ x : Fin n, P x := by intro x; fin_cases x <;> simp Pin meaning of a parameterised lemma on a small but complete sample. The most refactor-resistant for parameterised statements.

Primary axis: A + C + G. Limited use: D, B. Niche: F. Not primary: E.

For canonical examples of methods A / B / C / F / G in working code, see LatticeSystem/Tests/Foundation.lean — the test-method POC file from Phase 0 (#280). Each method has a labelled subsection with at least one minimal example.

Review check — tests

When a PR introduces or modifies tests, the reviewer checks:

2. Module split conventions (per refactor plan v4 §3.1)

Module split criteria, all four required:

  1. Two or more clearly separable responsibilities.
  2. Linear import DAG after split (no cycles).
  3. Public API boundary is clean.
  4. Tests already pin meaning (depends on Phase 1 of the refactor).

Façade module policy

Each split module retains a façade module that re-imports the new files. This preserves backward-compatible import paths:

LatticeSystem/Quantum/NeelState.lean  -- façade (re-imports NeelState.*)
LatticeSystem/Quantum/NeelState/Definition.lean
LatticeSystem/Quantum/NeelState/Definition2D.lean
...

Old import LatticeSystem.Quantum.NeelState works unchanged.

Façade variant: Core.lean sub-pattern (cycle avoidance)

When the original module’s bulk stays put and only a new feature file is extracted, the naive façade pattern would require the new feature file to import the (now-trimmed) original — and the original-as-façade would import the new feature file. That is a cycle.

The fix: rename the (trimmed) original to <Module>/Core.lean and create a fresh <Module>.lean façade that imports both <Module>/Core.lean and the new feature file:

LatticeSystem/Quantum/SpinDot.lean             -- façade (~30 lines)
LatticeSystem/Quantum/SpinDot/Core.lean        -- bulk of original
LatticeSystem/Quantum/SpinDot/Hamiltonian.lean -- new feature

Now Hamiltonian.lean imports SpinDot.Core (not SpinDot), and SpinDot.lean (façade) imports both. No cycle.

When to use: any time the new sub-file would need to import the original file’s content (which, if not renamed, would be the façade).

Origin: PR #317 (SpinDot/Hamiltonian extraction). See the cumulative roadmap history Phase 2 entries for cumulative usage.

Façade variant: content + extensions (no façade)

When the parent file already contains the core concept and the sub-file extends it with derived material (eigenvalue calculations, companion-theorem families, additional algebraic structure that doesn’t fit the parent’s responsibility), the parent stays as content (not turned into a façade) and the sub-file imports the parent. Downstream code that wants the extension content imports the sub-file directly. The parent’s module-header docstring must list the extension sub-files in a table.

LatticeSystem/Quantum/HeisenbergChain.lean             -- content
LatticeSystem/Quantum/HeisenbergChain/Eigenvalues.lean -- extension
LatticeSystem/Quantum/HeisenbergChain/Gibbs.lean       -- extension

Now Eigenvalues.lean and Gibbs.lean import HeisenbergChain, and HeisenbergChain.lean does not import them. Users who just need the basic chain Hamiltonian + Hermiticity import Quantum.HeisenbergChain; users who want the eigenvalue or Gibbs companion family import the specific sub-file. This keeps the parent file’s import surface small.

When to use vs Core.lean sub-pattern: use content + extensions when the parent’s identity as a content-bearing module is more important than convenience for downstream; use Core.lean when you want a single import to give the full module surface.

Examples in this codebase:

Helper visibility

When extracting a helper lemma from one file to another, if the helper is referenced by other downstream files, it must be lifted from private lemma to lemma at extraction time. Document the visibility change in the PR body.

Examples:

Review check — module split

3. Generic / dedup conventions (per refactor plan v4 §3.2)

When consolidating duplicated patterns:

Deprecation window

When generalising a definition, keep the specialised version with a @[deprecated <replacement> (since := "YYYY-MM-DD")] annotation. The deprecation window is at least one minor version of the project (or one feature-cluster of subsequent PRs). Concrete current policy: 6 months from since — see deprecations.html for the live tracking table, removal-PR checklist, and current entries.

Do not deprecate until all bridge lemmas are in place and verified.

When @[deprecated] cannot use a target name (because the generic replacement requires a lambda argument the deprecation syntax can’t express), use the message form @[deprecated "use the generic ... with the ... indicator ..." (since := "YYYY-MM-DD")] to give callers a concrete migration hint.

Internal companion theorems on the deprecated name continue to exist (they are the migration scaffolding). Suppress the deprecation linter for the block of companions immediately after the deprecated declaration with a single set_option linter.deprecated false and a comment explaining why. Tests that exercise the deprecated names for backward-compat coverage do the same at file level (test files exempt). The deprecation warning text itself is captured by #guard_msgs (method F) at the end of the test file.

Review check — generic / dedup

4. Naming and docstring conventions (per refactor plan v4 §3.3)

Review check — naming & docstring

5. Linter exception conventions (per refactor plan v4 §3.4)

Common rewrite patterns (use these instead of suppressing)

Review check — linter

6. Public-doc synchronisation (CLAUDE.local.md, longstanding)

For every PR adding def / theorem / lemma, the same PR must:

  1. Update the authoritative page in docs/formalization/legacy/ (Lean name + statement + file + citation). This is the interim update path until Issue #5228 performs the structured-data cutover.
  2. Update the tracking Issue or roadmap page if applicable.
  3. Update tex/proof-guide.tex if relevant.

This is enforced by review and not by CI.

Review check — public doc sync

6b. Verifying push before merge (incident-driven)

When chaining git commit && git push inside a single background shell command, the git push step can fail silently — for example due to a transient network error or an interrupted background task — without aborting the chain or surfacing the failure on subsequent commands. The PR API will then report a successful merge of an empty diff, silently dropping the intended changes.

Review check — push verification

Origin: PR #311 (intended JordanWigner Operators extraction) merged with empty diff; redone via PR #312.

7. Refactoring knowledge accumulation (this document)

This document is itself the single source of truth for review criteria. When new conventions emerge (e.g., from a Phase 2 split surfacing a new pitfall), the convention is added here in the same PR that demonstrates it.

The goal is that anyone reviewing a PR can apply this checklist mechanically and catch most regressions / drift.

History

2026-06-07 — t-J Theorem 11.26 chain dedup + build-speed checkpoint (Issue #4314)

Refactoring checkpoint at 17 feature PRs since the previous refactor (#4303): PRs #4304–#4313 (Prop 11.24 capstone) and #4315–#4321 (Theorem 11.26 half-filling: kinetic vanishing, exchange reduction, all-up spin-dot/ground, singlet annihilation, the Heisenberg-bond CAR identity, and the bond = ½ Δ†Δ / positive-semidefiniteness).

Code change (dedup): TJAllUpSpinDot.lean (#4317) had carried a private copy of fermionDownAnnihilation_commute_fermionSiteSpinMinus_of_ne; the same cross-site annihilation–site-spin commutator was later published in TJCrossSiteSpinCommute.lean (#4320). TJAllUpSpinDot.lean now imports TJCrossSiteSpinCommute and reuses the public lemma, removing the ~20-line duplicated CAR proof (and a redundant CrossSiteOfNe/FermionSiteSpin transitive import).

Build-speed evaluation: the 82 TJ*.lean modules total ≈ 8.3k lines, all small (≤ 282 lines; largest TJExchangeBondSum.lean 282, TJSpinSymmetry.lean 251). Single-file rebuilds are light (TJAllUpSpinDot ≈ 5 s incremental, ≈ 11 s with dependency replay), well under the historical split trigger (~2000–4000 lines / ~16 s). Zero linter warnings in the t-J chain. No split warranted; the only structural improvement this cycle is the dedup above. Cadence counter reset.