new lint: doc_comment_double_space_linebreak

[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_linebreak

[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?

[xFrednet]

@Jacherr You should be able to call span.from_expansion(). That's how we usually check for macro expansion.

[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.

[Marcono1234]

Should probably not have a space, see also #12876 (comment)

Suggested change

	/// /// This command takes two numbers as inputs and \
	/// /// This command takes two numbers as inputs and\

[Marcono1234]

@Jacherr, I think this is still open, or did you intentionally keep it this way?

[Jacherr]

Yep, somehow missed this 😅. Should be resolved now.

[Marcono1234]

Assuming this is a standard Rust doc comment¹, the \ probably has to be escaped, otherwise it currently (redundantly) escapes the ), and is rendered as "()":

Suggested change

	/// Detects doc comment linebreaks that use double spaces to separate lines, instead of back-slash (\).
	///
	/// ### Why is this bad?
	/// Double spaces, when used as doc comment linebreaks, can be difficult to see, and may
	/// accidentally be removed during automatic formatting or manual refactoring. The use of a back-slash (\)
	/// Detects doc comment linebreaks that use double spaces to separate lines, instead of back-slash (\\).
	///
	/// ### Why is this bad?
	/// Double spaces, when used as doc comment linebreaks, can be difficult to see, and may
	/// accidentally be removed during automatic formatting or manual refactoring. The use of a back-slash (\\)

Or alternatively could format it as code:

Suggested change

	/// Detects doc comment linebreaks that use double spaces to separate lines, instead of back-slash (\).
	///
	/// ### Why is this bad?
	/// Double spaces, when used as doc comment linebreaks, can be difficult to see, and may
	/// accidentally be removed during automatic formatting or manual refactoring. The use of a back-slash (\)
	/// Detects doc comment linebreaks that use double spaces to separate lines, instead of back-slash (`\`).
	///
	/// ### Why is this bad?
	/// Double spaces, when used as doc comment linebreaks, can be difficult to see, and may
	/// accidentally be removed during automatic formatting or manual refactoring. The use of a back-slash (`\`)

Footnotes

1. Not sure if the clippy documentation generation treats this in some special way. ↩

[xFrednet]

It is a standard Rust doc comment. The output is later rendered with a JS Markdown thingy, but it's likely that a double backslash is needed. Using backticks to make it inline code works and is the way I would recommend.

(@Marcono1234, thank you for the comment, I would have probably missed this :D)

[xFrednet]

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

I also commented on previous reviews:

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

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

[xFrednet]

Here you can use snippet_opt to just skip this if the snippet is unavailable

Suggested change

	&& let snippet = snippet(cx, span, "..")
	&& let Some(snippet) = snippet_opt(cx, span)

[xFrednet]

This should either use snippet_with_applicability or snippet_opt.

snippet_opt is probably easier here.

[bors]

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