Skip to content

engine: db::CompilationDiagnostic has no Debug impl, so every test drains it through .0.clone() #989

Description

@bpowers

Problem

CompilationDiagnostic (src/simlin-engine/src/db/diagnostic.rs:28) is a newtype over Diagnostic:

#[salsa::accumulator]
pub struct CompilationDiagnostic(pub Diagnostic);

Diagnostic itself derives Debug unconditionally (diagnostic.rs:39), but the newtype does not derive anything. So a test that drains the accumulator and wants to print what it got cannot {:?} the result -- it has to unwrap and clone through the tuple field first:

// src/simlin-engine/src/db/units.rs:817-819
check_model_units::accumulated::<CompilationDiagnostic>(&db, source, sync.project)
    .into_iter()
    .map(|cd| cd.0.clone())

// src/simlin-engine/src/db/stages_tests.rs:99-101
    .map(|cd| cd.0.clone())

accumulated yields &CompilationDiagnostic, so the .clone() is only there to reach a printable/comparable Diagnostic; the map exists solely to make the collection Debug-able for an assertion message.

Why it matters

Purely developer experience, but it is the kind of friction that gets copy-pasted: every new test that inspects accumulated diagnostics reproduces the same .map(|cd| cd.0.clone()) line, and a reader has to work out that the clone is not load-bearing. Two sites today; the number only grows as diagnostic coverage grows.

Possible approaches

  1. #[derive(Debug)] on the newtype, unconditionally. Diagnostic already pays for its own Debug in every build configuration, so the newtype's derive adds essentially nothing.
  2. #[cfg_attr(feature = "debug-derive", derive(Debug))], matching the convention ModelStage0/ModelStage1 use (src/simlin-engine/src/model.rs:53,76). Consistent with the crate's other big salsa values, but strictly less useful than (1) here since the inner type's Debug is unconditional -- and the affected call sites are tests, which per docs/tech-debt.md item 60 have never compiled without debug-derive anyway.

(1) looks right; (2) is worth a moment's thought only if there is a deliberate reason the accumulator type should be invisible in a no-debug-derive build.

Either way the follow-up is to delete the .map(|cd| cd.0.clone()) at the two sites above (and anywhere else a git grep 'cd\.0\.clone()' turns up).

Components affected

  • src/simlin-engine/src/db/diagnostic.rs (CompilationDiagnostic)
  • src/simlin-engine/src/db/units.rs (test helper at ~line 817)
  • src/simlin-engine/src/db/stages_tests.rs (test helper at ~line 99)

Severity

Low -- ergonomics only, no behaviour change.

Context

Noticed while implementing #966 (caching ModelStage0/ModelStage1 as salsa-tracked queries) on branch roundtrips-track-b, where db/stages_tests.rs had to reproduce the existing db/units.rs boilerplate verbatim.

Metadata

Metadata

Assignees

No one assigned

    Labels

    engineIssues with the rust-based simulation enginerustPull requests that update Rust codetech debt

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions