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

Split non macro portion of unused_doc_comment from macro part into two passes/lints #69084

Merged
merged 7 commits into from Feb 23, 2020

Conversation

@yaahc
Copy link
Contributor

yaahc commented Feb 12, 2020

Motivation

This change is motivated by the needs of the spandoc library. The specific use case is that my macro is removing doc comments when an attribute is applied to a fn with doc comments, but I would like the lint to still appear when I forget to add the #[spandoc] attribute to a fn, so I don't want to have to silence the lint globally.

Approach

This change splits the unused _doc_comment lint into two lints, unused_macro_doc_comment and unused_doc_comment. The non macro portion is moved into an early_lint_pass rather than a pre_expansion_pass. This allows proc macros to silence unused_doc_comment warnings by either adding an attribute to silence it or by removing the doc comment before the early_pass runs.

The unused_macro_doc_comment lint however will still be impossible for proc-macros to silence, but the only alternative that I can see is to remove this lint entirely, which I don't think is acceptable / is a decision I'm not comfortable making personally, so instead I opted to split the macro portion of the check into a separate lint so that it can be silenced globally with an attribute if necessary without needing to globally silence the unused_doc_comment lint as well, which is still desireable.

fixes #67838

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 12, 2020

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 12, 2020

@bors r+

I was initially concerned that this would break doc comments for macros, but this is just a lint, it's fine. We should not be linting about pretty much anything pre-proc macros except for things which affect lexing, which is true for the other pre-proc-macro pass.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2020

📌 Commit 0386323 has been approved by Manishearth

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Feb 12, 2020

This does break a change made last year to improve diagnostics on macros

tree cabd6022a0ec9a157fdce48be26ddc408d626219
parent 018d4d265fb40ea4301da5dc339bff962a6faa57
author Andy Russell <arussell123@gmail.com> Thu Jan 24 15:49:03 2019 -0500
committer Andy Russell <arussell123@gmail.com> Fri Mar 8 12:39:50 2019 -0500

expand unused doc comment diagnostic

Report the diagnostic on macro expansions, and add a label indicating
why the comment is unused.
@@ -802,7 +802,14 @@ impl LintPass for UnusedDocComment {
 }
 
 impl UnusedDocComment {
-    fn warn_if_doc(&self, cx: &EarlyContext, attrs: &[ast::Attribute]) {
+    fn warn_if_doc(
+        &self,
+        cx: &EarlyContext<'_>,
+        node_span: Span,
+        node_kind: &str,
+        is_macro_expansion: bool,
+        attrs: &[ast::Attribute]
+    ) {
         let mut attrs = attrs.into_iter().peekable();
 
         // Accumulate a single span for sugared doc comments.
@@ -825,27 +832,51 @@ impl UnusedDocComment {
             let span = sugared_span.take().unwrap_or_else(|| attr.span);
 
             if attr.name() == "doc" {
-                cx.struct_span_lint(
-                    UNUSED_DOC_COMMENTS,
-                    span,
-                    "doc comment not used by rustdoc",
-                ).emit();
+                let mut err = cx.struct_span_lint(UNUSED_DOC_COMMENTS, span, "unused doc comment");
+
+                err.span_label(
+                    node_span,
+                    format!("rustdoc does not generate documentation for {}", node_kind)
+                );
+
+                if is_macro_expansion {
+                    err.help("to document an item produced by a macro, \
+                              the macro must produce the documentation as part of its expansion");
+                }
+
+                err.emit();
             }
         }
     }
 }

I'd like to not break this if possible, is it possible for a lint to exist in two passes, I really only need the lint on items in fns to be done after expansion so I can strip out doc comments in fn bodies.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 12, 2020

@yaahc write two passes instead, one that specifically looks for doc comments on macro expansions only. multiple passes are allowed to produce the same lint

@bors r-

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Feb 12, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-02-12T03:42:50.7331339Z ========================== Starting Command Output ===========================
2020-02-12T03:42:50.7333350Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/c0481a41-7933-4ed4-8154-56d13d486a72.sh
2020-02-12T03:42:50.7333389Z 
2020-02-12T03:42:50.7336886Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-12T03:42:50.7342746Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69084/merge to s
2020-02-12T03:42:50.7344208Z Task         : Get sources
2020-02-12T03:42:50.7344254Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-12T03:42:50.7344287Z Version      : 1.0.0
2020-02-12T03:42:50.7344540Z Author       : Microsoft
---
2020-02-12T03:42:51.8331524Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-12T03:42:51.8345576Z ##[command]git config gc.auto 0
2020-02-12T03:42:51.8348006Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-12T03:42:51.8351250Z ##[command]git config --get-all http.proxy
2020-02-12T03:42:51.8358222Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69084/merge:refs/remotes/pull/69084/merge
---
2020-02-12T04:12:51.4997231Z Assembling stage1 compiler (x86_64-unknown-linux-gnu)
2020-02-12T04:12:51.5016978Z Building stage1 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-02-12T04:12:51.9532691Z    Compiling cc v1.0.50
2020-02-12T04:12:51.9533062Z    Compiling core v0.0.0 (/checkout/src/libcore)
2020-02-12T04:12:52.3733717Z thread 'rustc' panicked at 'src/librustc_lint/context.rs:179: duplicate specification of lint unused_doc_comments', src/librustc/util/bug.rs:37:26
2020-02-12T04:12:52.3741021Z 
2020-02-12T04:12:52.3745001Z error: internal compiler error: unexpected panic
2020-02-12T04:12:52.3747773Z 
2020-02-12T04:12:52.3751935Z note: the compiler unexpectedly panicked. this is a bug.
2020-02-12T04:12:52.3751935Z note: the compiler unexpectedly panicked. this is a bug.
2020-02-12T04:12:52.3753299Z 
2020-02-12T04:12:52.3759067Z note: we would appreciate a bug report: ***/blob/master/CONTRIBUTING.md#bug-reports
2020-02-12T04:12:52.3763368Z note: rustc 1.43.0-nightly (932f50894 2020-02-12) running on x86_64-unknown-linux-gnu
2020-02-12T04:12:52.3764497Z 
2020-02-12T04:12:52.3764497Z 
2020-02-12T04:12:52.3770128Z note: compiler flags: -Z macro-backtrace -Z binary-dep-depinfo -Z force-unstable-if-unmarked -C opt-level=3 -C codegen-units=1 -C debuginfo=0 -C link-args=-Wl,-rpath,$ORIGIN/../lib -C prefer-dynamic -C llvm-args=-import-instr-limit=10 -C debug-assertions=n --crate-type lib
2020-02-12T04:12:52.3774086Z note: some of the compiler flags provided by cargo are hidden
2020-02-12T04:12:52.3815228Z 
2020-02-12T04:12:52.4168956Z error: could not compile `core`.
2020-02-12T04:12:52.4179956Z warning: build failed, waiting for other jobs to finish...
---
2020-02-12T04:12:58.7923765Z   local time: Wed Feb 12 04:12:58 UTC 2020
2020-02-12T04:12:58.8851931Z   network time: Wed, 12 Feb 2020 04:12:58 GMT
2020-02-12T04:12:58.8854570Z == end clock drift check ==
2020-02-12T04:13:00.3423539Z 
2020-02-12T04:13:00.3518173Z ##[error]Bash exited with code '1'.
2020-02-12T04:13:00.3529979Z ##[section]Finishing: Run build
2020-02-12T04:13:00.3551814Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69084/merge to s
2020-02-12T04:13:00.3553398Z Task         : Get sources
2020-02-12T04:13:00.3553440Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-12T04:13:00.3553498Z Version      : 1.0.0
2020-02-12T04:13:00.3553533Z Author       : Microsoft
2020-02-12T04:13:00.3553533Z Author       : Microsoft
2020-02-12T04:13:00.3553575Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-12T04:13:00.3553648Z ==============================================================================
2020-02-12T04:13:00.7926765Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-12T04:13:00.7972887Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69084/merge to s
2020-02-12T04:13:00.8089911Z Cleaning up task key
2020-02-12T04:13:00.8090814Z Start cleaning up orphan processes.
2020-02-12T04:13:00.8235017Z Terminate orphan process: pid (3460) (python)
2020-02-12T04:13:00.8878082Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 12, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2020

📌 Commit a89a791 has been approved by Manishearth

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Feb 12, 2020

Actually, I think I need to disable the macro prepass lint for check_stmt. I think this will be okay because we probably dont have people commonly trying to declare items in macros as statements and document them at the same time...

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 12, 2020

@bors r- delegate+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2020

✌️ @yaahc can now approve this pull request

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Feb 12, 2020

So for my needs I would definitely prefer that doc comments on macro calls as statements are still handled in the early_pass so they can be silenced. I tried removing the stmt check in the pre_expansion pass lint with the hope that expr / stmt checks would catch the attributes on the post expansion version but the attributes seem to disappear and we no longer produce a lint on the macro call within the fn body, even if I changed the macro call to actually expand to an expr.

Could I get some help figuring out how to correctly lint for doc comments on macros as expressions from an early pass lint or decide which would be preferable between not linting in this case at all or linting on this but reporting the lint as unused_macro_doc_comment so that it can be silenced independently, either of which would be okay though not ideal solutions for my problem.

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Feb 12, 2020

I went ahead and implemented the version that exports the macro portion of the lint as a separately named lint, in addition to having it implemented as a separate lint pass internally. I figured this would be the most likely solution we'd end up needing.

If someone knows of a way to still appropriately lint on the attributes on macros after they've been expanded then I'm down to put in the work to swap the solution to that, but it looks like prior to this being moved to be a preexpansion pass lint the lint just straight up didn't trigger for macros, so my expectation is that attributes on macros are discarded after expansion, and that this is the only way to preserve the lint but while not requiring a blanket allow on all unused doc comments to silence the macro lint warnings.

@petrochenkov petrochenkov self-assigned this Feb 12, 2020
@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 12, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2020

📌 Commit df6eb29 has been approved by Manishearth

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Feb 12, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-02-12T06:45:21.6918066Z ========================== Starting Command Output ===========================
2020-02-12T06:45:21.7189109Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/5183fbc9-d6b9-4868-b3ea-de2f5e170ca1.sh
2020-02-12T06:45:21.7189311Z 
2020-02-12T06:45:21.7192499Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-12T06:45:21.7198998Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69084/merge to s
2020-02-12T06:45:21.7201454Z Task         : Get sources
2020-02-12T06:45:21.7201491Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-12T06:45:21.7201575Z Version      : 1.0.0
2020-02-12T06:45:21.7201612Z Author       : Microsoft
---
2020-02-12T06:45:22.5581913Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-12T06:45:22.5709064Z ##[command]git config gc.auto 0
2020-02-12T06:45:22.5772649Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-12T06:45:22.5835238Z ##[command]git config --get-all http.proxy
2020-02-12T06:45:22.5975697Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69084/merge:refs/remotes/pull/69084/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 12, 2020

@bors rollup=never
(I'd like to review this.)

@yaahc

This comment has been minimized.

Copy link
Contributor Author

yaahc commented Feb 22, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 22, 2020

📌 Commit 09bc5e3 has been approved by yaahc

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

⌛️ Testing commit 09bc5e3 with merge ca07a23...

bors added a commit that referenced this pull request Feb 23, 2020
Split non macro portion of unused_doc_comment from macro part into two passes/lints

## Motivation

This change is motivated by the needs of the [spandoc library](https://github.com/yaahc/spandoc). The specific use case is that my macro is removing doc comments when an attribute is applied to a fn with doc comments, but I would like the lint to still appear when I forget to add the `#[spandoc]` attribute to a fn, so I don't want to have to silence the lint globally.

## Approach

This change splits the `unused _doc_comment` lint into two lints, `unused_macro_doc_comment` and `unused_doc_comment`. The non macro portion is moved into an `early_lint_pass` rather than a pre_expansion_pass. This allows proc macros to silence `unused_doc_comment` warnings by either adding an attribute to silence it or by removing the doc comment before the early_pass runs.

The `unused_macro_doc_comment` lint however will still be impossible for proc-macros to silence, but the only alternative that I can see is to remove this lint entirely, which I don't think is acceptable / is a decision I'm not comfortable making personally, so instead I opted to split the macro portion of the check into a separate lint so that it can be silenced globally with an attribute if necessary without needing to globally silence the `unused_doc_comment` lint as well, which is still desireable.

fixes #67838
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

💔 Test failed - checks-azure

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 23, 2020

@yaahc
Meta: the convention is to @bors r=petrochenkov on behalf of the reviewer instead of @bors r+ing on behalf of yourself.

The CI failure looks spurious.
@bors r-

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 23, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: #69218
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

📌 Commit 09bc5e3 has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

⌛️ Testing commit 09bc5e3 with merge 0cbf1a6...

bors added a commit that referenced this pull request Feb 23, 2020
Split non macro portion of unused_doc_comment from macro part into two passes/lints

## Motivation

This change is motivated by the needs of the [spandoc library](https://github.com/yaahc/spandoc). The specific use case is that my macro is removing doc comments when an attribute is applied to a fn with doc comments, but I would like the lint to still appear when I forget to add the `#[spandoc]` attribute to a fn, so I don't want to have to silence the lint globally.

## Approach

This change splits the `unused _doc_comment` lint into two lints, `unused_macro_doc_comment` and `unused_doc_comment`. The non macro portion is moved into an `early_lint_pass` rather than a pre_expansion_pass. This allows proc macros to silence `unused_doc_comment` warnings by either adding an attribute to silence it or by removing the doc comment before the early_pass runs.

The `unused_macro_doc_comment` lint however will still be impossible for proc-macros to silence, but the only alternative that I can see is to remove this lint entirely, which I don't think is acceptable / is a decision I'm not comfortable making personally, so instead I opted to split the macro portion of the check into a separate lint so that it can be silenced globally with an attribute if necessary without needing to globally silence the `unused_doc_comment` lint as well, which is still desireable.

fixes #67838
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 23, 2020

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

💔 Test failed - checks-azure

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 23, 2020

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

⌛️ Testing commit 09bc5e3 with merge b1f395d...

@bors bors mentioned this pull request Feb 23, 2020
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 23, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing b1f395d to master...

@bors bors added the merged-by-bors label Feb 23, 2020
@bors bors merged commit b1f395d into rust-lang:master Feb 23, 2020
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20200222.53 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@yaahc yaahc deleted the yaahc:delayed-doc-lint branch Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.