Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seemingly false positive of doc_lazy_continuation #12917

Closed
torokati44 opened this issue Jun 11, 2024 · 5 comments · Fixed by #13002
Closed

Seemingly false positive of doc_lazy_continuation #12917

torokati44 opened this issue Jun 11, 2024 · 5 comments · Fixed by #13002
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@torokati44
Copy link

torokati44 commented Jun 11, 2024

Summary

We started getting these lints here: https://github.com/ruffle-rs/ruffle/actions/runs/9463299965/job/26068182584#step:6:1150

The actual piece of code: https://github.com/ruffle-rs/ruffle/blob/2b17b6ce20591b958b879e27182ad5d0c5712084/core/src/avm2/globals/flash/display/graphics.rs#L416-L482

Instinctively I don't see anything wrong with it, and it looks like the /// and // blocks, separated by empty lines, are conflated in various ways by different tools.

Lint Name

doc_lazy_continuation

Reproducer

I tried this code:

/// This is a constant.
///
/// The meaning of which should not be explained.
const A: i32 = 42;

/// This is another constant, no longer used.
///
/// This block of documentation has a long
/// explanation and derivation to explain
/// why it is what it is, and how it's used.
///
/// It is left here for historical reasons, and
/// for reference.
///
/// Reasons it's great:
///  - First reason
///  - Second reason
//const B: i32 = 1337;

/// This is yet another constant.
///
/// This has a similar fate as `B`.
///
/// Reasons it's useful:
///  1. First reason
///  2. Second reason
//const C: i32 = 8008;

/// This is still in use.
const D: i32 = 20;

I saw this happen:

warning: doc list item missing indentation
  --> src/lib.rs:21:5
   |
21 | /// This is yet another constant.
   |     ^
   |
   = help: if this is supposed to be its own paragraph, add a blank line
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_lazy_continuation
   = note: `#[warn(clippy::doc_lazy_continuation)]` on by default
help: indent this line
   |
21 | ///    This is yet another constant.
   |     +++

warning: doc list item missing indentation
  --> src/lib.rs:30:5
   |
30 | /// This is still in use.
   |     ^
   |
   = help: if this is supposed to be its own paragraph, add a blank line
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_lazy_continuation
help: indent this line
   |
30 | ///     This is still in use.
   |     ++++

I expected to see this happen:

Somehow these warnings don't seem right when looking at the code snippet with human eyes.

Version

$ cargo +beta -vV
cargo 1.80.0-beta.1 (34a6a87d8 2024-06-04)
release: 1.80.0-beta.1
commit-hash: 34a6a87d8a2330d8c9d578f927489689328a652d
commit-date: 2024-06-04
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Fedora 40.0.0 [64-bit]

Additional Labels

No response

@torokati44 torokati44 added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Jun 11, 2024
@Alexendoo
Copy link
Member

What's happening here is doc comments are converted to #[doc = "..."] attributes so rustdoc essentially sees

/// This is a constant.
///
/// The meaning of which should not be explained.
const A: i32 = 42;

/// This is another constant, no longer used.
///
/// This block of documentation has a long
/// explanation and derivation to explain
/// why it is what it is, and how it's used.
///
/// It is left here for historical reasons, and
/// for reference.
///
/// Reasons it's great:
///  - First reason
///  - Second reason
/// This is yet another constant.
///
/// This has a similar fate as `B`.
///
/// Reasons it's useful:
///  1. First reason
///  2. Second reason
/// This is still in use.
const D: i32 = 20;

But the output is confusing, we should probably have a lint for this behaviour since it's unexpected and suppress the other doc lints when it triggers

@torokati44
Copy link
Author

suppress the other doc lints when it triggers

That would be at the cost of occasional false negatives, right?

@Alexendoo
Copy link
Member

If we make it so that they're only suppressed when the lint is actually shown we should be good, that way you receive the other lints again after either fixing or #[allow]ing it

@Alexendoo Alexendoo self-assigned this Jun 12, 2024
@workingjubilee
Copy link
Member

False negatives are massively preferable to false positives. Clippy has 433 false-positive issues open and only 72 false-negatives about now. It should be the other way around.

@Alexendoo
Copy link
Member

Few false negatives are reported because they're hard to notice and/or just considered part of the territory of a linter, that is despite there being an essentially never ending amount of them. But why comment that here? Obviously we know false positives are not good

notriddle added a commit to notriddle/rust-clippy that referenced this issue Jun 27, 2024
This change addresses cases where doc comments are separated
by blank lines, comments, or non-doc-comment attributes,
like this:

```rust
/// - first line
// not part of doc comment
/// second line
```

Before this commit, Clippy gave a pedantically-correct
warning about how you needed to indent the second line.
This is unlikely to be what the user intends, and has
been described as a "false positive" (since Clippy is
warning you about a highly unintuitive behavior that
Rustdoc actually has, we definitely want it to output
*something*, but the suggestion to indent was poor).

rust-lang#12917
notriddle added a commit to notriddle/rust-clippy that referenced this issue Jun 28, 2024
This change addresses cases where doc comments are separated
by blank lines, comments, or non-doc-comment attributes,
like this:

```rust
/// - first line
// not part of doc comment
/// second line
```

Before this commit, Clippy gave a pedantically-correct
warning about how you needed to indent the second line.
This is unlikely to be what the user intends, and has
been described as a "false positive" (since Clippy is
warning you about a highly unintuitive behavior that
Rustdoc actually has, we definitely want it to output
*something*, but the suggestion to indent was poor).

rust-lang#12917
notriddle added a commit to notriddle/rust-clippy that referenced this issue Jun 28, 2024
This change addresses cases where doc comments are separated
by blank lines, comments, or non-doc-comment attributes,
like this:

```rust
/// - first line
// not part of doc comment
/// second line
```

Before this commit, Clippy gave a pedantically-correct
warning about how you needed to indent the second line.
This is unlikely to be what the user intends, and has
been described as a "false positive" (since Clippy is
warning you about a highly unintuitive behavior that
Rustdoc actually has, we definitely want it to output
*something*, but the suggestion to indent was poor).

rust-lang#12917
@bors bors closed this as completed in 2f80536 Jun 28, 2024
bors added a commit that referenced this issue Aug 25, 2024
Rewrite `empty_line_after_doc_comments` and `empty_line_after_outer_attr`, move them from `nursery` to `suspicious`

changelog: [`empty_line_after_doc_comments`], [`empty_line_after_outer_attr`]: rewrite and move them from `nursery` to `suspicious`

They now lint when there's a comment between the last attr/doc comment and the empty line, to cover the case:

```rust
/// Docs for `old_code
// fn old_code() {}

fn new_code() {}
```

When these lints or `suspicious_doc_comments` trigger we no longer trigger any other doc lint as a broad fix for #12917, reverts some of #13002 as the empty line lints cover it

I ended up not doing #12917 (comment) as I don't think it's needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants