Skip to content

Commit

Permalink
Fix handling of lifetimes not used in fn param types (#208)
Browse files Browse the repository at this point in the history
  • Loading branch information
Veetaha authored Nov 17, 2024
1 parent dad40c7 commit c985900
Show file tree
Hide file tree
Showing 3 changed files with 151 additions and 2 deletions.
19 changes: 17 additions & 2 deletions bon-macros/src/builder/builder_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,8 @@ impl BuilderGenCtx {
_ => None,
});

let types = receiver_ty
.into_iter()
let types = std::iter::empty()
.chain(receiver_ty)
.chain(member_types)
.chain(generic_types)
.map(|ty| {
Expand All @@ -305,6 +305,11 @@ impl BuilderGenCtx {
quote!(fn() -> ::core::marker::PhantomData<#ty>)
});

let lifetimes = self.generics.args.iter().filter_map(|arg| match arg {
syn::GenericArgument::Lifetime(lifetime) => Some(lifetime),
_ => None,
});

let state_var = &self.state_var;

quote! {
Expand All @@ -318,6 +323,16 @@ impl BuilderGenCtx {
// applicability of the auto traits.
fn() -> #state_var,

// Even though lifetimes will most likely be used somewhere in
// member types, it is not guaranteed in case of functions/methods,
// so we mention them all separately. This covers a special case
// for function builders where the lifetime can be entirely unused
// (the language permis that).
//
// This edge case was discovered thanks to @tonywu6 ❤️:
// https://github.com/elastio/bon/issues/206
#( &#lifetimes (), )*

// There is an interesting quirk with lifetimes in Rust, which is the
// reason why we thoughtlessly store all the function parameter types
// in phantom data here.
Expand Down
133 changes: 133 additions & 0 deletions bon/tests/integration/builder/generics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,3 +209,136 @@ fn lifetimes_with_bounds() {

sut().arg(&42).arg2(&42).call();
}

// This is based on the issue https://github.com/elastio/bon/issues/206
mod lifetimes_used_only_in_type_predicates {
use crate::prelude::*;

trait Trait<'a> {}

impl Trait<'_> for u32 {}

#[test]
fn function() {
#[builder]
fn sut<'a, T: Trait<'a>>(x1: T) -> T {
x1
}

assert_eq!(sut().x1(2).call(), 2);
}

#[test]
fn generic_method() {
struct Sut;

#[bon]
impl Sut {
#[builder]
fn method<'a, T: Trait<'a>>(x1: T) -> T {
x1
}

#[builder]
fn with_self<'a, T: Trait<'a>>(&self, x1: T) -> T {
let _ = self;
x1
}
}

assert_eq!(Sut::method().x1(2).call(), 2);
assert_eq!(Sut.with_self().x1(2).call(), 2);
}

#[test]
fn generic_impl() {
struct Sut<T>(T);

#[bon]
impl<'a, T: Trait<'a>> Sut<T> {
#[builder]
fn method(x1: T) -> T {
x1
}

#[builder]
fn with_self(&self, x1: T) -> T {
let _ = self;
x1
}
}

assert_eq!(Sut::<u32>::method().x1(2).call(), 2);
assert_eq!(Sut(1).with_self().x1(2).call(), 2);
}
}

mod unused_lifetimes {
use crate::prelude::*;

#[test]
fn function() {
#[builder]
#[allow(clippy::extra_unused_lifetimes, unused_lifetimes)]
fn sut<'a, 'b>() {}

sut().call();
}

#[test]
fn generic_method() {
struct Sut;

#[bon]
impl Sut {
#[builder]
#[allow(clippy::extra_unused_lifetimes, unused_lifetimes)]
fn method<'a, 'b>() {}

#[builder]
#[allow(clippy::extra_unused_lifetimes, unused_lifetimes)]
fn with_self<'a, 'b>(&self) {
let _ = self;
}
}

Sut::method().call();
Sut.with_self().call();
}

#[test]
fn generic_impl() {
struct Sut<T>(T);

// Interestingly, this code doesn't produce an `unused_lifetimes` warning
// because the generated starting functions are inserted into this impl
// block and they do use the lifetimes by storing them in the builder.
// There isn't a good way to deal with this problem.
//
// We could generate the starting functions in a separate impl block, but
// then it would break the lexical order of methods as they are declared
// in this impl block in regards to how they are displayed in `rustdoc`.
//
// Also, rustdoc permits documentation on the impl block itself, so if
// we create a separate impl block for the starting functions, that
// would be rendered as separate impl blocks in `rustdoc` as well and we
// would need to do something about the docs on the original impl block,
// (e.g. copy them to the impl block for starting functions?).
//
// Anyway, the problem of unused lifetimes lint false-negative is probably
// not that serious to justify the complexity of the solution to fix it.
#[bon]
impl<'a, 'b, T> Sut<T> {
#[builder]
fn method() {}

#[builder]
fn with_self(&self) {
let _ = self;
}
}

Sut::<u32>::method().call();
Sut(1).with_self().call();
}
}
1 change: 1 addition & 0 deletions scripts/test-msrv.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ step cargo update -p syn --precise 2.0.56
step cargo update -p tokio --precise 1.29.1
step cargo update -p expect-test --precise 1.4.1
step cargo update -p windows-sys --precise 0.52.0
step cargo update -p libc --precise 0.2.163

export RUSTFLAGS="${RUSTFLAGS:-} --allow unknown-lints"

Expand Down

0 comments on commit c985900

Please sign in to comment.