Skip to content

improve doc attr span tracking for proc-macro - #17678

Merged
blyxyas merged 1 commit into
rust-lang:masterfrom
andogq:fix/doc-attr-checks
Sep 23, 2026
Merged

blyxyas merged 1 commit into
rust-lang:masterfrom
andogq:fix/doc-attr-checks

Conversation

@andogq

@andogq andogq commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Resolves #16317
Resolves #16382
Resolves #16713

Largely based on #16713.

Lint documentation is contained within doc-comments within a macro invocation. Therefore, any lints were not applied to them up until now. A separate commit addresses these problems.

changelog: [empty_docs]: lint user-provided doc on proc-macro expanded codes
changelog: [missing_safety_doc]: lint user-provided doc on proc-macro expanded codes
changelog: [missing_panics_doc]: lint user-provided doc on proc-macro expanded codes

@rustbot rustbot added the S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. label Sep 3, 2026
@rustbot

rustbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in clippy_lints/src/doc

cc @notriddle

@rustbot

rustbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 3, 2026
@rustbot

This comment has been minimized.

@andogq
andogq force-pushed the fix/doc-attr-checks branch from 0fcb239 to ff99bf7 Compare September 3, 2026 05:11
@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown

Lintcheck changes for 1b55172

Lint Added Removed Changed
clippy::doc_markdown 8 13 5
clippy::doc_paragraphs_missing_punctuation 1 0 1
clippy::too_long_first_doc_paragraph 1 0 0

This comment will be updated if you push new changes

@andogq
andogq force-pushed the fix/doc-attr-checks branch from ff99bf7 to a14ea79 Compare September 3, 2026 09:35

@andogq andogq Sep 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(follow on from #16713 (comment))

Only some of these item kinds produced the 'missing backticks' clippy::doc_markdown lint due to a very specific requirement in order to resolve a Span from a markdown string:


All that being said, I feel that properly backticking these item kinds is appropriate.

View changes since the review

@andogq
andogq force-pushed the fix/doc-attr-checks branch from a14ea79 to 02eac0e Compare September 3, 2026 09:56

@CommanderStorm CommanderStorm 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.

Community review: LGTM.

Maybe the test coverage could be improved a bit, since it is currently only #[doc..] and not ///.
Though as you noted (is my understanding of ?), this is an false negative currently, so 🤷🏻‍♂️

View changes since this review

@CommanderStorm

Copy link
Copy Markdown
Contributor

(also if you are nice, make sure to include an Resolves #16713 in the PR description, since I think closing also works with PRs)

@andogq

andogq commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

make sure to include an Resolves #16713 in the PR description

Good idea, thanks!

test coverage could be improved

These issues occur when doc comments are in the #[doc = "..."] form. The existing test cases only handle /// ..., so these new test cases close that gap. If there's any specific tests you'd like to see however, I'd be happy to add them in.

@CommanderStorm

CommanderStorm commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

These issues occur when doc comments are in the #[doc = "..."] form.

Yes, but adding test coverage for the other form would be nice to have too, even if just to confirm that we don't regress on this one. So the combination of external! and ///, everywhere where you added the combination of external! and #[doc..]

As I alured, this one is for bonus points and not something that is required 😉

@rustbot rustbot removed the S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. label Sep 3, 2026
@rustbot

rustbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

r? @blyxyas

rustbot has assigned @blyxyas for the project review.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: 9 candidates
  • 9 candidates expanded to 9 candidates
  • Random selection from 6 candidates

@rustbot

This comment has been minimized.

@andogq
andogq force-pushed the fix/doc-attr-checks branch from 02eac0e to 6302650 Compare September 14, 2026 02:46
@rustbot

rustbot commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different master 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.

Comment thread clippy_lints/src/bool_to_int_with_if.rs Outdated
Comment thread clippy_lints/src/doc/mod.rs Outdated
Comment thread clippy_lints/src/doc/mod.rs Outdated
@andogq
andogq force-pushed the fix/doc-attr-checks branch from 6302650 to 17964ca Compare September 22, 2026 02:17
@andogq
andogq requested a review from blyxyas September 22, 2026 04:34
Comment on lines +883 to +891
let (_, kind) = attr.doc_str_and_fragment_kind()?;
let (is_raw, doc_str_span) = match kind {
DocFragmentKind::Sugared(_) => (false, attr.span()),
DocFragmentKind::Raw(span) => (true, span),
};
let from_include_str_macro = is_raw
&& !attr.span().in_external_macro(cx.sess().source_map())
&& originated_from_include_str_macro(cx, doc_str_span);
(from_include_str_macro || !doc_str_span.in_external_macro(cx.sess().source_map())).then_some((attr, None))

@blyxyas blyxyas Sep 22, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Much better! ❤️

View changes since the review

@blyxyas blyxyas left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for addressing this issue! ❤️ ฅ•ω•ฅ

Could you squash these commits?

View changes since this review

@andogq
andogq force-pushed the fix/doc-attr-checks branch from 17964ca to 1b55172 Compare September 23, 2026 00:14
@andogq

andogq commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

@blyxyas Thank you for the review! Commits have been squashed, let me know if there's anything else that's needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False negative of empty_docs when processing proc-macros clippy::missing_safety_doc triggers on cxx::bridge-generated code

5 participants