Skip to content

Stage 1: remove Pass 2's production imports of Pass 1 #1191

Closed

Description

@milindsrivastava1997

Why

After #1096 PR 1, Stage 1's Pass 1 no longer imports Pass 2 in production code, and a test enforces it. Pass 2 still imports Pass 1, so the two passes are not independent modules.

Remaining Pass 2 → Pass 1 production dependencies

  • PerComputationCandidateError (pass1/per_computation_candidates.rs) is in effect Stage 1's shared error type: pass2/asap_aware_cse.rs returns it from 6 functions, pass2/memo/enumeration.rs wraps it, and its TooManyCandidates variant is produced only by Pass 2. It is public API (re-exported from lib.rs).
  • pass2/asap_aware_cse.rs uses the Pass 1 helpers exact_accumulator and partitions_over.

Proposed change

  • Give Pass 2 its own error type, or move the shared error into a Stage 1 module both passes use (decide on the public name).
  • Move exact_accumulator and partitions_over into a shared module.
  • Extend the import-direction test to forbid pass2 → pass1 in production code too.

Test modules may keep using the other pass to build fixtures.

Follows #1096.

Activity

  1. milindsrivastava1997 commented on Nov 4, 2026

    @milindsrivastava1997
    CollaboratorAuthor

    Agreed scope

    1. Errors
      • Add a private CseError { Structure(#[from] SchemaDerivationError), Unsupported(&'static str), TooManyCandidates{..} } in pass2/asap_aware_cse.rs, replacing PerComputationCandidateError in Pass 2. No code matches its variants: callers only stringify it, drop the candidate, or call .ok().
      • Remove TooManyCandidates from the public PerComputationCandidateError (only Pass 2 produces it).
      • ErrorBuildingLogicalASAPDagsFromChosenCandidates::Recompose wraps SchemaDerivationError directly.
      • Pass2RuleError is unchanged. It is fatal and public, while most helper errors only drop one candidate.
    2. Helpers
      • Add a new crate-private src/ir_construction.rs holding partitions_over, column_ref and exact_family. Both passes import from it.
      • rolled_up builds its Sum/Min/Max SummaryAgg inline (reads the value column, no item, UnknownOrSigned), keeping the check that the column resolves. Pass 2 no longer calls exact_accumulator, which would otherwise pull ~300 lines of Pass 1 logic and its error type into a shared module.
    3. Layering test
      • One helper walks a pass's directory recursively and matches pass1:: / pass2:: in production lines (not a bare pass1, which Pass 2's entry uses as a parameter name).
      • New pass2_does_not_import_pass1, confirmed to fail on the current code first.
    4. Coverage
      • No new tests beyond the layering test, unless no existing test covers a Sum or Min/Max rolled_up.
      • Corpus snapshots must come out byte-identical.
    5. Out of scope: splitting the passes into crates, filed as Stage 1: split Pass 1 and Pass 2 into separate crates #1209.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions