Using both try_setter and setter(strip_option) does not compile

[chanced]

Using both try_setter and setter(trip_option) results in the error expected enum Option, found struct String.

version: 0.12.0
repro:

#[derive(derive_builder::Builder)]
struct Example {
    #[builder(try_setter, setter(strip_option))]
    value: Option<String>,
}

error[E0308]: mismatched types
   --> src/lib.rs:3:10
    |
3   | #[derive(Builder)]
    |          ^^^^^^^
    |          |
    |          expected enum `Option`, found struct `String`
    |          arguments to this enum variant are incorrect
    |
    = note: expected enum `Option<String>`
             found struct `String`

https://github.com/chanced/derive_builder_issue

[TedDriggs]

Can you provide the output of running cargo expand on this input? You may need to install the crate if you haven't used the tool before.

[chanced]

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
struct Example {
    #[builder(try_setter, setter(strip_option))]
    value: Option<String>,
}
#[allow(clippy::all)]
/**Builder for [`Example`](struct.Example.html).
*/
struct ExampleBuilder {
    value: ::derive_builder::export::core::option::Option<Option<String>>,
}
#[automatically_derived]
#[allow(clippy::all)]
impl ::core::clone::Clone for ExampleBuilder {
    #[inline]
    fn clone(&self) -> ExampleBuilder {
        ExampleBuilder {
            value: ::core::clone::Clone::clone(&self.value),
        }
    }
}
#[allow(clippy::all)]
#[allow(dead_code)]
impl ExampleBuilder {
    #[allow(unused_mut)]
    pub fn value(&mut self, value: String) -> &mut Self {
        let mut new = self;
        new
            .value = ::derive_builder::export::core::option::Option::Some(
            ::derive_builder::export::core::option::Option::Some(value),
        );
        new
    }
    pub fn try_value<VALUE: ::derive_builder::export::core::convert::TryInto<String>>(
        &mut self,
        value: VALUE,
    ) -> ::derive_builder::export::core::result::Result<&mut Self, VALUE::Error> {
        let converted: String = value.try_into()?;
        let mut new = self;
        new.value = ::derive_builder::export::core::option::Option::Some(converted);
        Ok(new)
    }
    /**Builds a new `Example`.

# Errors

If a required field has not been initialized.
*/
    fn build(
        &self,
    ) -> ::derive_builder::export::core::result::Result<Example, ExampleBuilderError> {
        Ok(Example {
            value: match self.value {
                Some(ref value) => {
                    ::derive_builder::export::core::clone::Clone::clone(value)
                }
                None => {
                    return ::derive_builder::export::core::result::Result::Err(
                        ::derive_builder::export::core::convert::Into::into(
                            ::derive_builder::UninitializedFieldError::from("value"),
                        ),
                    );
                }
            },
        })
    }
    /// Create an empty builder, with all fields set to `None` or `PhantomData`.
    fn create_empty() -> Self {
        Self {
            value: ::derive_builder::export::core::default::Default::default(),
        }
    }
}
impl ::derive_builder::export::core::default::Default for ExampleBuilder {
    fn default() -> Self {
        Self::create_empty()
    }
}
///Error type for ExampleBuilder
#[non_exhaustive]
enum ExampleBuilderError {
    /// Uninitialized field
    UninitializedField(&'static str),
    /// Custom validation error
    ValidationError(::derive_builder::export::core::string::String),
}
#[automatically_derived]
impl ::core::fmt::Debug for ExampleBuilderError {
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        match self {
            ExampleBuilderError::UninitializedField(__self_0) => {
                ::core::fmt::Formatter::debug_tuple_field1_finish(
                    f,
                    "UninitializedField",
                    &__self_0,
                )
            }
            ExampleBuilderError::ValidationError(__self_0) => {
                ::core::fmt::Formatter::debug_tuple_field1_finish(
                    f,
                    "ValidationError",
                    &__self_0,
                )
            }
        }
    }
}
impl ::derive_builder::export::core::convert::From<
    ::derive_builder::UninitializedFieldError,
> for ExampleBuilderError {
    fn from(s: ::derive_builder::UninitializedFieldError) -> Self {
        Self::UninitializedField(s.field_name())
    }
}
impl ::derive_builder::export::core::convert::From<
    ::derive_builder::export::core::string::String,
> for ExampleBuilderError {
    fn from(s: ::derive_builder::export::core::string::String) -> Self {
        Self::ValidationError(s)
    }
}
impl ::derive_builder::export::core::fmt::Display for ExampleBuilderError {
    fn fmt(
        &self,
        f: &mut ::derive_builder::export::core::fmt::Formatter,
    ) -> ::derive_builder::export::core::fmt::Result {
        match self {
            Self::UninitializedField(ref field) => {
                f
                    .write_fmt(
                        ::core::fmt::Arguments::new_v1(
                            &["`", "` must be initialized"],
                            &[::core::fmt::ArgumentV1::new_display(&field)],
                        ),
                    )
            }
            Self::ValidationError(ref error) => {
                f
                    .write_fmt(
                        ::core::fmt::Arguments::new_v1(
                            &[""],
                            &[::core::fmt::ArgumentV1::new_display(&error)],
                        ),
                    )
            }
        }
    }
}
impl std::error::Error for ExampleBuilderError {}

[TedDriggs]

The issue here appears to be an ambiguity about the scope of strip_option.

Some of the code seems to think that strip_option would also apply to the try-setter, such as here where the type of VALUE is derived from the option-stripped ty.

However, immediately after that (here), the possibility of strip_option being used is not considered, as we only do one wrapping based on whether the builder field is an option.

Fixing this in either direction is straightforward, but I'm not sure what people's expected behavior here would be; should try_setter be looking for TryInto<String> or TryInto<Option<String>>?

[chanced]

Would adding a strip_option setting to try_setter make sense? If you use strip_option in one, you'd need to use it in both?

To answer your question though, if I have a strip_option in the setter, I'd want try_setter to implement TryInto<String>.

as a temporary stopgap, i rolled those methods by hand, e.g.:

    pub fn try_jwt_id<T: TryInto<String>>(&mut self, jwt_id: T) -> Result<&mut Self, T::Error> {
        Ok(self.jwt_id(jwt_id.try_into()?))
    }

[TedDriggs]

Would adding a strip_option setting to try_setter make sense?

I think it would be better to have the try_setter inherit the value of strip_option from setter. It feels weird, but the try_setter already picks up its name and ownership pattern from the setter options, so there's precedent for it.

Since this code previously wouldn't compile, we know that choosing to make it honor strip_option won't break any existing crate users, and there's more likely to be a useful TryInto<T> than a TryInto<Option<T>>.

Therefore, the change here is to:

1. Add an extra layer of wrapping for the stripped_option case, using the non-try-setter block as a blueprint
2. Add tests for try_setter and strip_option at the same time
3. Add a test where try_setter, strip_option, and a custom builder field type are all used together to make sure that wrapping case is handled properly
4. Add this to CHANGELOG

If you're interested in making those updates I'm happy to review and merge them.

[chanced]

Sounds good. My CI is currently blocked by #285 so that would take precedence for me. I will circle back to this at some point and work on it though.

Thank you for your help on this and for the crate. It is quite helpful!