Skip to content

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Feb 28, 2025

I think in this case, instead of spawning unpportable_markdown, we should instead notify that this footnote reference doesn't have an associated definition.

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 Feb 28, 2025
@notriddle
Copy link
Contributor

Fixes #137740.

I think you might have misunderstood the issue description. "The term of what is meant by anchors is borrowed from" a description of this style of anchor links. It doesn't say anything about footnotes.

/// Consider [panics](std::alloc::GlobalAllocator#Panics) on allocation.
//                                               ^^^^^^^ anchor

@GuillaumeGomez
Copy link
Member Author

I indeed misunderstood and went on a completely different path. Although I still think that this PR is useful. I plan to send a follow-up PR for unused footnote references as well. ^^'

I removed the "fixes" mention though.

@notriddle
Copy link
Contributor

This lint seems too noisy to belong in core.

Review case F1 of #121659 (comment), where [^a-zA-Z0-9] is syntactically valid as a footnote reference, but not intended to be parsed as one. Following the suggestion (to add \ to the front) makes the plain-text representation harder to read, and Rustdoc is already doing what the author wanted, so where's the problem?

If the goal of this lint is to catch typo-ed footnotes, then there should be an unused footnote definition, somewhere, that the author tried and failed to reference. Those are guaranteed to be a problem: either they typo-ed the reference, or they never intended a footnote at all (but still got one).

@GuillaumeGomez
Copy link
Member Author

That's why we suggest to escape the [ if it was not intended. Like I said, I'm also planning to send a PR for unused footnote definitions. But even without it, I think it makes sense to warn that a footnote reference doesn't match anything and suggest to escape it if it's not meant to be a footnote.

@notriddle
Copy link
Contributor

That's why we suggest to escape the [ if it was not intended.

Doing that makes the doc comment less plain-text (noisier).

@GuillaumeGomez
Copy link
Member Author

After discussing with @notriddle, it was decided to move the implementation of this lint into its own file.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 7, 2025

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

github-merge-queue bot pushed a commit to rust-lang/rust-clippy that referenced this pull request Jun 5, 2025
changelog: [`doc_suspicious_footnotes`]: lint for text that looks like a
footnote reference but has no definition

This is an alternative to rust-lang/rust#137803,
meant to address the concerns about false positives.

This lint only fires when the apparent footnote reference has a name
that's made from pure ASCII digits. This choice is justified by running
lintcheck on the top 200 crates, plus the clippy default set:

1. [I ran
lintcheck](https://gist.github.com/notriddle/59072476c9c1fd569fee421270dad665)
with a modded version of this lint that didn't check for digits only. It
produced a false positive warning on a line in mdbook that had a regex,
and no true positives at all.
2. [I also ran
lintcheck](https://gist.github.com/notriddle/74eb8c9e1939b9f5c5549bf1d4fa238a)
with a custom lint that fired on any valid footnote reference with a
non-ascii-digit name. `cargo` uses one in its job_queue module, and
that's all it found.

cc @GuillaumeGomez
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants