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

WIP: empty_doc #11633

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4985,6 +4985,7 @@ Released 2018-09-13
[`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument
[`duration_subsec`]: https://rust-lang.github.io/rust-clippy/master/index.html#duration_subsec
[`else_if_without_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#else_if_without_else
[`empty_docs`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_docs
[`empty_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_drop
[`empty_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_enum
[`empty_line_after_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_line_after_doc_comments
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::drop_forget_ref::MEM_FORGET_INFO,
crate::duplicate_mod::DUPLICATE_MOD_INFO,
crate::else_if_without_else::ELSE_IF_WITHOUT_ELSE_INFO,
crate::empty_docs::EMPTY_DOCS_INFO,
crate::empty_drop::EMPTY_DROP_INFO,
crate::empty_enum::EMPTY_ENUM_INFO,
crate::empty_structs_with_brackets::EMPTY_STRUCTS_WITH_BRACKETS_INFO,
Expand Down
61 changes: 61 additions & 0 deletions clippy_lints/src/empty_docs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
use clippy_utils::diagnostics::span_lint_and_help;
use rustc_ast::ast::*;
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};

declare_clippy_lint! {
/// ### What it does
/// Detects documentation that is empty.

/// ### Why is this bad?
/// It is unlikely there is any reason to have empty documentation for an entity
///
/// ### Example
/// ```rust
/// ///
/// fn returns_true() {
/// true
/// }
/// ```
/// Use instead:
/// ```rust
/// fn returns_true() {
/// true
/// }
/// ```
#[clippy::version = "1.74.0"]
pub EMPTY_DOCS,
suspicious,
"docstrings exist but documentation is empty"
}

declare_lint_pass!(EmptyDocs => [EMPTY_DOCS]);

fn trim_comment(comment: &str) -> String {
comment
.trim()
.split("\n")
.map(|comment| comment.trim().trim_matches('*').trim_matches('!'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that one shouldn't be needed.

.collect::<Vec<&str>>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a missing .filter(|line| !line.is_empty())

.join("")
}

impl EarlyLintPass for EmptyDocs {
fn check_attribute(&mut self, cx: &EarlyContext<'_>, attribute: &Attribute) {
match attribute.kind {
AttrKind::DocComment(_line, comment) => {
if trim_comment(comment.as_str()).len() == 0 {
span_lint_and_help(
cx,
EMPTY_DOCS,
attribute.span,
"empty doc comment",
None,
"consider removing or fill it",
);
}
},
_ => {},
}
}
}
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ mod double_parens;
mod drop_forget_ref;
mod duplicate_mod;
mod else_if_without_else;
mod empty_docs;
mod empty_drop;
mod empty_enum;
mod empty_structs_with_brackets;
Expand Down Expand Up @@ -1123,6 +1124,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
});
store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv())));
store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter));
store.register_early_pass(|| Box::new(empty_docs::EmptyDocs));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
27 changes: 27 additions & 0 deletions tests/ui/empty_docs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#![allow(unused)]
#![warn(clippy::empty_docs)]

pub mod outer_module {

//!

//! valid doc comment

//!!

//!! valid doc comment

///

/// valid doc comment

/**
*
*/

/**
* valid block doc comment
*/

pub mod inner_module {}
Copy link
Member

@Alexendoo Alexendoo Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these are actually empty documentation, this is ~equivalent to

//!
//! valid doc comment
//!!
//!! valid doc comment

///
/// valid doc comment
/// 
/// valid block doc comment

Because doc comments and #[doc] attributes are merged together

This really should go in doc.rs where this merging/parsing is all handled already

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alexendoo according to https://doc.rust-lang.org/reference/comments.html there are one line and block doc comments.
What you show in your example is 8 separate one-line doc comments. They are not being merged in 2, just because there are no line breaks within them. So lint in this PR will trigger on lines 1, 3, 5, 6 (and I'm updating the test to demonstrate this).

As for block doc comments, I assume they might have empty lines (for formatting purposes) because the definition of empty for a block comment is when all lines are empty.

Tbh I do not understand the part about merging with #[doc] attributes. Can you please explain?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really should go in doc.rs where this merging/parsing is all handled already

there is no much merging/parsing happening in this current lint. The idea is to have all doc-related lints in doc.rs?

Copy link
Member

@Alexendoo Alexendoo Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying the attributes are merged in the AST/HIR, but the multiple attributes representing doc comments are merged into a single piece of documentation by rustdoc, be they line comments, block comments or #[doc = ".."] attributes

My example was to represent what rustdoc approximately sees as the documentation, not to say that doc comments get converted to line comments. A screenshot may be clearer:

    ///

    /// valid doc comment

    /**
     *
     */

    /**
     * valid block doc comment
     */

    pub mod inner_module {}

image

The whitespace between the doc comments are not significant, e.g.

/// a

///

/// b

is the same as

/// a
///
/// b

But we certainly don't want to lint the middle line comment, it's significant because it causes a and b to be separate paragraphs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no much merging/parsing happening in this current lint. The idea is to have all doc-related lints in doc.rs?

Yeah, it could go here

if doc.is_empty() {
return Some(DocHeaders::default());
}

With .is_empty() being changed to check that doc only contains whitespace if needed, you can use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_resolve/rustdoc/fn.span_of_fragments.html on fragments to get the span

}
38 changes: 38 additions & 0 deletions tests/ui/empty_docs.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
error: empty doc comment
--> $DIR/empty_docs.rs:14:5
|
LL | ///
| ^^^
|
= help: consider removing or fill it
= note: `-D clippy::empty-docs` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::empty_docs)]`

error: empty doc comment
--> $DIR/empty_docs.rs:18:5
|
LL | / /**
LL | | *
LL | | */
| |_______^
|
= help: consider removing or fill it

error: empty doc comment
--> $DIR/empty_docs.rs:6:5
|
LL | //!
| ^^^
|
= help: consider removing or fill it

error: empty doc comment
--> $DIR/empty_docs.rs:10:5
|
LL | //!!
| ^^^^
|
= help: consider removing or fill it

error: aborting due to 4 previous errors