refactor(hm-vm): SnapshotId exposes a public String field, allowing arbitrary IDs to be minted anywhere#119
Merged
markovejnovic merged 4 commits intoJun 10, 2026
Conversation
…rbitrary IDs to be minted anywhere
…xposes-a-public-string-field-11
… as_str Replace the inherent as_str method with an idiomatic impl AsRef<str>; call sites use as_ref() for &str args, Display for formatting, and a typed binding for the rusqlite param.
markovejnovic
deleted the
cq/hm-vm-snapshotid-exposes-a-public-string-field-11
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Smell
SnapshotIdwas declared aspub struct SnapshotId(pub String). The public tuple field meant any code could:SnapshotId(some_string)), and.0(done acrossdocker.rs,vm.rs, andregistry.rs).A domain handle that exposes its raw
Stringfor free construction and mutation carries no invariant and is interchangeable with any otherString, defeating the point of the newtype.Change
pub struct SnapshotId(String).SnapshotId::new(impl Into<String>)as the single construction entry point.as_str()accessor and aDeref<Target = str>impl for read access (the type already derivesDisplay).SnapshotId::new(...)and read via.as_str()instead of.0.Behavior is unchanged: every
.0read becomes.as_str()(same&str), and everySnapshotId(x)becomesSnapshotId::new(x)(same value). No public-API capability is lost — construction and read are still available, just routed through named methods.Pattern
This applies rust-analyzer's "distinct id newtypes per domain" doctrine: a domain handle should encapsulate its representation rather than expose it for free construction. rust-analyzer's
FileId,CrateId, and the various interned ids follow exactly this shape — opaque newtypes constructed through dedicated constructors, not barepubfields.Validation
Only
cargo check -p hm-vm(and--tests) was run locally, both pass. Full CI runs on this PR.This is a low-blast-radius change, but it is a draft pending review.