Skip to content

refactor(math): drop Polynomial operator impls unused by production#618

Open
diegokingston wants to merge 1 commit into
cleanup/math-dead-codefrom
cleanup/math-unused-poly-operators
Open

refactor(math): drop Polynomial operator impls unused by production#618
diegokingston wants to merge 1 commit into
cleanup/math-dead-codefrom
cleanup/math-unused-poly-operators

Conversation

@diegokingston
Copy link
Copy Markdown
Collaborator

Summary

Stacked on #598. Removes 6 Polynomial operator families from polynomial.rs that no production code reaches — proven by a clean cargo build --workspace.

Removed (319 lines, 24 impls):

  • Sub Polynomial − Polynomial
  • Mul Polynomial × FieldElement (and FieldElement × Polynomial)
  • Add Polynomial ± FieldElement (and FieldElement ± Polynomial)
  • Sub FieldElement − Polynomial

Of these, only Sub Poly−Poly had tests (2, self-referential); the other 5 families were fully dead with no test coverage at all.

Why these 4 families stay

Production uses operators at exactly 2 call sites, both building ∏(x − rᵢ):

  • boundary.rs:149zerofier * binomialMul Poly×Poly
  • transition.rs:127acc * (x_poly - current)Mul Poly×Poly, Sub Poly−FieldElement

Sub<FieldElement> for Polynomial is implemented as -monomial + self, so Neg and Add Poly±Poly remain as transitive dependencies. Kept families: Mul Poly×Poly, Sub Poly−FieldElement, Neg, Add Poly±Poly.

Test plan

  • cargo build --workspace — clean
  • cargo clippy -p math --all-targets -- -D warnings — clean
  • cargo test -p math — 196 passed (−2: the removed self-referential Sub Poly−Poly tests)

Removes 6 operator families from polynomial.rs that no production code
reaches — proven by a clean `cargo build --workspace`:

- Sub  Polynomial - Polynomial
- Mul  Polynomial * FieldElement   (and FieldElement * Polynomial)
- Add  Polynomial +/- FieldElement (and FieldElement +/- Polynomial)
- Sub  FieldElement - Polynomial

Production uses exactly two operator entry points, both when building
the product-of-linear-factors polynomial prod(x - r_i):
- boundary.rs:149   zerofier * binomial         -> Mul Poly x Poly
- transition.rs:127 acc * (x_poly - current)    -> Mul Poly x Poly,
                                                   Sub Poly - FieldElement

Sub<FieldElement> for Polynomial is implemented as `-monomial + self`,
so Neg and Add Poly+/-Poly remain as transitive dependencies; those four
families (Mul Poly x Poly, Sub Poly - FieldElement, Neg, Add Poly+/-Poly)
are kept.

Also removes the 2 self-referential Sub Poly-Poly tests and the
now-orphaned polynomial_b_minus_a test helper.
@github-actions
Copy link
Copy Markdown

Codex Code Review

Findings

  1. Potential bug: PR removes Sub implementations for Polynomial - Polynomial, but the merged code still uses that operator in transition.rs:
    Polynomial::new_monomial(FieldElement::<F>::one(), 1) - current.clone()
    With the impl removed from polynomial.rs, this should fail to compile. Keep the Sub<Polynomial<...>> impls or update the call sites to use an explicit supported operation.

Notes

I attempted cargo check -p stark, but this sandbox cannot write to rustup’s home temp directory, so compilation could not run here. The source-level break is direct: the exact Sub<Polynomial> impl existed at base polynomial.rs:266 and is gone in this PR, while the operator use remains.

@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

Review: LGTM with one merge-order note

Pure dead-code removal - no logic changes, no correctness issues, no security concerns.

Verified:

  • Both production call sites (boundary.rs zerofier * binomial and transition.rs acc * (x_poly - current)) use only the kept operator families (Mul Poly x Poly, Sub Poly-FieldElement).
  • The Sub that's kept correctly delegates to Neg + Add Poly+Poly, which are also kept - dependency chain is intact.
  • Removed tests were entirely self-referential to the removed impls.

Merge order: This PR is stacked on #598 (cleanup/math-dead-code), which is still open. Do not merge this until #598 lands on main and this branch is rebased, or the base ref will be wrong.

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