new lint: doc_comment_double_space_linebreaks

[Jacherr]

Fixes #12163

I decided to initially make this a restriction lint because it felt a bit niche and opinionated to be a warn-by-default style lint. It may be appropriate as a style lint if the standard or convention is to use \ as doc comment linebreaks - not sure if they are!

The wording on the help message could be improved, as well as the name of the lint itself since it's a bit wordy - suggestions welcome.

This lint works on both /// and //! doc comments.

changelog: new lint: doc_comment_double_space_linebreaks

[rustbot]

r? @xFrednet

rustbot has assigned @xFrednet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Marcono1234]

Thanks a lot for implementing the lint I suggested!

I have added two small comments here, but feel free to consider them only suggestions since I am not a project member.

[xFrednet]

Very nice start! I left some comments, mostly for some additional tests. Once they have been addressed, this should be ready to be merged :D

Also thank you for the suggestions from @Marcono1234!

[notriddle]

This lint also needs to make sure it's not firing in any of the cases where Markdown doesn't treat two spaces at the end of a line as a hard line break.

Here's a link to a markdown-it playground where almost every line ends in two spaces, but it contains no hard line breaks. It gets the same results in GitHub and Rustdoc.

[Jacherr]

That's quite a lot of cases to handle - I'm not sure building in a subset of a markdown parser just for this one lint is the right way forward? I suppose it's only a handful of cases, but seems like it'd be a lot of work to implement and test properly, for the sake of this lint. If push comes to shove I can do it, but perhaps there's an easier/more general way to cover these cases?

[notriddle]

I think this can be done more easily than that, yes.

https://github.com/rust-lang/rust/blob/e9e6e2e444c30c23a9c878a88fbc3978c2acad95/src/tools/clippy/clippy_lints/src/doc/mod.rs#L660

In clippy_lints::doc::check_doc, the range variable gives you the start and end byte position of the event. By doing doc.as_bytes()[range.clone()], you can extract the source code for an event.

If the event is a HardBreak, and the source code is b" ", then you've got what you're looking for. No parsing required!

[Jacherr]

I'm trying to use the events to do this, but even though according to the markdown you provided most of those aren't valid linebreaks, I still seem to be getting HardBreak events for them? It's entirely possible I'm doing something wrong, but it doesn't seem to automatically "distinguish" between a valid linebreak and an invalid one.

[notriddle]

It’s an outdated version of pulldown. The current version does it right.

pulldown-cmark/pulldown-cmark#318

[Jacherr]

Given the fact that this is pretty niche, and that this is a restriction lint, and the pulldown crate is not up-to-date (with no simple way of updating it), I think the best course of action is leaving this as a FIXME in the lint for future improvement?

@xFrednet any thoughts on this? Personally I feel like this is a niche enough issue to not be too important, since the double spaces in these cases do nothing and don't need to be there at all in tables etc.

[notriddle]

Once rust-lang/rust#127127 is merged, those issues should go away?

[xFrednet]

The changes from the pull request will need some time to get synced back to this PR (Should be ~2 weeks). For now you can edit the Crates.toml file here to use the updated version.

@notriddle Thank you for the update and comments! ❤️

[xFrednet]

@xFrednet any thoughts on this? Personally I feel like this is a niche enough issue to not be too important, since the double spaces in these cases do nothing and don't need to be there at all in tables etc.

It depends a bit on how much work it is. Personally, I would prefer it to use pulldown since we have it. It will likely also be more maintainable.

[bors]

☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey @Jacherr, this is a ping from triage, since there hasn't been any activity in some time. The new version should now be available if you rebase on master.

Do you plan on continuing this implementation?

@rustbot author

[Jacherr]

Hey, apologies for the silence, been rather busy the last few weeks. I will complete this implementation but unfortunately will be a week or two before I get to it, if that’s okay.

[Jacherr]

I almost have this working properly, the one case I'm struggling to handle is detecting if a doc comment is in a macro definition, which we don't want to lint on. Is there any way to detect this from a span?

[Jacherr]

@rustbot ready

Hopefully got enough test-cases and caught any edge cases now. I'm not sure if how I extended check_doc is considered "correct" - it can pretty easily be changed if needed, I imagine.

[notriddle]

Do things work if you avoid pointing at the line break? The version that spans the whole thing looks really weird when it's formatted.

Suggested change

	lo_span.to(hi_span),
	lo_span.shrink_to_lo(),

[Jacherr]

I imagine this will only work if every line is linted individually. This could be done but not sure it's desired behaviour as it will drastically increase the amount of warnings per paragraph, usually.

[notriddle]

That makes sense. 👍

[xFrednet]

I would be in favor of having a warning for each double space. Looking at the lintcheck output in our CI, it's hard to see what the actual problem is.

In this window it looks like a normal line break:

   --> target/lintcheck/sources/arrayvec-0.7.4/src/arrayvec.rs:380:53
    |
380 |       /// This is a checked version of `.swap_remove`.  
    |  _____________________________________________________^
381 | |     /// This operation is O(1).
    | |________^

If the message only spanned the two spaces, it would look like this:

   --> target/lintcheck/sources/arrayvec-0.7.4/src/arrayvec.rs:380:53
    |
380 |       /// This is a checked version of `.swap_remove`.  
    |                                                       ^^
    |

This is much clearer IMO.

It also makes it easier to apply the suggestion automatically as there is less chance to have the span overlap with other lint emissions.

[xFrednet]

Two smaller comments, but this implementation already looks good :D

I also commented on previous reviews:

• new lint: doc_comment_double_space_linebreaks #12876 (comment)
• new lint: doc_comment_double_space_linebreaks #12876 (comment)

Also, big thanks to @notriddle and @Marcono1234 for their reviews ❤️

[bors]

☔ The latest upstream changes (presumably #12993) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably 8298da7) made this pull request unmergeable. Please resolve the merge conflicts.

[Jacherr]

@xFrednet this has been sat for a while, does all look good on your end? Seems that all that was left were a few nits which I cleaned up so once the conflicts are solved would this be ready to be merged? No rush of course

[xFrednet]

Woops, this totally slipped by my inbox. Sorry, thank you for pinging me! 😅

I'll put it in my reviewing queue. Currently it takes a bit longer due to RL stuff, but you should have a review in 1-2 weeks.

[xFrednet]

I believe a previous comment is still unresolved, at least according to the .stderr file, otherwise LGTM

[xFrednet]

Have you tried changing the error span, like suggested here?

#12876 (comment)

Suggested change

	--> tests/ui/doc/doc_comment_double_space_linebreak.rs:7:43
	|
	LL | //! Should warn on double space linebreaks
	| ___________________________________________^
	LL | | //! in file/module doc comment
	| |____^
	|
	--> tests/ui/doc/doc_comment_double_space_linebreak.rs:7:43
	|
	LL | //! Should warn on double space linebreaks
	| ^^
	|
	|

It should be something simple, like taking a span from lo_span to lo_span + 2

[Jacherr]

Just to be clear - is this fine as a restriction lint? I have no idea if the preferred way is to use a backslash or double space. If the correct method is a backslash maybe this would be better as a warn by default?

[xFrednet]

Just to be clear - is this fine as a restriction lint?

Yes, I think this category is perfect for the lint. That's why I didn't comment on it further.

I have no idea if the preferred way is to use a backslash or double space. If the correct method is a backslash maybe this would be better as a warn by default?

I think a backslash should be the default if you need a random line break, for the reasons described in the lint description.

[Jacherr]

So just to be completely sure, it's in pedantic right now and that's all good? Just don't want to miss anything before getting it looked at again and hopefully merged 😅

[xFrednet]

Currently, it's a pedantic lint. I'll ask for feedback on the category as part of the Final Comment Period (FCP) :)

[xFrednet]

Hej @Jacherr, I wanted to play a bit around with the error message to have the best of both worlds, one lint per paragraph, but also just the two spaces highlighted. I pushed my fix now and also adjusted the lint name, by adding an s to the end.

Now this should be good to go, I'll create an FCP and also ask for feedback on the lint category.

(I hope it was okay to push to your branch and also squash your commits 🙈 )

Edit: FCP: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.20new.20lint.3A.20.60doc_comment_double_space_linebreaks.60