Support for optional fields in structs

[hermes85pl]

I am having a hard time trying to use this library for structs that have fields of types that are wrapped in Option. I feel like this enum is too omnipresent not to deserve some kind of dedicated support. Is it there and I am just mising some docs?

[frozenlib]

There is no support for the field wrapped in Option yet.

I think the support you need depends on how you string Option. If it's enough to make None an empty string, we might as well make it possible to use something like the try operator in the format string, as follows.

use parse_display::*;

#[derive(Display, FromStr)]
#[display("{x?},{y?}")]
struct Point {
    x: Option<u32>,
    y: Option<u32>,
}

let pt = Point {
    x: None,
    y: Some(10),
};
assert_eq!(pt.to_string(), ",10");

We might as well make it so that Option support can be applied by specifying it as an attribute, like this.

#[derive(Display, FromStr)]
#[display("{x}{y}")]
struct PointEx {
    #[display("x={};", option)]
    x: Option<u32>,
    #[display("y={};", option)]
    y: Option<u32>,
}

let pt = PointEx {
    x: None,
    y: Some(10),
};
assert_eq!(pt.to_string(), "y=10;");

[hermes85pl]

I was thinking more of something like this:

#[derive(Display)]
#[display("Got these: {a}{b}{c}")]
struct Whatever {
    #[display("a={}")]
    a: u32,
    #[display(", b={}")]
    b: Option<u32>,
    #[display(", c={}")]
    c: Option<u32>,
    d: Option<String>,
}

let pt = Whatever {
    a: 1,
    b: None,
    c: Some(7),
    d: ".".to_string(),
};
assert_eq!(pt.to_string(), "Got these: a=1, c=7.");

This way I would be able e.g. to surround the value stored in Option with a prefix and suffix if it's there, e.g. to put a comma or brackets and spaces, or it would render nothing if it is None.

It would be nice if Option would not have to be introduced to the library each time it is employed. In my opinion it is reasonable to render only the value from Some(value), or use a dedicated format string for the value if it is provided using display, or render nothing for None.

Having dedicated support for Option is nothing uncommon in libraries related to marshalling. However, looking at serde I can see that it supports also #[serde(skip_serializing_if = "path")] which is a nice feature that allows for providing some optional custom logic. See https://serde.rs/field-attrs.html for details.

[frozenlib]

I also think it would be more convenient to be able to handle fields of type Option without specifying option in the attribute.

The reason for using option in attributes in the previous example(https://github.com/frozenlib/parse-display/issues/19#issuecomment-963680567) was to allow both the value wrapped in the Option and the value of type Option itself to be used for formatting, such as the following.

use parse_display::*;

#[derive(Display)]
#[display("{err1} {err2}")]
struct Errs {
    #[display("err1={:?}")]
    err1: Option<std::io::ErrorKind>,
    #[display("err2={:?}", option)]
    err2: Option<std::io::ErrorKind>,
}

let e = Errs {
    err1: Some(std::io::ErrorKind::AddrInUse),
    err2: Some(std::io::ErrorKind::BrokenPipe),
};
assert_eq!(e.to_string(), "err1=Some(AddrInUse) err2=BrokenPipe");

However, since the values wrapped in Option are likely to be used more often for formatting than the Option type values themselves, I think it would be a good idea to make the one that uses the values wrapped in Option the default.

[hermes85pl]

From the example that you provided I can see that there is a potential issue with combining the reasonable defaults for Option (i.e. printing its value or empty string, as I believe we agreed in the above discussion) with printing using Debug (by specifying :?). Other than that I would expect to see something like this:

use parse_display::*;

#[derive(Display)]
#[display("{a} {b}")]
struct Errs {
    #[display("a={:?}")]
    a: Option<u32>,
    #[display("b={}")]
    b: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
};
assert_eq!(e.to_string(), "a=Some(1) b=2");

I think that personally I would not bother about using Debug that much, since if one really cares how a type is printed in the Display mode, they should implement it properly for that type.

Furthermore, I think that it would be nice to provide an option to specify an alternative text in case there is None, so that we would be able to achieve something like this:

use parse_display::*;

#[derive(Display)]
#[display("{a} {b}")]
struct Errs {
    #[display("a={}")]
    a: Option<u32>,
    #[display("b={}", alt="b=nil")]
    b: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: None,
};
assert_eq!(e.to_string(), "a=1 b=nil");

Perhaps that would address some of your concerns.

[frozenlib]

If we infer how to handle Option from the format string, I fear that the inference method will become more complicated and difficult to understand with the addition of future features.

For example, suppose we allow method calls with a format string(#11).

#[derive(Display)]
#[display("{path}")]
struct Log {
    #[display("path={.display()}")]
    path: Option<std::path::PathBuf>,
}

let p = Log {
    path: Some(std::path::PathBuf::from("/")),
};
assert_eq!(p.to_string(), "path=/");

In the example above, the display method cannot be distinguished from the macro as being the method of std::path::PathBuf or method of Option(an extension method added to Option by the user). This is similar to the situation with #[display("a={:?}")] a: Option<u32> in your example.

But unlike #[display("a={:?}")] a: Option<u32>, it is more appropriate to treat it as the method of std::path::PathBuf, not Option.

To make the specification easier to understand, I think it would be better to determine the default behavior based on whether the field type is Option or not. (If the type of the field is Option<T>, it always defaults to using values wrapped in Option.)

I think that personally I would not bother about using Debug that much, since if one really cares how a type is printed in the Display mode, they should implement it properly for that type.

I think so too.

I believe that the recommended way should be short code, and the unrecommended way should be long code. So, if we use the Debug trait for formatting, I think it is acceptable to need redundant code, like #[display("a={:?}", nowrap)] in the following example.

use parse_display::*;

#[derive(Display)]
#[display("{a} {b} {c}")]
struct Errs {
    #[display("a={:?}", nowrap)]
    a: Option<u32>,
    #[display("b={:?}")]
    b: Option<u32>,
    #[display("c={}")]
    c: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
    c: Some(3),
};
assert_eq!(e.to_string(), "a=Some(1) b=2 c=3");

[hermes85pl]

That example of yours looks fine to me and looks intuitively with the simple assumption that by default Some(value) as wrapped in Option gets unwrapped.

Personally I am always for convention over configuration and for having reasonable defaults. That said, although I understand your fears, I think that it is better to have a slightly more complex implementation of the macro rather than require the users to explicitly inform the macro that each field that uses Option should be treated as optional. And perhaps when the rules for handling Option are defined in a way that they are clear and obvious, the implementation could follow by being split into some kind of composable components with optional logic for handling Option when it gets in the way.

So at this point I would assume that Some(value) should be unwrapped by default (unless marked otherwise) and None should render as an empty string (unless an alternative string is provided).

And the below simple example should be possible too, right?

use parse_display::*;

#[derive(Display)]
#[display("{a:?} {b:?} {c}")]
struct Errs {
    #[display(nowrap)]
    a: Option<u32>,
    b: Option<u32>,
    c: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
    c: Some(3),
};
assert_eq!(e.to_string(), "Some(1) 2 3");

Personally I am not convinced to the name nowrap as in this context I would expect something opposite, i.e. nounwrap, to instruct the macro not to use the default behavior of unwrapping Some(value) from an Option field (as all the values in this example are wrapped anyway).

[frozenlib]

Personally I am always for convention over configuration and for having reasonable defaults.

I think so too.

However, I believe that users will be confused if there are similar but different conventions and default values. And the example that I was concerned would confuse is exactly the code you showed.

use parse_display::Display;

#[derive(Display)]
#[display("{b:?}")]
struct X {
    b: Option<u32>,
}
let x = X { b: Some(2) };
assert_eq!(x.to_string(), "2");

Looking at https://github.com/rust-lang/rust/pull/90473, it seems that the Rust compiler's format_args_capture feature will be stabilized in the near future, so I think users will eventually come to associate code like the above with code like the below.

let b: Option<u32> = Some(2);
assert_eq!(format!("{b:?}"), "Some(2)");

Thus, it looks strange to have different results with similar code.

So, I came up with the following specification.

• Format strings in #[display] attribute for struct behave as if the field has a different Display implementation than usual.
  • The default behavior is as follows
  • If the field type is Option, it will be an empty string if it is None, or use the Display implementation of value if it is Some(value).
  • If the field's type is not Option, use the Display implementation of the field's type.
  • The #[display] attribute for the field can change the field's vritual Display implementation (In the ways discussed so far.).
• Since the above process is for Display, it does not affect other format traits such as Debug.
  • In other words, if you specify #[display("{b:?}")] for the struct as in the previous example, Option will not be unwrapped because Debug is used instead of Display for the field.
• Support for the ? operator (https://github.com/frozenlib/parse-display/issues/19#issuecomment-963680567) when referring to fields in a format string to make it easier to handle Option.

With this specification, your example would be written as follows.

use parse_display::*;

#[derive(Display)]
#[display("{a:?} {b?:?} {c}")]
struct Errs {
    a: Option<u32>,
    b: Option<u32>,
    c: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
    c: Some(3),
};
assert_eq!(e.to_string(), "Some(1) 2 3");

Personally I am not convinced to the name nowrap as in this context I would expect something opposite, i.e. nounwrap, to instruct the macro not to use the default behavior of unwrapping Some(value) from an Option field (as all the values in this example are wrapped anyway).

Indeed, you are right.

I also thought about required, option = false, etc., but I couldn't think of a word that would fit, so I decided to use nowrap, even though I thought it was strange.

If there is a more fit word, I would like to use that one. If possible, I like to use the keywords used in other crates.

It could be nounwrap, but I'm a little concerned about the fact that nounwrap is not done in FromStr, since it affects FromStr as well as Display.

[hermes85pl]

Thus, it looks strange to have different results with similar code.

Although I agree with where you come from with your approach in regards to code predictability, I cannot find such an issue here, as in the example that I provided a field was clearly marked with #[display(nowrap)].

How is

use parse_display::*;

#[derive(Display)]
#[display("{a:?} {b:?} {c}")]
struct Errs {
    #[display(nowrap)]
    a: Option<u32>,
    b: Option<u32>,
    c: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
    c: Some(3),
};
assert_eq!(e.to_string(), "Some(1) 2 3");

which actually typically would be replaced with

use parse_display::*;

#[derive(Display)]
#[display("{a} {b} {c}")]
struct Errs {
    #[display("{:?}", nowrap)]
    a: Option<u32>,
    b: Option<u32>,
    c: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
    c: Some(3),
};
assert_eq!(e.to_string(), "Some(1) 2 3");

less readable and more confusing than

use parse_display::*;

#[derive(Display)]
#[display("{a:?} {b:?} {c}")]
struct Errs {
    a: Option<u32>,
    #[display("{?:?}"]
    b: Option<u32>,
    c: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
    c: Some(3),
};
assert_eq!(e.to_string(), "Some(1) 2 3");

or some other strange things one could write?

Having that said, I remain with my opinion that by default Option should be transparent as it is e.g. with serde among other popular libraries for the sake of simplicity with reasonable defaults and I kindly suggest that you reconsider taking this approach.

[frozenlib]

Although I agree with where you come from with your approach in regards to code predictability, I cannot find such an issue here, as in the example that I provided a field was clearly marked with #[display(nowrap)].

It is not field a marked with #[display(nowrap)] that I feel uncomfortable with, but field b. It would be more natural if the output was the same as if the same format string was used in the format!() macro, as shown below.

#[derive(Display)]
#[display("{b:?}")]
struct Errs {
    b: Option<u32>,
}
let b: Option<u32> = Some(2);
let x = Errs { b };
assert_eq!(format!("{b:?}"), "Some(2)");
assert_eq!(Errs { b }.to_string(), "Some(2)");

The goal of the proposal in https://github.com/frozenlib/parse-display/issues/19#issuecomment-968201058 is to make this happen, while still being able to write like the second code example in https://github.com/frozenlib/parse-display/issues/19#issuecomment-968273096. So, it is not that the second code example in https://github.com/frozenlib/parse-display/issues/19#issuecomment-968273096 is less readable.

This proposal assumes that the following codes are all equivalent.

#[derive(Display)]
#[display("{a} {b} {c}")]
struct Errs {
    #[display("{:?}", nowrap)]
    a: Option<u32>,
    #[display("{:?}")]
    b: Option<u32>,
    c: Option<u32>,
}

#[derive(Display)]
#[display("{a} {b} {c}")]
struct Errs {
    #[display("{:?}", nowrap)]
    a: Option<u32>,
    #[display("{:?}")]
    b: Option<u32>,
    #[display("{}")]
    c: Option<u32>,
}

#[derive(Display)]
#[display("{a} {b} {c}")]
struct Errs {
    #[display("{:?}", nowrap)]
    a: Option<u32>,
    #[display("{?:?}", nowrap)]
    b: Option<u32>,
    #[display("{?}", nowrap)]
    c: Option<u32>,
}

#[derive(Display)]
#[display("{a:?} {b?:?} {c?}")]
struct Errs {
    a: Option<u32>,
    b: Option<u32>,
    c: Option<u32>,
}

let e = Errs {
    a: Some(1),
    b: Some(2),
    c: Some(3),
};
assert_eq!(e.to_string(), "Some(1) 2 3");

It might be a good idea to not implement ? operator in the format string, and only allow it to be written the first style.