-
Notifications
You must be signed in to change notification settings - Fork 3
Open
Labels
enhancementNew feature or requestNew feature or requestlow prioritywait, wait, wait, do I really need this?wait, wait, wait, do I really need this?
Description
Problem
Most model structs (~55) use #[derive(Deserialize)] which bypasses constructor validation (assert!/panic! in new()). When users load invalid JSON via pred solve/pred evaluate/pred reduce/pred inspect, the deserialization path (serde_json::from_value in declare_variants! factory) skips all checks, leading to:
- Runtime panics with unhelpful messages (e.g., index out of bounds in
SpinGlasswhenfields.len() != num_spins) - Silent invalid state (e.g.,
ExactCoverBy3Setswithuniverse_size % 3 != 0— silently unsolvable)
pred create is safe (constructs via new()). Only the JSON loading path is affected.
Already protected (~8 models)
Models with manual impl Deserialize, serde(from), or serde(try_from):
SequencingWithinIntervals,SequencingToMinimizeMaximumCumulativeCost,RectilinearPictureCompression,PartiallyOrderedKnapsack,SumOfSquaresPartition,MultipleChoiceBranching,StrongConnectivityAugmentation,BalancedCompleteBipartiteSubgraph
Proposed fix: #[serde(try_from)]
Replace #[derive(Deserialize)] with #[serde(try_from = "RawStruct")] + impl TryFrom:
#[derive(Serialize, Deserialize)]
#[serde(try_from = "FooRaw")]
pub struct Foo { ... }
#[derive(Deserialize)]
struct FooRaw { /* same fields */ }
impl TryFrom<FooRaw> for Foo {
type Error = String;
fn try_from(raw: FooRaw) -> Result<Self, String> {
// reuse validation logic from new(), return Result instead of panic
}
}Advantages over manual impl Deserialize:
- No hand-written Deserializer trait code
- Validation errors become proper serde errors (clean CLI messages)
- Shared validation function between
new()(unwrap/panic) andTryFrom(propagate)
Scope
~55 models need migration. Can be done incrementally per-category (graph/, misc/, set/, algebraic/, formula/).
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
enhancementNew feature or requestNew feature or requestlow prioritywait, wait, wait, do I really need this?wait, wait, wait, do I really need this?