Skip to content

Add new unused_footnote_definition rustdoc lint#137858

Open
GuillaumeGomez wants to merge 8 commits into
rust-lang:mainfrom
GuillaumeGomez:unused_footnote_def
Open

Add new unused_footnote_definition rustdoc lint#137858
GuillaumeGomez wants to merge 8 commits into
rust-lang:mainfrom
GuillaumeGomez:unused_footnote_def

Conversation

@GuillaumeGomez

@GuillaumeGomez GuillaumeGomez commented Mar 1, 2025

Copy link
Copy Markdown
Member

View all comments

Follow-up of #137803 (where the two first commits come from).

It adds a new lint which checks for unused footnote definitions.

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 1, 2025
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Fixed tidy.

@GuillaumeGomez GuillaumeGomez force-pushed the unused_footnote_def branch 2 times, most recently from 40b5fa2 to 27c7ec4 Compare March 11, 2025 14:14
@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Updated to use new footnote lint code instead.

@bors

bors commented May 7, 2025

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #140726) made this pull request unmergeable. Please resolve the merge conflicts.

@lolbinarycat lolbinarycat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One main issue (possibly parsing footnotes as code blocks) and a bunch of small nits.

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
Copy Markdown
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?

Comment thread src/librustdoc/passes/lint/footnotes.rs Outdated
Comment on lines +35 to +44
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 });
}

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could be easily answered by some tests that exercise the case of broken markdown inside a code block, as well as a footnote definition outside the code block referenced by markdown inside the block, and the other way around.

Comment thread tests/rustdoc-ui/lints/unused-footnote.rs Outdated
Comment thread tests/rustdoc-ui/lints/unused-footnote.rs Outdated
Comment thread src/librustdoc/lint.rs Outdated
@lolbinarycat

Copy link
Copy Markdown
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2025
@notriddle

notriddle commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

This PR is still based on #137803, which I thought was a bad idea because of the false positives.

The unused_footnote_definition lint shouldn't have any false positives, so it's fine, but this PR needs rebased to remove the other, rejected lint.

@rustbot

This comment has been minimized.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Sorry for the delay, finally applied suggestions.

Comment thread src/librustdoc/passes/lint/footnotes.rs Outdated
Comment thread src/librustdoc/passes/lint/footnotes.rs Outdated
@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot rustbot added the has-merge-commits PR has merge commits, merge with caution. label Feb 25, 2026
@rustbot

This comment has been minimized.

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2026
@GuillaumeGomez

Copy link
Copy Markdown
Member Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2026
Comment thread src/librustdoc/passes/lint/footnotes.rs Outdated
Comment on lines +24 to +29
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 == "]" =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This parsing logic doesn't fully account for backslashes or special characters. This test case fails (it's a false positive, because it produces a warning when it shouldn't):

/// Backslash escaped footnotes should not be recognized:
///
/// [\^4]
///
/// [^5\]
pub struct BackslashEscape;

And so does this one (it's a false negative, since it's supposed to produce a warning, but it doesn't):

/// Footnotes can contain asterisks, underscores, and other specials:
///
/// [^*]
//~^ ERROR: no footnote definition matching this footnote
///
/// [^_]
//~^ ERROR: no footnote definition matching this footnote
///
/// [^<inside></inside>]
//~^ ERROR: no footnote definition matching this footnote
pub struct Specials;

To do this correctly, you need to parse the source text, not the returned event stream. Mostly copy scan_link_label, but strip out everything unrelated to footnotes.

Comment thread tests/rustdoc-ui/lints/broken-footnote.rs
@@ -0,0 +1,7 @@
#![deny(rustdoc::broken_footnote)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A selection of many good corner cases to test this lint against can be found here: https://pulldown-cmark.github.io/pulldown-cmark/specs/footnotes.html

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Contributor

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #158498 (comment)

This is a separate issue from what caused me to start the try job
@bors r-

@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 27, 2026
@rust-bors

rust-bors Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

This pull request was unapproved.

View changes since this unapproval

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Ah my apologies. Gonna take a look at it tomorrow then.

@rustbot

This comment has been minimized.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Rebased the PR. Let's try the failed jobs.

@bors try jobs=auto,i686-gnu-nopt-1

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Jun 29, 2026
Add new `unused_footnote_definition` rustdoc lint


try-job: auto
try-job: i686-gnu-nopt-1
@rust-bors

rust-bors Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

💔 Test for a162f68 failed: CI. Failed job:

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

@bors try jobs=i686-gnu-nopt-1

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Jun 29, 2026
Add new `unused_footnote_definition` rustdoc lint


try-job: i686-gnu-nopt-1
@rust-bors

rust-bors Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

💔 Test for a366c3a failed: CI. Failed job:

@rust-log-analyzer

This comment has been minimized.

@rustbot

rustbot commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

I rebased and changed nothing. Test passed locally too. Gonna try again and investigate further if it still fails.

@bors try jobs=i686-gnu-nopt-1

@rust-bors

rust-bors Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

⌛ Trying commit 6a2cf7f with merge 13fc244

To cancel the try build, run the command @bors try cancel.

Workflow: https://github.com/rust-lang/rust/actions/runs/28453583673

rust-bors Bot pushed a commit that referenced this pull request Jun 30, 2026
Add new `unused_footnote_definition` rustdoc lint


try-job: i686-gnu-nopt-1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants