[rustdoc] Do not emit redundant_explicit_links lint if the doc comment comes from expansion #141648
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
redundant_explicit_links-expansion
branch
from
5e8b79b to
ec9530a
Compare
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #141869) made this pull request unmergeable. Please resolve the merge conflicts. |
|
(I've been rather busy catching up with stuff after vacation; I probably won't be able to review this myself soon ,sorry!) |
GuillaumeGomez
force-pushed
the
redundant_explicit_links-expansion
branch
from
b486863 to
9ac0d61
Compare
This comment has been minimized.
This comment has been minimized.
|
No worries, take your time. We can pick another reviewer in the meantime. r? notriddle |
GuillaumeGomez
force-pushed
the
redundant_explicit_links-expansion
branch
from
0ada37b to
8bcc067
Compare
|
Applied suggestions and added more tests. |
| 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()) |
There was a problem hiding this comment.
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.
| #[doc = mac3!()] | ||
| /// a [`BufferProvider`](crate::BufferProvider). | ||
| //~^ ERROR: redundant_explicit_links | ||
| pub fn f() {} |
There was a problem hiding this comment.
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.
GuillaumeGomez
force-pushed
the
redundant_explicit_links-expansion
branch
from
8bcc067 to
8c7779b
Compare
|
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? |
| @@ -659,8 +681,13 @@ pub fn source_span_for_markdown_range_inner( | |||
| } | |||
| } | |||
|
|
|||
| Some(span_of_fragments(fragments)?.from_inner(InnerSpan::new( | |||
| let (span, _) = span_of_fragments_with_expansion(fragments)?; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It would make a lot of unrelated changes, like in clippy. A follow-up is likely better to prevent blocking this fix.
…t comes from expansion
…ing from expansion
…it_links` new API
GuillaumeGomez
force-pushed
the
redundant_explicit_links-expansion
branch
from
8c7779b to
3b5525b
Compare
|
Applied suggestions. |
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