Skip to content

Delete StringId's relational operators so handle order and name order stop being the same three characters #185

Description

@typeless

Summary

StringId is enum class StringId : std::uint32_t, so < on it is the built-in enum comparison: handle order, i.e. the order strings were first interned. Nothing at any call site says whether a given comparison means "handle order, because this is a set" or "name order, because this order is observable". Both spellings are the same three characters, and the wrong one compiles silently.

The bug class this produces is now four instances deep:

site how it surfaced
src/parser/glob.cpp:258 #171%f followed readdir order, so command identity varied by machine (fixed, #174)
src/core/layout.cpp:221 #175 — variant hint listed in readdir order (fixed, #176)
src/exec/scheduler.cpp:62 vs :44 handle-sorted, name-searched — exported variables silently dropped from the child environment (open, filed separately)
src/cli/context.cpp:677 vs src/graph/builder.cpp:1305 handle-sorted, name-searched — import VAR loses its cached value, changing command identity (open, filed separately)

Two of these were found only because someone went looking, and the last two were audited and wrongly cleared in PR #176's own body. That is the signature of a conventional invariant: correctness depends on every author remembering, and reviewers cannot tell the two meanings apart by reading.

Proposal

Delete the relational operators and make each site state its meaning:

// include/pup/core/string_id.hpp
auto operator<(StringId, StringId) -> bool = delete;
auto operator>(StringId, StringId) -> bool = delete;
auto operator<=(StringId, StringId) -> bool = delete;
auto operator>=(StringId, StringId) -> bool = delete;

inline constexpr auto handle_less = [](StringId a, StringId b) {
    return to_underlying(a) < to_underlying(b);
};

A user-declared deleted operator for a scoped enum wins overload resolution over the built-in candidate — this does not require turning StringId into a class type. Verified on both compilers this project uses:

g++ -std=c++23      std::sort(v.begin(), v.end()) on Vec<StringId>  -> 3 "use of deleted function" diagnostics
clang++-18 -std=c++23                                               -> 6 diagnostics

and the intentional forms still compile on both:

std::ranges::sort(v, {}, [](StringId id) { return pool.get(id); });  // by name
std::ranges::sort(v, handle_less);                                    // by handle, stated
a == StringId::Empty;                                                 // equality untouched

Cost

Roughly 18 sites need one word added to say which order they mean. There are no std::map/std::set keyed on StringId in the codebase, and SortedIdVec/SortedPairVec take raw std::uint32_t, so they are unaffected. Pair comparisons that use the defaulted < on pair<StringId, ...> need an explicit comparator — that is the point, since src/cli/context.cpp:677 is exactly such a pair sort and is exactly one of the live bugs.

Why this and not more sorting

Sorting at each site (#174, #176) and at the platform boundary (#181) fixes instances. Neither prevents the next one, and neither would have caught the two live env-var bugs, because those are not enumeration-order bugs at all — they are handle-vs-name comparator mismatches, which no amount of sorting upstream can fix. This change moves the invariant one rung up CLAUDE.md's ladder, from convention to compiler-checked.

Provenance

Proposed by the completeness critic in an adversarial multi-agent review of #176/#180/#181, which also corrected my mistaken claim that deleting these operators required changing StringId to a class type. Compilation verified independently before filing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions