Trait Default Method Missing Override: Silent No-Op on Data Mutation

Trait Default Method Missing Override

Problem

The TypeRepository trait in crates/application/src/type_def/services.rs had a method clear_default_layout_references() with a no-op default implementation that returns Ok(()). This was intended for backward compatibility so that existing implementations would not break when the method was added. However, the SqliteTypeRepository in crates/infrastructure/sqlite/src/workspace/type_repository.rs never overrode it.

When DeleteLayoutUseCase called type_repository.clear_default_layout_references(layout_id), the no-op default silently succeeded, leaving dangling default_layout_id references in the types table pointing to a layout that no longer existed.

Root Cause

Trait default methods are invisible to the compiler. There is no warning when an implementation does not override a method that has a default body. In Clean Architecture with separate crates, the gap is even harder to spot during code review: the trait lives in application and the implementation lives in infrastructure, often in different files reviewed by different people or at different times.

The default was added as a convenience to avoid breaking existing code, but the intent was always for SqliteTypeRepository to provide a real implementation. The no-op default masked the missing override, and the bug was only discovered during a deep review of the INK-235 layout system.

Symptoms

• Deleting a layout definition succeeds without error
• Types that referenced the deleted layout still have default_layout_id pointing to the deleted layout’s UUID
• Attempting to apply the “default layout” for such a type results in a lookup failure or stale data
• All unit tests pass because mocks inherit the same no-op default
• Only integration tests with real SQLite would catch the discrepancy

Solution

1. Override the method in SqliteTypeRepository

fn clear_default_layout_references(
    &self,
    workspace_path: &Path,
    layout_id: Uuid,
) -> TypeResult<()> {
    let db = Self::open_db(workspace_path)?;
    db.with_writer(|conn| {
        conn.execute(
            "UPDATE types SET default_layout_id = NULL WHERE default_layout_id = ?1",
            params![layout_id.to_string()],
        )
        .map_err(map_db_error)?;
        Ok(())
    })
}

2. Add a test verifying the behavior

#[test]
fn test_clear_default_layout_references() {
    // Create types referencing a layout
    // Call clear_default_layout_references with that layout's ID
    // Verify all references are set to NULL
}

Prevention

When adding trait methods with defaults

• Immediately check all implementations and add the override in the same PR. Do not rely on a follow-up.
• Use doc comments to indicate whether a default is intentionally permanent (truly optional behavior) or a placeholder awaiting real implementations.
• Prefer adding trait methods WITHOUT defaults when possible. This forces compile-time errors for every implementation that has not been updated, which is exactly what you want for data integrity operations.
• Only use defaults for truly optional behavior such as logging, metrics, or diagnostic hooks. Never use a default Ok(()) for a method that performs a data mutation.

During code review

• When reviewing a PR that adds a trait method with a default body, always ask: “Which implementations need to override this?”
• When reviewing a PR that adds a new use case calling a trait method, verify the method is not relying on a no-op default in the production implementation.
• Search for all impl TraitName for blocks across crates to confirm coverage.

Warning signs in the codebase

• A trait method with a default body of Ok(()) that is semantically a data mutation (DELETE, UPDATE, INSERT)
• A use case that calls a trait method but the observable behavior in production appears to be a no-op
• Tests that pass because mocks inherit the default rather than exercising the real implementation
• Trait methods added in one PR with the real implementation deferred to a “follow-up”

Audit pattern

Search for trait methods with no-op defaults that should have real implementations:

# Find trait methods returning Ok(()) as default — potential missing overrides
rg "fn \w+\(" --type rust -A 5 crates/application/ | grep -B 3 "Ok(())"

# Cross-reference: find all impl blocks for a given trait
rg "impl.*TypeRepository.*for" --type rust

Additional Example: SideEffectContext::try_send_fts_rebuild

SideEffectContext::try_send_fts_rebuild() in apps/desktop/src-tauri/src/side_effects.rs had a no-op default that silently suppressed FTS rebuild events for any future SideEffectContext implementor that forgot to override it. McpState was using this default (as noted by a comment: “uses default — McpState has no FTS rebuild worker”).

The risk: adding a third implementor (e.g., an HTTP bridge state or a test double) would silently inherit the no-op, causing FTS to stop updating for that context with no compile-time warning.

Fix (INK-788): Removed the default body entirely, making try_send_fts_rebuild a required method. McpState now carries an explicit empty implementation with a doc comment explaining why it is intentionally a no-op:

// Before: silent trait default — new implementors silently skip FTS
fn try_send_fts_rebuild(&self, event: FtsRebuildEvent) {
    let _ = event; // no-op default
}

// After: required method — each implementor is forced to make a conscious choice
fn try_send_fts_rebuild(&self, event: FtsRebuildEvent);

// McpState explicit no-op (now visible and documented at the impl site)
/// McpState has no FTS rebuild worker — FTS updates are driven by the
/// desktop app's background task, not the MCP server. Intentional no-op.
fn try_send_fts_rebuild(&self, _event: FtsRebuildEvent) {}

This pattern applies to any side-effect dispatch method: prefer required methods so that every implementor is an explicit, reviewable decision rather than an inherited default.

References

• INK-235: Layout definition and rendering (deep review finding)
• INK-310: Associate default layout with type definitions (where the trait method was added)
• INK-788: Remove no-op default from FTS rebuild trait method
• Commit: b376190 (feat(types): associate default layout with type definitions)