Throw errors when doc comments are added where they're unused #43009

GuillaumeGomez · 2017-07-01T23:38:38Z

#42617

rust-highfive · 2017-07-01T23:38:49Z

r? @nikomatsakis

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

Mark-Simulacrum

I feel like this needs a warning cycle... or at least a crater run.

Mark-Simulacrum · 2017-07-01T23:40:35Z

src/test/compile-fail/issue-34222.rs

@@ -13,6 +13,6 @@

 #[rustc_error]
 fn main() { //~ ERROR compilation successful
-    /// crash
+    // crash
    let x = 0;
 }


We might as well remove this test -- it now tests nothing.

And this test still needs removing

GuillaumeGomez · 2017-07-02T09:40:34Z

@Mark-Simulacrum: I remember that we talked about it but can't remember where and when so couldn't find the discussion... But putting warnings instead is fine for me.

nikomatsakis · 2017-07-07T16:55:55Z

Crater run initiated.

Building toolchains 1, 2
Running tests
Await quiescence
Generate report

nikomatsakis · 2017-07-10T15:43:38Z

I was still unable to build one of the toolchains (toolchain 1, if you follow the links above):

error[E0585]�(B: found a documentation comment that doesn't document anything�(B
   �(B--> �(B/root/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-bidi-0.3.3/src/lib.rs:656:9�(B
    �(B|�(B
656�(B �(B| �(B        /// BidiTest:69635 (AL ET EN)�(B
    �(B| �(B        �(B^^^^^^^^^^^^^^^^^^^^^^^^^^^^^�(B
    �(B|�(B
    �(B= �(Bhelp�(B: doc comments must come before what they document, maybe a comment was intended with `//`?�(B
error[E0585]�(B: found a documentation comment that doesn't document anything�(B
   �(B--> �(B/root/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-bidi-0.3.3/src/lib.rs:798:9�(B
    �(B|�(B
798�(B �(B| �(B        /// BidiTest:946 (LRI PDI)�(B
    �(B| �(B        �(B^^^^^^^^^^^^^^^^^^^^^^^^^^�(B

GuillaumeGomez · 2017-07-11T12:33:53Z

So I need to make a lint out of this. Looking into https://github.com/rust-lang/rust/blob/master/src/librustc_lint/builtin.rs#L725

kennytm · 2017-07-11T13:05:04Z

src/libsyntax/parse/parser.rs

+                    true
+                } else { false } {
+                    return expr;
+                }


😨

Why not simply

if let Some(ref doc) = ... { self.span_fatal_err(...).emit(); return expr; }

Tired I was.

GuillaumeGomez · 2017-07-15T23:05:28Z

@nikomatsakis: You can start the crater run (if all tests are fine) with deny=unused_doc_comment.

nikomatsakis · 2017-07-16T08:50:57Z

@GuillaumeGomez I can't easily set settings on a crater run, unfortunately --- though making it a lint (usually with deny-by-default) is actually the right way to do these tests (which I had forgotten). Specifically because this way we find out the full impact (otherwise, if any dep has an error, you get an error).

And now that you mention it: this should mean that the compiler keeps working too, because the errors were in our dependencies. Duh.

nikomatsakis · 2017-07-16T08:52:48Z

Errors from travis:

[00:55:54] error[E0560]: struct `ast::Block` has no field named `attrs`

[00:55:54]    --> /checkout/src/libsyntax/parse/mod.rs:907:41

[00:55:54]     |

[00:55:54] 907 |                                         attrs: ThinVec::new(),

[00:55:54]     |                                         ^^^^^^ `ast::Block` does not have this field

[00:55:54]

GuillaumeGomez · 2017-07-16T10:02:32Z

@nikomatsakis: oups, I removed your commits by mistake. :-/ Can you put it back please?

nikomatsakis · 2017-07-17T09:29:12Z

Crater run initiated.

Build compilers old, new
Running tests.
Await quiescence.
Generate report.

nikomatsakis · 2017-07-17T11:31:12Z

Huh, we encountered an error, I guess because one of our primary targets makes use of this feature. Seems to be actually a use in error-chain...

nikomatsakis · 2017-07-17T11:31:28Z

See failure here.

alexcrichton · 2017-07-20T18:31:13Z

I think @nikomatsakis is on vacation now, so changing to

r? @nrc

nrc

Looks good. I had a couple of nits about the phrasing, plus simulacrum's comment could be addressed. Otherwise r+ (iiuc, crater showed an issue, but if this is just a warning lint, that is OK. If I'm wrong, we'll want to fix that first).

nrc · 2017-07-20T22:39:04Z

src/librustc_lint/builtin.rs

@@ -723,6 +723,45 @@ impl EarlyLintPass for IllegalFloatLiteralPattern {
 }

 declare_lint! {
+    pub UNUSED_DOC_COMMENT,
+    Deny,
+    "detects use of useless doc comments"


I would not say "useless", rather that they will be ignored by rustdoc (theoretically, another tool could use them).

nrc · 2017-07-20T22:39:45Z

src/librustc_lint/builtin.rs

+                   I: Iterator<Item=&'a ast::Attribute>,
+                   C: LintContext<'tcx>>(&self, mut attrs: I, cx: &C) {
+        if let Some(attr) = attrs.find(|a| a.is_value_str() && a.check_name("doc")) {
+            cx.struct_span_lint(UNUSED_DOC_COMMENT, attr.span, "unused doc comment").emit();


again, I'd prefer to be explicit that unused means unused by rustdoc

nrc · 2017-07-20T22:40:39Z

src/librustc_lint/builtin.rs

@@ -724,7 +724,7 @@ impl EarlyLintPass for IllegalFloatLiteralPattern {

 declare_lint! {
    pub UNUSED_DOC_COMMENT,
-    Warn,
+    Deny,


Should be Warn now

I'll just remove @nikomatsakis's commits.

GuillaumeGomez · 2017-07-21T20:20:08Z

@nrc: Updated and applied your comments.

nrc · 2017-07-25T05:11:11Z

src/test/compile-fail/useless_comment.rs

+#![deny(unused_doc_comment)]
+
+fn foo() {
+    /// a //~ ERROR unused doc comment


I think this test needs updating now

nrc · 2017-07-25T05:11:22Z

src/test/compile-fail/issue-34222.rs

@@ -13,6 +13,6 @@

 #[rustc_error]
 fn main() { //~ ERROR compilation successful
-    /// crash
+    // crash
    let x = 0;
 }


And this test still needs removing

Update to rc error chain In order to make rust-lang/rust#43009 works, I needed some small updates that are now in error-chain.

GuillaumeGomez · 2017-07-29T14:19:36Z

With all the external issues fixed, let's give it another try!

@bors: r=nrc

bors · 2017-07-29T14:19:36Z

📌 Commit 5636d32 has been approved by nrc

bors · 2017-07-29T14:47:18Z

⌛ Testing commit 5636d32 with merge e2f1b7f...

Throw errors when doc comments are added where they're unused #42617

kennytm · 2017-07-29T17:57:42Z

dist stage2-rls failed. Legit. cargo changed the signature of Packages::from_flags 2 days ago (rust-lang/cargo@33431d7) but rls did not catch up.

[01:19:29] error[E0061]: this function takes 4 parameters but 3 parameters were supplied
[01:19:29]    --> /checkout/src/tools/rls/src/build/cargo.rs:108:37
[01:19:29]     |
[01:19:29] 108 |     let spec = Packages::from_flags(opts.all, &opts.exclude, &opts.package)
[01:19:29]     |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 4 parameters
[01:19:29] 
[01:19:30] error: aborting due to previous error
[01:19:30] 
[01:19:30] error: Could not compile `rls`.

(Travis link, because bors is dead or something: https://travis-ci.org/rust-lang/rust/builds/258881061)

GuillaumeGomez · 2017-07-29T18:33:37Z

And let's retry!

@bors: r=nrc

kennytm · 2017-07-29T18:44:17Z

@GuillaumeGomez Err no, the current rls is still using the old signature. You need to file a PR for rls to fix that first.

GuillaumeGomez · 2017-07-29T18:56:15Z

Ah indeed! Thanks!

@bors: r-

alexcrichton · 2017-07-29T19:37:57Z

@bors: retry r-

GuillaumeGomez · 2017-07-29T20:53:35Z

Ok, so it has been updated to new cargo version. Let's give it another try! :)

@bors: r=nrc

bors · 2017-07-29T20:53:36Z

📌 Commit 3142ca0 has been approved by nrc

bors · 2017-07-29T23:01:50Z

⌛ Testing commit 3142ca0 with merge 53bf790...

Throw errors when doc comments are added where they're unused #42617

bors · 2017-07-30T01:40:03Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 53bf790 to master...

kennytm · 2017-07-31T19:51:30Z

@bors retry p=-2

(trying to get bors running properly)

rust-highfive assigned nikomatsakis Jul 1, 2017

Mark-Simulacrum reviewed Jul 1, 2017

View reviewed changes

GuillaumeGomez force-pushed the unused-doc-comments branch from f7b7ad7 to 3cf53fc Compare July 2, 2017 09:39

GuillaumeGomez force-pushed the unused-doc-comments branch from 6c369c7 to 8134ab4 Compare July 3, 2017 18:14

kennytm reviewed Jul 11, 2017

View reviewed changes

GuillaumeGomez force-pushed the unused-doc-comments branch 2 times, most recently from c16d3fe to 553c0b8 Compare July 15, 2017 23:04

GuillaumeGomez force-pushed the unused-doc-comments branch from 7113883 to f5a11d3 Compare July 16, 2017 10:01

rust-highfive assigned nrc and unassigned nikomatsakis Jul 20, 2017

nrc approved these changes Jul 20, 2017

View reviewed changes

GuillaumeGomez force-pushed the unused-doc-comments branch from 24ed34c to af94f8a Compare July 21, 2017 20:19

nrc reviewed Jul 25, 2017

View reviewed changes

Make a lint instead

1cebf98

GuillaumeGomez force-pushed the unused-doc-comments branch from 25814ce to 1cebf98 Compare July 27, 2017 18:38

Update tests

2f2623b

This was referenced Jul 27, 2017

Fix rustc build rust-lang-deprecated/error-chain#199

Merged

Update to rc error chain rust-lang/cargo#4338

Merged

bors added a commit to rust-lang/cargo that referenced this pull request Jul 28, 2017

Auto merge of #4338 - GuillaumeGomez:lint, r=alexcrichton

305bc25

Update to rc error chain In order to make rust-lang/rust#43009 works, I needed some small updates that are now in error-chain.

Update cargo version

5636d32

GuillaumeGomez force-pushed the unused-doc-comments branch from d9b356d to 5636d32 Compare July 29, 2017 13:11

bors added a commit that referenced this pull request Jul 29, 2017

Auto merge of #43009 - GuillaumeGomez:unused-doc-comments, r=nrc

e2f1b7f

Throw errors when doc comments are added where they're unused #42617

GuillaumeGomez mentioned this pull request Jul 29, 2017

Update to last cargo version rust-lang/rls#429

Merged

Update rls submodule

3142ca0

GuillaumeGomez force-pushed the unused-doc-comments branch from 7a52c78 to 3142ca0 Compare July 29, 2017 20:52

bors added a commit that referenced this pull request Jul 29, 2017

Auto merge of #43009 - GuillaumeGomez:unused-doc-comments, r=nrc

53bf790

Throw errors when doc comments are added where they're unused #42617

bors merged commit 3142ca0 into rust-lang:master Jul 30, 2017

bors mentioned this pull request Jul 30, 2017

Add warnings when rustdoc html rendering differs #41991

Merged

GuillaumeGomez deleted the unused-doc-comments branch July 30, 2017 12:29

euclio mentioned this pull request Nov 28, 2018

Add a pass to mark doc attributes as used #14408

Closed

Throw errors when doc comments are added where they're unused #43009

Throw errors when doc comments are added where they're unused #43009

Uh oh!

Conversation

GuillaumeGomez commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jul 1, 2017

Uh oh!

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jul 2, 2017

Uh oh!

nikomatsakis commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Jul 10, 2017

Uh oh!

GuillaumeGomez commented Jul 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jul 15, 2017

Uh oh!

nikomatsakis commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Jul 16, 2017

Uh oh!

GuillaumeGomez commented Jul 16, 2017

Uh oh!

nikomatsakis commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Jul 17, 2017

Uh oh!

nikomatsakis commented Jul 17, 2017

Uh oh!

alexcrichton commented Jul 20, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jul 29, 2017

Uh oh!

bors commented Jul 29, 2017

Uh oh!

bors commented Jul 29, 2017

Uh oh!

kennytm commented Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Jul 29, 2017

Uh oh!

kennytm commented Jul 29, 2017

Uh oh!

GuillaumeGomez commented Jul 29, 2017

Uh oh!

alexcrichton commented Jul 29, 2017

GuillaumeGomez commented Jul 1, 2017 •

edited

Loading

nikomatsakis commented Jul 7, 2017 •

edited

Loading

nikomatsakis commented Jul 16, 2017 •

edited

Loading

nikomatsakis commented Jul 17, 2017 •

edited

Loading

kennytm commented Jul 29, 2017 •

edited

Loading