[rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion by GuillaumeGomez · Pull Request #141648 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation49 Commits5 Checks10 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Fixes #141553.

The problem was that we change the context for the attributes in some cases to get better error output, preventing us to detect if the attribute comes from expansion. Most of the changes are about keeping track of the "does this span comes from expansion" information.

r? @Manishearth

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-rustdoc

Relevant to the rustdoc team, which will review and decide on the PR/issue.

labels

May 27, 2025

This comment has been minimized.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

This comment has been minimized.

Choose a reason for hiding this comment

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

Mostly nits about comments, but a few concerns about the actual logic as well.

(I've been rather busy catching up with stuff after vacation; I probably won't be able to review this myself soon ,sorry!)

This comment has been minimized.

No worries, take your time. We can pick another reviewer in the meantime.

r? notriddle

Applied suggestions and added more tests.

Choose a reason for hiding this comment

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

There's 1 edge case I think this doesn't handle 100% correctly, and other than that I just have a few nits about wording, 1 tiny inefficiency, and some comments about how some control flow could be rewritten to use pattern matching (if we wanted that)

Comment on lines 212 to +216

if let Some((doc_str, comment_kind)) = attr.doc_str_and_comment_kind() {
let doc = beautify_doc_string(doc_str, comment_kind);
let (span, kind) = if attr.is_doc_comment() {
(attr.span(), DocFragmentKind::SugaredDoc)
let (span, kind, from_expansion) = if attr.is_doc_comment() {
let span = attr.span();
(span, DocFragmentKind::SugaredDoc, span.from_expansion())

Choose a reason for hiding this comment

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

This is actually a bit odd to me, doc_str_and_comment_kind knows if a doc fragment is raw or a comment, but it then we throw that info away only to recalculate it immediately. These functions probably get inlined and probably llvm de-duplicates the match, but it's still a bit messy, especially considering this is the only call site of doc_str_and_comment_kind besides 1 place in clippy that only checks if the return value is None.

Comment on lines +36 to +40

#[doc = mac3!()]
/// a [`BufferProvider`](crate::BufferProvider).
//~^ ERROR: redundant_explicit_links
pub fn f() {}

Choose a reason for hiding this comment

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

I kinda would like to see some tests exercising the all-sugared doc fragment case, where half of the fragments are macro-generated and half aren't. Ideally there would be two cases for this, one where the redundant link is in the macro generated fragment and thus the lint is suppressed, and one where the redundant link is in the non-macro generated fragment and thus the link is emitted.

I understand that this bail-out is needed because, in the event of a false positive, disabling the lint in the macro is impossible. There's nowhere to attach the attribute.

In that case, would it be simpler and make more sense to disable all of the markdown lints, and not just this one?

Choose a reason for hiding this comment

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

we can revert span_for_fragments
more places could use if/else

}

Some(span_of_fragments(fragments)?.from_inner(InnerSpan::new(
let (span, _) = span_of_fragments_with_expansion(fragments)?;

Choose a reason for hiding this comment

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

We can revert span_of_fragments_with_expansion to just be span_of_fragments again now, right?

any future potential users of it would probably be suspect to the same edge case as is present here, and having a useless iteration isn't ideal.

Choose a reason for hiding this comment

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

It would make a lot of unrelated changes, like in clippy. A follow-up is likely better to prevent blocking this fix.

Choose a reason for hiding this comment

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

I mean, I would agree with that reasoning, although it does make me wonder somewhat what your criteria for when a working but not ideal solution should be merged, given your review on #142653

Choose a reason for hiding this comment

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

Current PR is a bugfix. #142653 is a behaviour change introducing potentially a performance regression.

Choose a reason for hiding this comment

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

Should probably get a followup PR to clean up some oddities, but is definitely an improvement over the status quo.

Thanks for the review! Let's approve so this bug can finally be removed. :)

@bors r=lolbinarycat rollup

📌 Commit 3b5525b has been approved by lolbinarycat

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jun 25, 2025

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Jun 26, 2025

…links-expansion, r=lolbinarycat

[rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion

Fixes rust-lang#141553.

r? @Manishearth

bors added a commit that referenced this pull request

Jun 26, 2025

Rollup of 17 pull requests

Successful merges:

#124595 (Suggest cloning Arc moved into closure)
#139594 (Simplify ObligationCauseCode::IfExpression)
#141311 (make tidy-alphabetical use a natural sort)
#141648 ([rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion)
#142255 (Add edition checks for some tests that had divergent output)
#142285 (tests: Do not run afoul of asm.validity.non-exhaustive in input-stats)
#142549 (small iter.intersperse.fold() optimization)
#142637 (Remove some glob imports from the type system)
#142647 ([perf] Compute hard errors without diagnostics in impl_intersection_has_impossible_obligation)
#142700 (Remove incorrect comments in Weak)
#142884 (StableMIR: Add method to retrieve body of coroutine)
#142925 (Rewrite .gitattributes CRLF ui tests into run-make tests)
#143001 (Rename run always )
#143010 (Update browser-ui-test version to 0.20.7)
#143015 (Add sym::macro_pin diagnostic item for core::pin::pin!())
#143020 (codegen_fn_attrs: make comment more precise)
#143033 (Expand const-stabilized API links in relnotes)

r? @ghost @rustbot modify labels: rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jun 26, 2025

…links-expansion, r=lolbinarycat

[rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion

Fixes rust-lang#141553.

r? @Manishearth

bors added a commit that referenced this pull request

Jun 26, 2025

Rollup of 9 pull requests

Successful merges:

#124595 (Suggest cloning Arc moved into closure)
#139594 (Simplify ObligationCauseCode::IfExpression)
#141311 (make tidy-alphabetical use a natural sort)
#141648 ([rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion)
#142285 (tests: Do not run afoul of asm.validity.non-exhaustive in input-stats)
#142393 (Don't give APITs names with macro expansion placeholder fragments in it)
#142884 (StableMIR: Add method to retrieve body of coroutine)
#142981 (Make missing lifetime suggestion verbose)
#143030 (Fix suggestion spans inside macros for the unused_must_use lint)

r? @ghost @rustbot modify labels: rollup

compiler-errors added a commit to compiler-errors/rust that referenced this pull request

Jun 26, 2025

…links-expansion, r=lolbinarycat

[rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion

Fixes rust-lang#141553.

r? @Manishearth

bors added a commit that referenced this pull request

Jun 26, 2025

Rollup of 8 pull requests

Successful merges:

#124595 (Suggest cloning Arc moved into closure)
#139594 (Simplify ObligationCauseCode::IfExpression)
#141311 (make tidy-alphabetical use a natural sort)
#141648 ([rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion)
#142285 (tests: Do not run afoul of asm.validity.non-exhaustive in input-stats)
#142393 (Don't give APITs names with macro expansion placeholder fragments in it)
#142884 (StableMIR: Add method to retrieve body of coroutine)
#142981 (Make missing lifetime suggestion verbose)

r? @ghost @rustbot modify labels: rollup

bors added a commit that referenced this pull request

Jun 26, 2025

Rollup of 8 pull requests

Successful merges:

#124595 (Suggest cloning Arc moved into closure)
#139594 (Simplify ObligationCauseCode::IfExpression)
#141311 (make tidy-alphabetical use a natural sort)
#141648 ([rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion)
#142285 (tests: Do not run afoul of asm.validity.non-exhaustive in input-stats)
#142393 (Don't give APITs names with macro expansion placeholder fragments in it)
#142884 (StableMIR: Add method to retrieve body of coroutine)
#142981 (Make missing lifetime suggestion verbose)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Jun 27, 2025

Rollup merge of #141648 - GuillaumeGomez:redundant_explicit_links-expansion, r=lolbinarycat

[rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion

Fixes #141553.

r? @Manishearth

flip1995 pushed a commit to flip1995/rust that referenced this pull request

Jul 10, 2025

…links-expansion, r=lolbinarycat

[rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion

Fixes rust-lang#141553.

r? @Manishearth

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-rustdoc

Relevant to the rustdoc team, which will review and decide on the PR/issue.