Preserve fn and {} for start_fn so that rustdoc generates correct links

[Eisverygoodletter]

Preserve fn and {} during start_fn construction

Closes #57.

This PR changes the generation of start_fn such that it reuses the fn keyword and { } tokens from the original definition.

This fix works because rustdoc only considers the spans of the first and last tokens when determining the span of a function. In our case, the start token is pub, const or fn. The ending token is }. The original code already preserves pub and const so I have added code for preserving fn and {}.

The original implementation relies on quote! for generating {, } and fn which default to Span::call_site() which is why rustdoc linked to the macro callsite instead of the original function.

Seeing the changes

Some methods in the bon sandbox such as

bon/bon-sandbox/src/docs_comparison/methods.rs

Lines 10 to 26 in c0e4356

	pub fn method(
	&self,
	/// Doc comment on `x1`
	x1: u32,
	/// Doc comment on `x2`
	#[builder(default = 2 + 2)]
	x2: u32,
	/// Doc comment on `x3`
	x3: Option<u32>,
	/// Doc comment on `x4`
	#[builder(into)]
	x4: String,
	) {

should now generate a correct source link that links to the original function. Can be tested using cargo doc --package bon-sandbox --open.

Possible future improvements

Since rustdoc only cares about the start and end tokens, it is theoretically possible to modify builder method generation such that it also reuses the spans of the original struct member/parameter definitions (not implemented in this PR). This would be more complicated because you would have to assign spans from one type of token to another type of token which I haven't tested yet. I might make an issue for that later after digging through more of the codebase and thinking a bit more.

[Eisverygoodletter]

I want to mention that I fixed a typo in lib.rs: colletions -> collections for git hook to pass.

[Eisverygoodletter]

Seems like the failing CIs are caused by unused function lints in the sandbox/benchmark. Not sure what to do with them though.

[Veetaha]

Jobs in msrv do break sometimes because msrv jobs don't use a separate lock file, they are locked manually like this and this logic sometimes breaks:

bon/scripts/test-msrv.sh

Lines 37 to 47 in c0e4356

	step cargo update -p once_cell --precise 1.17.2
	step cargo update -p trybuild --precise 1.0.89
	step cargo update -p serde_json --precise 1.0.143
	step cargo update -p serde --precise 1.0.194
	step cargo update -p prettyplease --precise 0.2.17
	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
	step cargo update -p glob --precise 0.3.2

Tests in (beta/nightly) use the nightly/beta toolchains and may break any time due to new versions of said toolchains adding new or fixing existing lints. You can ignore those failures, I'll fix them separately

[Veetaha]

Merged unstable CI fixes #342. If you push any new commit or rebase the branch, CI should start using the updated version of code in master

[Eisverygoodletter]

I've made a merge commit to update my branch

[Veetaha]

Didn't expect this to be that simple. Thank you for researching and fixing this! Awaiting for tests update before merge (see last comment)

[Eisverygoodletter]

I've done a rewrite of my implementation. It now only preserves a Span from the original function block and applies it using syn::parse_quote_spanned! on the entire generated start_fn instead of attempting to preserve the fn and {} keywords. I see no visual differences between the two implementations and I find this version simpler (thanks for introducing me to quote_spanned!).

[Eisverygoodletter]

Just checked the CI results, seems like I blocked out clippy::elidable_lifetime_names (pedantic-allow lint) but clippy::needless_lifetimes (complexity-warn lint) is triggering. Not sure why it's only failing on MSRV tests, maybe they changed the lints sometime between versions? I'll try test it locally.

[Eisverygoodletter]

Seems like part of clippy::needless_lifetimes's functionality was split off and moved into elidable_lifetime_names between the MSRV versions and the stable version (rust-lang/rust-clippy#13960). elidable_lifetime_names is now the more aggressive lint while clippy::needless_lifetimes has less false-positive/bad-error-message issues.

This seems to not be the case in the MSRV versions where clippy::needless_lifetimes also did the work of clippy::elidable_lifetime_names.

Should I add allow(clippy::needless_lifetimes)? This would solve the MSRV build issues but might also cover up legitimate issues from bon that could be detected by newer versions of Rust.

[Eisverygoodletter]

I've managed to make it so that before Rust version 1.87 it will allow needless_lifetimes. After 1.87 it will allow elidable_lifetime_names. I don't block needless_lifetimes post 1.87 because the newer version of the lint doesn't false-positive on bon generated code and would probably detect user error. elidable_lifetime_names didn't exist before 1.87 so no point in allowing it before that version.

Edit: Seems like the lifetime elision errors aren't just generated by start_fn but other generated functions as well (Tested locally on 1.86). I'll investigate more tomorrow when I have time.

[Veetaha]

Seems like the lifetime elision errors aren't just generated by start_fn but other generated functions as well

Looks like CI is green now. Are you sure there are any more lint problems left?

[Eisverygoodletter]

I don't actually think that the lints were triggered by bon generated code. My original code (before bon macro application) was already triggering the false positives so it isn't bon's problem.

I tested with a few more examples and it seems like the issues are caused by internal clippy bugs (link to github issues page with relevant tags) making the lint trigger on some false positives. The lint issues I mentioned were my own (local) test cases which was why they triggered the niche false positives.

We don't need to do anything for the lint problems I mentioned, the CI is right.

Edit: Forgot to mention that the false positives are already fixed on stable.

[Eisverygoodletter]

Seems like this PR is ready. Any things that still need changing?

[Veetaha]

Alright, thank you for the contribution! I'll create a new patch with this after merge