Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Find more invalid doc attributes #83098

Merged
merged 8 commits into from
Mar 15, 2021
Merged

Find more invalid doc attributes #83098

merged 8 commits into from
Mar 15, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Mar 13, 2021 •
edited
Loading

  • Lint on #[doc(123)], #[doc("hello")], etc.
  • Lint every attribute; e.g., will now report two warnings for #[doc(foo, bar)]
  • Add hyphen to "crate level"
  • Display paths like #[doc(foo::bar)] correctly instead of as an empty string

@camelid
Refactor check_doc_attrs body
9613a88 
This change makes it easier to follow the control flow.

I also moved the end-of-line comments attached to some symbols to before
the symbol listing. This allows rustfmt to format the code; otherwise no
formatting occurs (see rust-lang/rustfmt#4750).
@camelid
Lint non-meta doc attributes
7189c05 
E.g., `#[doc(123)]`.
@camelid camelid added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. 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 Mar 13, 2021
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
camelid
camelid commented Mar 13, 2021
View reviewed changes
compiler/rustc_passes/src/check_attr.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/check_attr.rs Outdated Show resolved Hide resolved
src/test/ui/attributes/doc-attr.stderr Outdated
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #82730 <https://github.com/rust-lang/rust/issues/82730>

error: unknown `doc` attribute ``
Copy link
Member Author

Choose a reason for hiding this comment

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

How do I display the Path? There doesn't seem to be a Display impl.

Note that this is not a regression; this test is just exposing the previous behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of name_or_empty, could use name_value_literal_span and then get the snippet for that Span? Or is path_to_string accessible here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think rustc_ast_pretty is accessible here, though I could add it as a dependency. I'm trying out the span_to_snippet approach now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both span_to_snippet and path_to_string worked. I'm leaning towards path_to_string because I think it will handle foo :: bar:: baz gracefully as foo::bar::baz, but which one would you prefer I use? You can see what the code looks like with span_to_snippet vs path_to_string here: 8f40e11

Copy link
Member

Choose a reason for hiding this comment

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

I'd normally want to lean against introducing more dependencies between our crates, but depending on rust_ast_pretty when we already depend on rust_ast is probably fine. Let's stick with path_to_string.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like rustc_passes already depends transitively on rustc_ast_pretty anyway.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Add hyphen to "crate level"
5134047 
"crate level attribute" -> "crate-level attribute"
@camelid
Update rustdoc-ui versions of the doc-attr test
13884dc 
It seems there are two copies of it: one in `src/test/ui/attributes/`
and one in `src/test/rustdoc-ui/`. I'm guessing this is to test that the
lint is emitted both when you run the compiler and when you run rustdoc.
davidtwco
davidtwco approved these changes Mar 14, 2021
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Two minor nits in existing comments, otherwise LGTM.

@camelid camelid 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 Mar 14, 2021
@camelid
Tweak diagnostics
13076f9 
- Tweak lint message
- Display multi-segment paths correctly
@camelid
Use pretty-printer instead of span_to_snippet
8f40e11
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2021
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2021

📌 Commit 8f40e11 has been approved by davidtwco

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 14, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 15, 2021
@Dylan-DPC
Rollup merge of rust-lang#83098 - camelid:more-doc-attr-check, r=davi…
cbbf2d4 
…dtwco

Find more invalid doc attributes

- Lint on `#[doc(123)]`, `#[doc("hello")]`, etc.
- Lint every attribute; e.g., will now report two warnings for `#[doc(foo, bar)]`
- Add hyphen to "crate level"
- Display paths like `#[doc(foo::bar)]` correctly instead of as an empty string
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 15, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 15, 2021
@Dylan-DPC
Rollup merge of rust-lang#83098 - camelid:more-doc-attr-check, r=davi…
3ec47de 
…dtwco

Find more invalid doc attributes

- Lint on `#[doc(123)]`, `#[doc("hello")]`, etc.
- Lint every attribute; e.g., will now report two warnings for `#[doc(foo, bar)]`
- Add hyphen to "crate level"
- Display paths like `#[doc(foo::bar)]` correctly instead of as an empty string
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 15, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 15, 2021
@Dylan-DPC
Rollup merge of rust-lang#83098 - camelid:more-doc-attr-check, r=davi…
18abf7b 
…dtwco

Find more invalid doc attributes

- Lint on `#[doc(123)]`, `#[doc("hello")]`, etc.
- Lint every attribute; e.g., will now report two warnings for `#[doc(foo, bar)]`
- Add hyphen to "crate level"
- Display paths like `#[doc(foo::bar)]` correctly instead of as an empty string
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 15, 2021
Closed
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 15, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2021
@bors
Auto merge of rust-lang#83149 - Dylan-DPC:rollup-ov70c5v, r=Dylan-DPC
2ccf063 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#82989 (Custom error on literal names from other languages)
 - rust-lang#83054 (Validate rustc_layout_scalar_valid_range_{start,end} attributes)
 - rust-lang#83098 (Find more invalid doc attributes)
 - rust-lang#83108 (Remove unused `opt_local_def_id_to_hir_id` function)
 - rust-lang#83110 (Fix typos in `library/core/src/ptr/mod.rs` and `library/std/src/sys_common/thread_local_dtor.rs`)
 - rust-lang#83113 (Minor refactoring in try_index_step)
 - rust-lang#83127 (Introduce `proc_macro_back_compat` lint, and emit for `time-macros-impl`)
 - rust-lang#83132 (Don't encode file information for span with a dummy location)
 - rust-lang#83141 (:arrow_up: rust-analyzer)
 - rust-lang#83144 (Introduce `rustc_interface::interface::Config::parse_sess_created` callback)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 75a15bf into rust-lang:master Mar 15, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 15, 2021
@camelid camelid deleted the more-doc-attr-check branch March 15, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. 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.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
6 participants
@camelid @rust-highfive @rust-log-analyzer @davidtwco @bors @rustbot