Repository: elastio/bon (language: rust)

Issue:
into triggers `clippy::impl_trait_in_params`
See https://rust-lang.github.io/rust-clippy/master/index.html#/impl_trait_in_params.

Reproduce:

```rust
#![warn(clippy::impl_trait_in_params)]

fn main() {
    let example = Example::builder().content(42).build();
    dbg!(example.content);
}

#[bon::builder]
pub struct Example {
    #[builder(into)]
    content: i32,
}
```

Maybe it's a good idea to follow this lints suggestion and move to `fn content<T: Into<i32>(self, value: T)`. Otherwise, maybe ignore this lint by default (like other non clippy lints are already ignored)


Discussion / hints:
Yeah, this lint doesn't make sense for the particular case of setters that accept a single parameter with `impl Into` trait.

If you want to make PR, the place to add this attribute is on the setter generation code here: https://github.com/elastio/bon/blob/d49a3b56790768daafb4b0e869f0b5e2dd722d29/bon-macros/src/builder/builder_gen/setter_methods.rs#L272-L284
Not sure whether it really doesn't make sense. Having `content::<u8>("42".parse())` could be neat for people to use?
I see, didn't think of this `parse()` case, иге `parse` has a generic parameter on its own:
```rust
content("42".parse::<u32>())
```

Anyway, I still believe that using `impl Trait` syntax is better, just because it's easier to read it this way. The type of the value is visible right in the parameter's type annotation and you don't have to move your eyes to the generic parameter declaration. Also, the generic parameter will have an ugly name with `__`  prefix to prevent potential collisions with other generic parameters in scope from user's structs/functions.

Compare reading this:

```rust
fn member(value: impl Into<String>) -> NextBuilderState
```
to this:
```rust
fn member<__T: Into<String>>(value: __T) -> NextBuilderState
```
Notice that the generated code doesn't even qualify the `Into` trait with `::core::convert::Into` in the sake of better readability.
`"42".parse()` was just one example I could think of where it might be neat to have the option to use the turbofish on the method directly. Parse has the generic parameter for a reason, so I would do it similarly. And I don't think its significantly more complicated to read. It's also quite common, so I don't think that's a big argument.
The stuff all around it is already quite intimidating when looking at the docs / rust-analyzer preview.

---

`__T` wouldn't actually be needed because of its scope to this exact function shadowing anything else with the same name:

```rust
fn main() {
    let bla = whatever(42);
    dbg!(&bla);
}

fn whatever<T: core::fmt::Debug>(input: T) -> i32 {
    struct Bla {
        content: i32,
    }
    impl Bla {
        fn new<T: Into<i32>>(value: T) -> Self {
            Self {
                content: value.into(),
            }
        }
    }

    let bla = Bla::new(32_u8);
    dbg!(input);
    bla.content
}
```

---

rust-analyzer preview (VSCode / Zed):

```rust
fn new<T>(value: T) -> Self
where
    T: Into<i32>,

// With absolute qualifier used in the code:

fn new<T>(value: T) -> Self
where
    T: ::core::convert::Into<i32>,
```

`cargo doc` (either `Into` or `::core:…`):

> pub(crate) fn [new](file:///home/edjopato/tmp/clippy-playground/target/doc/clippy_playground/struct.Bla.html#method.new)<T: [Into](https://doc.rust-lang.org/1.80.1/core/convert/trait.Into.html)<[i32](https://doc.rust-lang.org/1.80.1/std/primitive.i32.html)>>(value: T) -> Self

when using `where T: …` it gets more into a multi line view:

> pub(crate) fn [new](file:///home/edjopato/tmp/clippy-playground/target/doc/clippy_playground/struct.Bla.html#method.new)<T>(value: T) -> Self
where
    T: [Into](https://doc.rust-lang.org/1.80.1/core/convert/trait.Into.html)<[i32](https://doc.rust-lang.org/1.80.1/std/primitive.i32.html)>,
Prefix `__` will be required in such a case because shadowing must not happen (it's actually a compile error if you try to shadow a generic parameter). Suppose there is a struct like this:

```rust
#[bon::builder]
struct MyLovelyStruct<T> {
    #[builder(into)]
    items: Vec<T>
}
```
The setter that will be generated is this:

```rust
impl<T, __State: /* some bounds */> MyLovelyStructBuilder<T, __State> {
    fn items(self, value: impl Into<Vec<T>>) -> MyLovelyStructBuilder<T, /* new state */> { /**/ }
}
```
Now if the setter defines a generic parameter that shadows `T`, things break. This won't work:
```rust
fn items<T: Into<Vec<T>>(/**/);
```
Error:
```log
  |
5 | impl<T> MyLovelyStruct<T> {
  |      - first use of `T`
6 |     fn items<T: Into<Vec<T>>>(self, value: T) {
  |              ^ already used
```

---

As for me people far more often read the code and type signatures, than have use cases for passing a generic parameter to the function that accepts an `Into`-ed value. If the setter accepts a value generic over the `Into` trait and you need to provide a type hint for it, then it's already a case of bad design (maybe `Into` shouldn't have been enabled in the first place), which must be quite rare. I think requiring the user to move the expression into a variable with an explicit type in such case is fine, than compromising the readability

---

`parse` has a generic parameter, because this parameter very often requires being specified explicitly. With `impl Into` setters 99% of the time you shouldn't need to specify a type hint and if you do, as I mentioned, this is a bad design, and `Into` shouldn't have been enabled there.

I'll need examples of realistic use cases where `Into` enabled on the setter follows the [rules of `Into`](https://elastio.github.io/bon/guide/patterns/into-conversions-in-depth#use-into-conversions), and requires the type hint for the generic parameter very often.

At this point I'm convinced such use cases don't exist, because having to specify the generic parameter for the setter so often defeats the purpose of using `into` (which is destined to shorten the code, not enlarge it for the caller).
Another example, of a widely used library that uses builders is AWS SDK. It uses `impl Into<String>` syntax for string members:
https://docs.rs/aws-sdk-s3/latest/aws_sdk_s3/operation/put_object/builders/struct.PutObjectInputBuilder.html#method.bucket

They expect that you won't need to specify a type hint for setter that accepts `impl Into<String>`.

..And `typed-builder` also uses `impl Into` syntax for its `#[builder(setter(into))]` attribute. It triggers the same clippy lint `clippy::impl_trait_in_params`
> If […] you need to provide a type hint for it, then it's already a case of bad design (maybe `Into` shouldn't have been enabled in the first place)

I think that's a good argument. I like the discussion!

So the remaining action from this is to add an `#[allow` to this macro, so users of the lint can still use bon. I'd like to attempt a PR for that (hopefully in a few hours).

Fix patch:
diff --git a/bon-macros/src/builder/builder_gen/setter_methods.rs b/bon-macros/src/builder/builder_gen/setter_methods.rs
index 8b70e429..3a054a32 100644
--- a/bon-macros/src/builder/builder_gen/setter_methods.rs
+++ b/bon-macros/src/builder/builder_gen/setter_methods.rs
@@ -271,6 +271,7 @@ impl<'a> MemberSettersCtx<'a> {
 
         quote! {
             #( #docs )*
+            #[allow(clippy::impl_trait_in_params)]
             #[inline(always)]
             #vis fn #method_name(self, #fn_params) -> #return_type {
                 #builder_ident {


Test patch: