check for misplaced headers in missing_safety/error_doc #5659
Conversation
|
r? @phansch (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
Hey @llogiq, I took a look at this issue before but never finished, I thought I could share some ideas if it's OK!
In general, I think the problem comes from a difference in how rustdoc and clippy parse the doc comments. Clippy adapted strip_doc_comment_decoration but rustdoc also uses an ad-hoc algorith for unindenting the comments here:
https://github.com/rust-lang/rust/blob/45127211566c53bac386b66909a830649182ab7a/src/librustdoc/passes/unindent_comments.rs#L49-L105
It would be great if clippy could adapt that code to behave in the same way.
| @@ -88,6 +88,21 @@ very_unsafe!(); | |||
| // we don't lint code from external macros | |||
| undocd_unsafe!(); | |||
|
|
|||
| /** This safety section is now recognized. | |||
|
|
|||
| # Safety | |||
There was a problem hiding this comment.
The problematic example in the issue has an indentation of 4 spaces (here there are only 3). I think this is important because in markdown 4 spaces start a code block, see my other comment.
| headers.safety |= in_heading && text.trim() == "Safety"; | ||
| headers.errors |= in_heading && text.trim() == "Errors"; | ||
| let trimmed = text.trim(); | ||
| headers.safety |= has_header(trimmed, in_heading, "# Safety"); |
There was a problem hiding this comment.
We should check that we are not in_code, otherwise this passes the test:
/// This is not a safety section.
/// ```
/// # Safety
/// ```
pub unsafe fn ceci_n_est_pas_un_titre() {
unimplemented!();
}If we add the check (and the missing space in the other test to make it 4) the header is still not recognised.
|
Yeah, not duplicating this would be nice. However, we don't need to unindent here, just check if there's an unindented header, so our code can do less. |
|
@llogiq I might have misread your last comment - Is that something you still want to fix for this PR or something for a separate PR? |
flip1995
added
S-waiting-on-author
|
ping from triage @llogiq. There's still the open question if you want to address this: #5659 (comment) in this PR? |
|
☔ The latest upstream changes (presumably #5912) made this pull request unmergeable. Please resolve the merge conflicts. |
|
ping from triage @llogiq. Closing this issue, since it is stale for quite some time now. Feel free to reopen anytime you want to continue working on this! |
flip1995
added
S-inactive-closed
|
I'm willing to revisit this in a new PR if that's okay. |
|
@rustbot label -S-inactive-closed |
This fixes #5593.
changelog: none