Skip to content

Add new unused_footnote_definition rustdoc lint #137858

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from all 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
16 changes: 16 additions & 0 deletions src/librustdoc/lint.rs
Original file line number Diff line number Diff line change
@@ -204,6 +204,20 @@ declare_rustdoc_lint! {
"detects markdown that is interpreted differently in different parser"
}

declare_rustdoc_lint! {
/// This lint checks for uses of footnote references without definition.
BROKEN_FOOTNOTE,
Warn,
"footnote reference with no associated definition"
}

declare_rustdoc_lint! {
/// This lint checks if all footnote definitions are used.
UNUSED_FOOTNOTE_DEFINITION,
Warn,
"unused footnote definition"
Comment on lines +211 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"footnote reference with no associated definition"
}
declare_rustdoc_lint! {
/// This lint checks if all footnote definitions are used.
UNUSED_FOOTNOTE_DEFINITION,
Warn,
"unused footnote definition"
"detects footnote references with no associated definition"
}
declare_rustdoc_lint! {
/// This lint checks if all footnote definitions are used.
UNUSED_FOOTNOTE_DEFINITION,
Warn,
"detects unused footnote definitions"

for consistency with other lint descriptions

}

pub(crate) static RUSTDOC_LINTS: Lazy<Vec<&'static Lint>> = Lazy::new(|| {
vec![
BROKEN_INTRA_DOC_LINKS,
@@ -218,6 +232,8 @@ pub(crate) static RUSTDOC_LINTS: Lazy<Vec<&'static Lint>> = Lazy::new(|| {
UNESCAPED_BACKTICKS,
REDUNDANT_EXPLICIT_LINKS,
UNPORTABLE_MARKDOWN,
BROKEN_FOOTNOTE,
UNUSED_FOOTNOTE_DEFINITION,
]
});

2 changes: 2 additions & 0 deletions src/librustdoc/passes/lint.rs
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@
mod bare_urls;
mod check_code_block_syntax;
mod footnotes;
mod html_tags;
mod redundant_explicit_links;
mod unescaped_backticks;
@@ -42,6 +43,7 @@ impl DocVisitor<'_> for Linter<'_, '_> {
if may_have_link {
bare_urls::visit_item(self.cx, item, hir_id, &dox);
redundant_explicit_links::visit_item(self.cx, item, hir_id);
footnotes::visit_item(self.cx, item, hir_id, &dox);
}
if may_have_code {
check_code_block_syntax::visit_item(self.cx, item, &dox);
91 changes: 91 additions & 0 deletions src/librustdoc/passes/lint/footnotes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
//! Detects specific markdown syntax that's different between pulldown-cmark
//! 0.9 and 0.11.
//!
//! This is a mitigation for old parser bugs that affected some
//! real crates' docs. The old parser claimed to comply with CommonMark,
//! but it did not. These warnings will eventually be removed,
//! though some of them may become Clippy lints.
//!
//! <https://github.com/rust-lang/rust/pull/121659#issuecomment-1992752820>
//!
//! <https://rustc-dev-guide.rust-lang.org/bug-fix-procedure.html#add-the-lint-to-the-list-of-removed-lists>
use std::ops::Range;

use pulldown_cmark::{Event, Options, Parser, Tag};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir::HirId;
use rustc_lint_defs::Applicability;
use rustc_resolve::rustdoc::source_span_for_markdown_range;

use crate::clean::Item;
use crate::core::DocContext;

pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &str) {
let tcx = cx.tcx;

let mut missing_footnote_references = FxHashSet::default();
let mut footnote_references = FxHashSet::default();
let mut footnote_definitions = FxHashMap::default();

let options = Options::ENABLE_FOOTNOTES;
let mut parser = Parser::new_ext(dox, options).into_offset_iter().peekable();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be making sure the lint is enabled before invoking the parser? I know the other lints don't do this, but maybe they should?

while let Some((event, span)) = parser.next() {
match event {
Event::Text(text)
if &*text == "["
&& let Some((Event::Text(text), _)) = parser.peek()
&& text.trim_start().starts_with('^')
&& parser.next().is_some()
&& let Some((Event::Text(text), end_span)) = parser.peek()
&& &**text == "]" =>
{
missing_footnote_references.insert(Range { start: span.start, end: end_span.end });
}
Comment on lines +35 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

It is quite odd that pulldown_cmark isn't emmitting some form of FootnoteReference here despite the docs saying they might not map to an actual definition.

In any case, I don't think this implementation is correct, since Text events are emitted for the bodies of all blocks. Crucially, this includes code blocks, which certainly should not be parsed for footnotes. An integration test should be added to show that we are not wrongfully parsing footnotes within code blocks.

One way to handle this is to track the type of the last Start event and make sure it isn't CodeBlock. luckily other blocks can't appear within code blocks so we don't have to track the full stack of tags. We might want to clear that variable whenever we reach an End event, but I'm not sure if the text after a code block will always get its own separate Paragraph event or not. An integration test with an unused footnote directly after a code block should clear this up.

Event::FootnoteReference(label) => {
footnote_references.insert(label);
}
Event::Start(Tag::FootnoteDefinition(label)) => {
footnote_definitions.insert(label, span.start + 1);
}
_ => {}
}
}

#[allow(rustc::potential_query_instability)]
for (footnote, span) in footnote_definitions {
if !footnote_references.contains(&footnote) {
let span = source_span_for_markdown_range(
tcx,
dox,
&(span..span + 1),
&item.attrs.doc_strings,
)
.unwrap_or_else(|| item.attr_span(tcx));

tcx.node_span_lint(crate::lint::UNUSED_FOOTNOTE_DEFINITION, hir_id, span, |lint| {
lint.primary_message("unused footnote definition");
});
}
}

#[allow(rustc::potential_query_instability)]
for span in missing_footnote_references {
let (ref_span, precise) =
source_span_for_markdown_range(tcx, dox, &span, &item.attrs.doc_strings)
.map(|span| (span, true))
.unwrap_or_else(|| (item.attr_span(tcx), false));
Comment on lines +74 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this just be a let Some(ref_span) = ... else { continue }; ?

It doesn't seem like we use ref_span if precise is false.


if precise {
tcx.node_span_lint(crate::lint::BROKEN_FOOTNOTE, hir_id, ref_span, |lint| {
lint.primary_message("no footnote definition matching this footnote");
lint.span_suggestion(
ref_span.shrink_to_lo(),
"if it should not be a footnote, escape it",
"\\",
Applicability::MaybeIncorrect,
);
});
}
}
}
8 changes: 8 additions & 0 deletions tests/rustdoc-ui/lints/broken-footnote.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#![deny(rustdoc::broken_footnote)]
#![allow(rustdoc::unportable_markdown)]

//! Footnote referenced [^1]. And [^2]. And [^bla].
//!
//! [^1]: footnote defined
//~^^^ ERROR: no footnote definition matching this footnote
//~| ERROR: no footnote definition matching this footnote
24 changes: 24 additions & 0 deletions tests/rustdoc-ui/lints/broken-footnote.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
error: no footnote definition matching this footnote
--> $DIR/broken-footnote.rs:4:45
|
LL | //! Footnote referenced [^1]. And [^2]. And [^bla].
| -^^^^^
| |
| help: if it should not be a footnote, escape it: `\`
|
note: the lint level is defined here
--> $DIR/broken-footnote.rs:1:9
|
LL | #![deny(rustdoc::broken_footnote)]
| ^^^^^^^^^^^^^^^^^^^^^^^^

error: no footnote definition matching this footnote
--> $DIR/broken-footnote.rs:4:35
|
LL | //! Footnote referenced [^1]. And [^2]. And [^bla].
| -^^^
| |
| help: if it should not be a footnote, escape it: `\`

error: aborting due to 2 previous errors

9 changes: 9 additions & 0 deletions tests/rustdoc-ui/lints/unused-footnote.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// This test ensures that the rustdoc `unused_footnote` is working as expected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This test ensures that the rustdoc `unused_footnote` is working as expected.
// This test ensures that the `rustdoc::unused_footnote` lint is working as expected.


#![deny(rustdoc::unused_footnote_definition)]

//! Footnote referenced. [^2]
//!
//! [^1]: footnote defined
//! [^2]: footnote defined
//~^^ unused_footnote_definition
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//~^^ unused_footnote_definition
//~^^ ERROR: unused footnote definition

Nit: for consistency with other tests, look for the actual error and not the note telling you why it is enabled

14 changes: 14 additions & 0 deletions tests/rustdoc-ui/lints/unused-footnote.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: unused footnote definition
--> $DIR/unused-footnote.rs:7:6
|
LL | //! [^1]: footnote defined
| ^
|
note: the lint level is defined here
--> $DIR/unused-footnote.rs:3:9
|
LL | #![deny(rustdoc::unused_footnote_definition)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 1 previous error