Skip to content
Open
Show file tree
Hide file tree
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
Expand Up @@ -196,6 +196,20 @@ declare_rustdoc_lint! {
"detects redundant explicit links in doc comments"
}

declare_rustdoc_lint! {
/// This lint checks for uses of footnote references without definition.
BROKEN_FOOTNOTE,
Warn,
"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"
}

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

Expand Down
2 changes: 2 additions & 0 deletions src/librustdoc/passes/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

mod bare_urls;
mod check_code_block_syntax;
mod footnotes;
mod html_tags;
mod redundant_explicit_links;
mod unescaped_backticks;
Expand Down Expand Up @@ -41,6 +42,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);
Expand Down
88 changes: 88 additions & 0 deletions src/librustdoc/passes/lint/footnotes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
//! 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>
Comment on lines +1 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

This doc comment is wrong.


use std::ops::Range;

use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir::HirId;
use rustc_lint_defs::Applicability;
use rustc_resolve::rustdoc::pulldown_cmark::{Event, Options, Parser, Tag};
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs are outdated. FootnoteReference is only emitted if the footnote definition exists.

pulldown-cmark/pulldown-cmark#1038

Copy link
Contributor

Choose a reason for hiding this comment

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

glad to see the docs getting fixed, but i still believe this code handles code blocks incorrectly.

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), false));

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, _) =
source_span_for_markdown_range(tcx, dox, &span, &item.attrs.doc_strings)
.unwrap_or_else(|| (item.attr_span(tcx), false));

tcx.node_span_lint(crate::lint::BROKEN_FOOTNOTE, hir_id, ref_span, |lint| {
lint.primary_message("no footnote definition matching this footnote");
Copy link
Contributor

Choose a reason for hiding this comment

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

This lint is redundant with the doc_suspicious_footnotes clippy lint, but it doesn't implement the number checking the same way. Shouldn't that be added?

lint.span_suggestion(
ref_span.shrink_to_lo(),
"if it should not be a footnote, escape it",
"\\",
Applicability::MaybeIncorrect,
);
});
}
}
7 changes: 7 additions & 0 deletions tests/rustdoc-ui/lints/broken-footnote.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![deny(rustdoc::broken_footnote)]

//! 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:3: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:3: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` lint is working as expected.

#![deny(rustdoc::unused_footnote_definition)]

//! Footnote referenced. [^2]
//!
//! [^1]: footnote defined
//! [^2]: footnote defined
//~^^ ERROR: unused footnote definition
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

Loading