Add new comment_within_doc lint

[GuillaumeGomez]

Fixes #15245.

Considering how many false positive we might trigger with this lint, I put it into pedantic.

cc @ojeda
r? @samueltardieu

changelog: Add new comment_within_doc lint

Summary Notes

• Feature-freeze by github-actions[bot]

Managed by @rustbot—see help for details

[rustbot]

Some changes occurred in clippy_lints/src/doc

cc @notriddle

[github-actions]

Lintcheck changes for 6634d4b

Lint	Added	Removed	Changed
clippy::comment_within_doc	3	0	0

This comment will be updated if you push new changes

[ojeda]

If I understand correctly, this finds surrounding cases like:

/// ...
// ...
/// ...

But at least #15245 was intended to only lint on empty ones like:

/// ...
//
/// ...

The reason was to be more conservative to avoid false positives -- at least in the Linux kernel we expect to be able to write comments together to docs, and we already have 6 adjacent cases we would hit, e.g. one of them:

/// Use the given initializer to in-place initialize a `T`.
///
/// If `T: !Unpin` it will not be able to move afterwards.
// We don't implement `InPlaceInit` because `ListArc` is implicitly pinned. This is similar to
// what we do for `Arc`.
#[inline]
pub fn pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Self, E>

Moreover, I think we should also probably want to lint on adjacent cases, not just surrounded cases, e.g.

/// ...
//

So I think there are these cases:

• Surrounding, empty in all lines.
• Surrounding, with some content in at least one line.
• Adjacent, empty in all lines.
• Adjacent, with some content in at least one line.

Of those, 1 and 3 seem both pretty safe to enable and that was my original intention with the issue.

4 is not something we would want at least for the Linux kernel, since we expect to write those (and we have 6 cases already).

And finally 3 could perhaps be interesting for some projects -- in the Linux kernel we don't have any case yet (if I grepped correctly), but when I wrote our coding guidelines about comments I expected it to be possible (https://docs.kernel.org/rust/coding-guidelines.html#comments):

/// # Examples
///
// TODO: Find a better example.
/// ```
/// let foo = f(42);
/// ```

[notriddle]

Not sure if this us supposed to be empty comments only, or if it's supposed to be any comment sandwiched between doc comment lines.

I'm not sure if blank-lines-only would have enough true positives to justify maintaining it. Has this happened more than once?

[notriddle]

That "fix" is wrong, isn't it?

[GuillaumeGomez]

Completely wrong, good catch!

[GuillaumeGomez]

@ojeda I think checking for comments surrounded by doc comments is the right call. You can easily put an empty line before or after a code comment to not trigger this lint (which should be suggested by this lint too). Like that, we can handle all cases at once. I don't think there is any reason to forget a / only if a code comment is empty.

[ojeda]

I think checking for comments surrounded by doc comments is the right call. You can easily put an empty line before or after a code comment to not trigger this lint

Hmm... Do you mean an empty //, an empty /// or a fully empty line? None of those seem ideal (especially the 2 last ones), i.e. we expected to write // TODOs or similar in the middle.

I don't think there is any reason to forget a / only if a code comment is empty.

I am not sure what you mean. If you mean forgotten by the user, that is what triggered this issue, no? i.e. a missing slash.

[GuillaumeGomez]

I think checking for comments surrounded by doc comments is the right call. You can easily put an empty line before or after a code comment to not trigger this lint

Hmm... Do you mean an empty //, an empty /// or a fully empty line? None of those seem ideal (especially the 2 last ones), i.e. we expected to write // TODOs or similar in the middle.

The only case where it triggers this lint is:

///
//
///

Empty or not. If you add an empty line:

///

//
///

Or your code comment is on multiple lines:

///
//
//
///

The lint is not triggered.

I don't think there is any reason to forget a / only if a code comment is empty.

I am not sure what you mean. If you mean forgotten by the user, that is what triggered this issue, no? i.e. a missing slash.

I mean, forgetting a / is common enough that I don't think that linting only if the comment is empty is a good heuristic.

[ojeda]

Thanks, that is clearer. Then this seems a different lint.

What we would normally expect to be able to write (and thus not lint) things like:

/// empty or not
// not empty

/// empty or not
// not empty
/// empty or not

/// empty or not
// not empty
// empty
// not empty
/// empty or not

However, we would consider these to be likely mistakes (and thus lint):

/// empty or not
// empty
/// empty or not

/// empty or not
// empty
// empty
/// empty or not

/// empty or not
// empty

// empty
/// empty or not

Thus that is why I suggested linting for empty // lines as long as no other // in the group has content.

Now, if I understand you correctly, with this lint, we would need to write this case:

/// empty or not
// not empty
/// empty or not

as:

/// empty or not
// empty
// not empty
/// empty or not

It is not the end of the world, since it is quite uncommon. (A fully empty line looks worse, I think, so I don't think we would do that).

And for this case, your lint wouldn't trigger, but it sounds like it is likely a mistake:

/// empty or not
// empty
// empty
/// empty or not

Finally, there are other possibilities that are likely mistakes (in our version, I mean) but that may be best not to lint, or it could be a different lint, e.g. having an starting or ending empty // or ///` line in both the adjacent and surrounding cases, e.g.

/// empty or not
// empty
// not empty
/// empty or not

/// empty or not
/// empty

(With your lint the former would be OK since that is precisely the way to allow a comment).