From c9859000c7cf266d8a939f1dd10c2d06fa7ee717 Mon Sep 17 00:00:00 2001 From: Vitalii Kryvenko Date: Sun, 17 Nov 2024 06:39:58 +0200 Subject: [PATCH] Fix handling of lifetimes not used in fn param types (#208) --- bon-macros/src/builder/builder_gen/mod.rs | 19 +++- bon/tests/integration/builder/generics.rs | 133 ++++++++++++++++++++++ scripts/test-msrv.sh | 1 + 3 files changed, 151 insertions(+), 2 deletions(-) diff --git a/bon-macros/src/builder/builder_gen/mod.rs b/bon-macros/src/builder/builder_gen/mod.rs index 535e8655..08b13870 100644 --- a/bon-macros/src/builder/builder_gen/mod.rs +++ b/bon-macros/src/builder/builder_gen/mod.rs @@ -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| { @@ -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! { @@ -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. diff --git a/bon/tests/integration/builder/generics.rs b/bon/tests/integration/builder/generics.rs index 7ee99eec..a02e0201 100644 --- a/bon/tests/integration/builder/generics.rs +++ b/bon/tests/integration/builder/generics.rs @@ -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); + + #[bon] + impl<'a, T: Trait<'a>> Sut { + #[builder] + fn method(x1: T) -> T { + x1 + } + + #[builder] + fn with_self(&self, x1: T) -> T { + let _ = self; + x1 + } + } + + assert_eq!(Sut::::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); + + // 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 { + #[builder] + fn method() {} + + #[builder] + fn with_self(&self) { + let _ = self; + } + } + + Sut::::method().call(); + Sut(1).with_self().call(); + } +} diff --git a/scripts/test-msrv.sh b/scripts/test-msrv.sh index b6332f8d..8f2f1950 100755 --- a/scripts/test-msrv.sh +++ b/scripts/test-msrv.sh @@ -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"