search

elastio / bon

Generate builders for everything!
https://elastio.github.io/bon/
Apache License 2.0
1.03k stars 16 forks source link

`must_use` build() #77

Closed EdJoPaTo closed 2 weeks ago

EdJoPaTo commented 3 weeks ago

While I like seeing the struct FooBuilder<…> having a must_use (#26) I'm not sure why the build() method does not have one. Some Into implementations could have side effects but other than that it should just create an object which hasn't been used so far. So its likely wrong when the build() output isn't used?

Veetaha commented 2 weeks ago

I agree, must_use should be the on the build() method. It's an oversight that it isn't there.

If you want, you can make a PR with this change, or I can do it myself a bit later anyway. There change should go into the FinishFunc struct config of the builder_gen module (the shared module that is used to generate the builder for both struct and function inputs): https://github.com/elastio/bon/blob/d49a3b56790768daafb4b0e869f0b5e2dd722d29/bon-macros/src/builder/builder_gen/mod.rs#L45-L52

We should configure it to enable must_use for struct inputs only. I believe must_use doesn't make sense for functions by default (only for structs). Additionally, maybe the code could also search for a must_use attribute on the function (if the input is an fn), and if it's present, then forward that attribute to the FinishFunc.

EdJoPaTo commented 2 weeks ago

I'd be happy to add that as it would be a learning for me to tinker with proc macros. So I'd like to provide a PR later.

For now: adding must_use is considered a breaking change. For this case I would say its rather fixing a bug as it restores the intended behaviour. When there is no must_use it's likely a bug. So such a PR should not require a new major release in my opinion.

Veetaha commented 2 weeks ago

For this case I would say its rather fixing a bug as it restores the intended behaviour.

This is my thinking as well. I think this kind of breakage is the one people would like to see.

But.. I'd like to argue that adding #[must_use] is not a breaking change by itself. It's breaking if and only if you deny warnings on CI. The violation of #[must_use] is a warning, not a compile error.

According to the cargo book "introducing new lints" such as #[deprecated], #[must_use], or any other change to the crate API that doesn't physically produce a compile error can be regarded as a minor change.

Once this change lands in master a minor version of bon will be bumped.

EdJoPaTo commented 2 weeks ago

Just noticed I remembered this wrongly, cargo-semver-checks also sees must_use as a minor change.

$ cargo semver-checks --list | rg must_use
enum_must_use_added                      minor An enum has been marked with #[must_use].
function_must_use_added                  minor A function has been marked with #[must_use].
inherent_method_must_use_added           minor An inherent method or associated fn has been marked #[must_use].
struct_must_use_added                    minor A struct has been marked with #[must_use].
trait_must_use_added                     minor A trait has been marked with #[must_use].
union_must_use_added                     minor A union has been marked with #[must_use].