diff-struct icon indicating copy to clipboard operation
diff-struct copied to clipboard

Incorrect handling of optional fields with String

Open ghostinushanka opened this issue 4 years ago • 1 comments

Hello @BenHall-7 ,

Playing with the lib I've come across inconsistent behavior for structs containing String types. Example:

use diff::Diff;

#[derive(Debug, PartialEq, Diff)]
#[diff(attr(
    #[derive(Debug, PartialEq)]
))]
struct OptionalInteger {
    x: i32,
    ox: Option<i32>,
}

#[derive(Debug, PartialEq, Diff)]
#[diff(attr(
    #[derive(Debug, PartialEq)]
))]
struct OptionalString {
    x: String,
    ox: Option<String>,
}

fn main() {
    let empty_integer = OptionalInteger { x: 42, ox: None };
    let filled_integer = OptionalInteger { x: 42, ox: Some(42)};
    let di = empty_integer.diff(&filled_integer);
    println!("{:#?}", di);

    let empty_string = OptionalString { x: "42".to_string(), ox: None};
    let filled_string = OptionalString { x: "42".to_string(), ox: Some("42".to_string())};
    let ds = empty_string.diff(&filled_string);
    println!("{:#?}", ds);
}

Expected output:

OptionalIntegerDiff {
    x: 0,
    ox: Some(
        42,
    ),
}
OptionalStringDiff {
    x: None,
    ox: Some(
        "42",
    ),
}

Actual output (note how instead of Some(String) we're getting Some(Some(String))

OptionalIntegerDiff {
    x: 0,
    ox: Some(
        42,
    ),
}
OptionalStringDiff {
    x: None,
    ox: Some(
        Some(
            "42",
        ),
    ),
}

Make it Option<Vec<String>> (or other compound type for that matter) and the end result is even more interesting.

ghostinushanka avatar Dec 29 '21 10:12 ghostinushanka

Hey, thanks for filing the issue! This one seems pretty interesting, in that it doesn't seem like there's necessarily a bug, but a better type definition for either or both of these types would help. In this case, the diff between None -> Some(T) is Some(T::Diff), but it's probably preferable to just let that be Some(T) with a cloned value. Although I need to consider the cases that yields when applying other diffs to this type

benhall-7 avatar Dec 31 '21 12:12 benhall-7