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.

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

rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint (INVALID_RUST_CODEBLOCKS) #84587

Merged
merged 5 commits into from
May 18, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 26, 2021

Fixes #79792. This already went through FCP in #79816, so it only needs final review.

This is mostly a rebase of #79816 - thank you @poliorcetics for doing most of the work!

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Apr 26, 2021
@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(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 Apr 26, 2021
@jyn514 jyn514 changed the title Make "rust code block is empty" and "could not parse code block" warnings a lint rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint Apr 26, 2021
src/doc/rustdoc/src/lints.md Outdated Show resolved Hide resolved
@CraftSpider
Copy link
Contributor

I don't see a test for the actual empty block case, could you add one? Otherwise looks generally good, given that I'm not super familiar with the lint system.

@jyn514
Copy link
Member Author

jyn514 commented May 1, 2021

I don't see a test for the actual empty block case, could you add one?

It already exists: https://github.com/rust-lang/rust/blob/7f4d7327da6dbec5958d144a12e2e4709ebcbfc1/src/test/rustdoc-ui/invalid-syntax.stderr#L105-L111

Err ... actually I don't think there was ever an FCP in #79816. Also the lint name should be plural, not singular. I'll start FCP once I rename it.

@jyn514 jyn514 added T-rustdoc and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 1, 2021
@jyn514
Copy link
Member Author

jyn514 commented May 1, 2021

@rfcbot fcp merge

This adds a new lint INVALID_RUST_CODEBLOCKS. These warnings were already being emitting, but as a hard warning and not a lint, so they couldn't be allowed or denied.

I'm open to renaming the lint, but I do think changing these from a warning to a lint is the right decision.

@rfcbot
Copy link

rfcbot commented May 1, 2021

Team member @jyn514 has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 1, 2021
@jyn514 jyn514 changed the title rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint (INVALID_RUST_CODEBLOCKS) May 1, 2021
@jyn514 jyn514 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2021
| ^^^^^^^^
LL | /// ```ignore (to-prevent-tidy-error)
| ^^^
= note: error from rustc: character literal may only contain one codepoint
Copy link
Member

Choose a reason for hiding this comment

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

Is there an existing issue about this not pointing to the span that contains the error? Would be nice to fix in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Why was this behavior changed in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

The note just moved below the help, it wasn't attached to the correct span before either.

Copy link
Member Author

Choose a reason for hiding this comment

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

The existing issue is #67857.

Copy link
Member

Choose a reason for hiding this comment

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

it wasn't attached to the correct span before either.

Ah, I see 👍

src/doc/rustdoc/src/lints.md Outdated Show resolved Hide resolved
| ^^^^^^^^
LL | /// ```ignore (to-prevent-tidy-error)
| ^^^
= note: error from rustc: character literal may only contain one codepoint
Copy link
Member

Choose a reason for hiding this comment

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

Why was this behavior changed in this PR?

@camelid
Copy link
Member

camelid commented May 1, 2021

@rfcbot reviewed

I agree with making this a lint, but why was #84587 (comment) changed in this PR? I would prefer it to be a separate change.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 2, 2021
@rfcbot
Copy link

rfcbot commented May 2, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 15, 2021
…pider

rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint (`INVALID_RUST_CODEBLOCKS`)

Fixes rust-lang#79792. This already went through FCP in rust-lang#79816, so it only needs final review.

This is mostly a rebase of rust-lang#79816 - thank you `@poliorcetics` for doing most of the work!
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 15, 2021
…pider

rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint (`INVALID_RUST_CODEBLOCKS`)

Fixes rust-lang#79792. This already went through FCP in rust-lang#79816, so it only needs final review.

This is mostly a rebase of rust-lang#79816 - thank you ``@poliorcetics`` for doing most of the work!
@GuillaumeGomez
Copy link
Member

I think it'll need to be rebased:

    Checking tracing-subscriber v0.2.16
    Checking tracing-tree v0.1.9
    Checking rustdoc-json-types v0.1.0 (/checkout/src/rustdoc-json-types)
    Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
error[E0599]: no method named `as_local` found for enum `types::FakeDefId` in the current scope
  --> src/librustdoc/passes/check_code_block_syntax.rs:56:42
   |
56 |         let local_id = match item.def_id.as_local() {
   |                                          ^^^^^^^^ help: there is an associated function with a similar name: `is_local`
  ::: src/librustdoc/clean/types.rs:54:1
   |
   |
54 | crate enum FakeDefId {
   | -------------------- method `as_local` not found for this
error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `rustdoc`

@jyn514
Copy link
Member Author

jyn514 commented May 15, 2021

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 15, 2021
poliorcetics and others added 4 commits May 17, 2021 21:31
This adds a new lint to `rustc` that is used in rustdoc when a code
block is empty or cannot be parsed as valid Rust code.

Previously this was unconditionally a warning. As such some
documentation comments were (unknowingly) abusing this to pass despite
the `-Dwarnings` used when compiling `rustc`, this should not be the
case anymore.
This also gives a better error message when a span is missing.
- Simplify boolean expression
- Give an example of invalid syntax
- Remove explanation of why code block is text
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented May 18, 2021

@bors r=CraftSpider

@bors
Copy link
Contributor

bors commented May 18, 2021

📌 Commit 587c504 has been approved by CraftSpider

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 18, 2021
…pider

rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint (`INVALID_RUST_CODEBLOCKS`)

Fixes rust-lang#79792. This already went through FCP in rust-lang#79816, so it only needs final review.

This is mostly a rebase of rust-lang#79816 - thank you `@poliorcetics` for doing most of the work!
bors added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2021
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#84587 (rustdoc: Make "rust code block is empty" and "could not parse code block" warnings a lint (`INVALID_RUST_CODEBLOCKS`))
 - rust-lang#85280 (Toggle-wrap items differently than top-doc.)
 - rust-lang#85338 (Implement more Iterator methods on core::iter::Repeat)
 - rust-lang#85339 (Report an error if a lang item has the wrong number of generic arguments)
 - rust-lang#85369 (Suggest borrowing if a trait implementation is found for &/&mut <type>)
 - rust-lang#85393 (Suppress spurious errors inside `async fn`)
 - rust-lang#85415 (Clean up remnants of BorrowOfPackedField)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 07d11cf into rust-lang:master May 18, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 18, 2021
@jyn514 jyn514 deleted the rustdoc-lint-block branch May 18, 2021 18:18
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 20, 2021
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

rustdoc -D warnings doesn't fail for all warnings