Skip to content

ModCodeRepr implements conflicting Ord and PartialOrd orderings #673

Description

@SuhasSrinivasan

ModCodeRepr implements conflicting Ord and PartialOrd orderings

Summary

ModCodeRepr derives Ord but manually implements PartialOrd with the opposite ordering for mixed letter-code and numeric-ChEBI variants. The same pair can therefore compare differently depending on whether a caller uses cmp, partial_cmp, or the corresponding comparison operators.

Severity

Severity: Low — latent logical/API consistency

Rationale: This violates Rust's documented ordering-law expectation and can make an external or future mixed-code sort depend on the comparator used. Current in-tree production consumers use Ord-based sorting and do not exercise the conflicting mixed-kind PartialOrd, so no current biological result or file is known to be wrong.

User and scientific impact

  • Affected result or workflow: Rust callers comparing a letter modification code with a numeric ChEBI identifier through PartialOrd, <, >, or sort_by(partial_cmp).
  • Direction of error: comparator-dependent reversal of mixed-code order.
  • Likely exposure: uncommon/latent in modkit itself; possible for external consumers because ModCodeRepr is public.
  • Detectability or workaround: callers can use Ord::cmp explicitly, but should not need to know that the two trait implementations conflict.

Affected versions and environment

  • Released version: modkit 0.6.4.
  • Development revision: 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c.
  • Operating system and architecture: platform independent.
  • Rust toolchain used for verification: rustc/cargo 1.90.0.

Steps to reproduce

No sequencing input is required. Add this focused test in modkit-core/src/mod_base_code.rs, where the enum variants are directly accessible:

#[test]
fn mod_code_ordering_obeys_total_order_laws() {
    let code = ModCodeRepr::Code('m');
    let chebi = ModCodeRepr::ChEbi(1);

    assert_eq!(code.cmp(&chebi), std::cmp::Ordering::Less);
    assert_eq!(
        code.partial_cmp(&chebi),
        Some(std::cmp::Ordering::Less)
    );
}

Then run:

cargo test -p mod_kit mod_code_ordering_obeys_total_order_laws

Observed behavior

On the affected parent, derived Ord returns Less for Code('m') versus ChEbi(1), while the manual PartialOrd implementation returns Some(Greater). The second assertion fails.

A comparator-dependent rendering can also be seen with:

let mut codes = vec![
    ModCodeRepr::ChEbi(2),
    ModCodeRepr::Code('m'),
    ModCodeRepr::ChEbi(1),
    ModCodeRepr::Code('a'),
];
codes.sort_by(|left, right| left.partial_cmp(right).unwrap());

The affected PartialOrd sorts numeric identifiers before letter codes, whereas ordinary codes.sort() uses Ord and sorts letter codes first.

Control or independent oracle

For a type implementing both total-order traits, every representative pair must satisfy:

left.partial_cmp(&right) == Some(left.cmp(&right))

Antisymmetry and transitivity must also hold across letter/letter, numeric/numeric, and mixed pairs.

Expected behavior

Ord and PartialOrd implement the same total order. Existing Ord, sort, and BTree behavior remains letter codes first (lexicographically), followed by numeric ChEBI identifiers (numerically).

Root-cause evidence

  • modkit-core/src/mod_base_code.rs: derived enum Ord follows variant declaration order (Code before ChEbi), while the manual mixed-variant PartialOrd arms explicitly return the reverse.
  • The parent fails the mixed-pair law assertion above. Representative pair/triple law checks pass when PartialOrd delegates to Ord.

Proposed fix scope

Delegate PartialOrd::partial_cmp to Ord::cmp and add representative pair, triple, and rendering controls.

Non-goals

  • No chemical, ontology, or biological ranking is implied; this is a deterministic presentation/API order.
  • No change to same-kind letter or numeric ordering.
  • No change to existing Ord/sort/BTree output.
  • No Summary, sampling, threshold, parsing, or performance change.

Acceptance criteria

  • partial_cmp(a, b) == Some(cmp(a, b)) for representative letter, numeric, and mixed pairs.
  • Equality consistency, antisymmetry, and transitivity checks pass.
  • Existing Ord-based output remains letter codes before numeric ChEBI identifiers.
  • The compatibility impact on mixed-kind PartialOrd callers is documented.
  • Focused tests and the applicable full workspace test suite pass.

Reproduction artifacts

No external artifacts are required.

Related work

  • Related issues: none found.
  • Proposed PR: to be linked once the completed branch has been rebased and regated.
  • Deferred policy question or upstream dependency: the separate deterministic Summary-output issue uses the same presentation order locally but does not require this global trait change; dependency-resolution PR Pin hts-sys to bindings compatible with rust-htslib 0.46 #666 affects publication timing only.

Metadata

Metadata

Assignees

No one assigned

    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