rust-lang
new lint: `doc_comment_double_space_linebreak`
Fixes https://github.com/rust-lang/rust-clippy/issues/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
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
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.
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?
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!
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.
It’s an outdated version of pulldown. The current version does it right.
https://github.com/pulldown-cmark/pulldown-cmark/issues/318
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.
Once https://github.com/rust-lang/rust/pull/127127 is merged, those issues should go away?
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! :heart:
@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.
:umbrella: The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.
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
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.
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 You should be able to call span.from_expansion(). That's how we usually check for macro expansion.
@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.
:umbrella: The latest upstream changes (presumably #12993) made this pull request unmergeable. Please resolve the merge conflicts.
:umbrella: The latest upstream changes (presumably 8298da72e7b81fa30c515631b40fc4c0845948cb) made this pull request unmergeable. Please resolve the merge conflicts.
@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
Woops, this totally slipped by my inbox. Sorry, thank you for pinging me! :sweat_smile:
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.
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?
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.
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 😅
Currently, it's a pedantic lint. I'll ask for feedback on the category as part of the Final Comment Period (FCP) :)
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 :see_no_evil: )
Edit: FCP: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.20new.20lint.3A.20.60doc_comment_double_space_linebreaks.60