Specific error message for missplaced doc comments #33922

estebank · 2016-05-28T04:27:11Z

Identify when documetation comments have been missplaced in the following places:

After a struct element:

// file.rs:
struct X {
    a: u8 /** document a */,
}

$ rustc file.rs
file.rs:2:11: 2:28 error: found documentation comment that doesn't
document anything
file.rs:2     a: u8 /** document a */,
                    ^~~~~~~~~~~~~~~~~
file.rs:2:11: 2:28 help: doc comments must come before what they document,
maybe a comment was intended with `//`?

As the last line of a struct:

// file.rs:
struct X {
    a: u8,
    /// incorrect documentation
}

$ rustc file.rs
file.rs:3:5: 3:27 error: found a documentation comment that doesn't
document anything
file.rs:3     /// incorrect documentation
              ^~~~~~~~~~~~~~~~~~~~~~
file.rs:3:5: 3:27 help: doc comments must come before what they document,
maybe a comment was intended with `//`?

As the last line of a fn:

// file.rs:
fn main() {
    let x = 1;
    /// incorrect documentation
}

$ rustc file.rs
file.rs:3:5: 3:27 error: found a documentation comment that doesn't
document anything
file.rs:3     /// incorrect documentation
              ^~~~~~~~~~~~~~~~~~~~~~
file.rs:3:5: 3:27 help: doc comments must come before what they document,
maybe a comment was intended with `//`?

Fix #27429, #30322

rust-highfive · 2016-05-28T04:27:26Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

GuillaumeGomez · 2016-05-28T10:52:34Z

src/test/parse-fail/doc-before-extern-rbrace.rs

@@ -12,5 +12,6 @@

 extern {
    /// hi
+    //~^ ERROR expected item after doc comment
+    //~^^ HELP May be you wanted a comment


GuillaumeGomez · 2016-05-28T10:54:59Z

Thanks for your PR! You need to update some tests which failed (like src/test/parse-fail/doc-before-extern-rbrace.rs).

estebank · 2016-05-28T14:46:47Z

@GuillaumeGomez I just pushed a new commit with the requested changes. The tests are running ATM.

GuillaumeGomez · 2016-05-28T15:56:13Z

Please squash your commits. Once done and travis says it's ok, I'll r+ it.

estebank · 2016-05-28T19:11:53Z

Please squash your commits. Once done and travis says it's ok, I'll r+ it.

I'm away for the weekend, I'll do it on Monday night.

estebank · 2016-05-30T22:58:21Z

@GuillaumeGomez I have rebased+push. Travis is running now, but I don't see why it'd fail :)

GuillaumeGomez · 2016-05-31T07:17:26Z

Travis is running now, but I don't see why it'd fail :)

Better sure than sorry!

Thanks for this PR. :)

@bors: r+

bors · 2016-05-31T07:17:28Z

📌 Commit ffd9efc has been approved by GuillaumeGomez

bors · 2016-05-31T08:55:45Z

⌛ Testing commit ffd9efc with merge 91dd180...

bors · 2016-05-31T10:07:19Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

alexcrichton · 2016-05-31T16:25:29Z

@bors: retry

Spurious "filename too long" failure on bots #33844

On Tue, May 31, 2016 at 3:07 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
http://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1302

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#33922 (comment), or mute
the thread
/~https://github.com/notifications/unsubscribe/AAD95L2opvz1ZaWB78955QicJ3aoNLpIks5qHAhagaJpZM4Io_dY
.

estebank · 2016-05-31T16:25:47Z

Travis is running now, but I don't see why it'd fail :)

Better sure than sorry!

Of course!

Thanks for this PR. :)

My pleasure.

I see the build failure in win, and I don't see how it was affected by my change. Do you have any ideas on what caused this?

---- sync::atomic::AtomicU8::new_0 stdout ----
    error: could not exec the linker `gcc`: The filename or extension is too long. (os error 206)
thread 'sync::atomic::AtomicU8::new_0' panicked at 'Box<Any>', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\librustc\session/mod.rs:171
note: Run with `RUST_BACKTRACE=1` for a backtrace.
thread 'sync::atomic::AtomicU8::new_0' panicked at 'couldn't compile the test', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\librustdoc\test.rs:285

---- sync::atomic::AtomicUsize::compare_exchange_0 stdout ----
    thread 'sync::atomic::AtomicUsize::compare_exchange_0' panicked at 'couldn't run the test: The filename or extension is too long. (os error 206)', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\librustdoc\test.rs:308

---- sync::atomic::AtomicUsize::compare_and_swap_0 stdout ----
    thread 'sync::atomic::AtomicUsize::compare_and_swap_0' panicked at 'couldn't run the test: The filename or extension is too long. (os error 206)', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\librustdoc\test.rs:308

---- unimplemented!_0 stdout ----
    error: could not exec the linker `gcc`: The filename or extension is too long. (os error 206)
thread 'unimplemented!_0' panicked at 'Box<Any>', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\librustc\session/mod.rs:171
thread 'unimplemented!_0' panicked at 'couldn't compile the test', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\librustdoc\test.rs:285


failures:
    sync::atomic::AtomicU8::new_0
    sync::atomic::AtomicUsize::compare_and_swap_0
    sync::atomic::AtomicUsize::compare_exchange_0
    unimplemented!_0

test result: FAILED. 944 passed; 4 failed; 9 ignored; 0 measured

estebank · 2016-05-31T17:44:24Z

@GuillaumeGomez @alexcrichton I fixed a typo in the commit description and repushed, which I believe will require a new r+, right? It didn't occur to me before I did it :-/

alexcrichton · 2016-05-31T18:01:19Z

No worries!

Taking a look at this I've got a few comments as well, but I'll leave them inline.

alexcrichton · 2016-05-31T18:03:08Z

src/libsyntax/parse/parser.rs

-                                       token_str)))
+                    let err = self.fatal(&format!("expected `{}`, found `{}`", "}",
+                                       token_str));
+                    return Err(err);


This seems like no change happened here, right?

alexcrichton · 2016-05-31T20:04:56Z

src/libsyntax/parse/parser.rs

@@ -3521,7 +3535,7 @@ impl<'a> Parser<'a> {
                if self.token != token::CloseDelim(token::Brace) {
                    let token_str = self.this_token_to_string();
                    return Err(self.fatal(&format!("expected `{}`, found `{}`", "}",
-                                       token_str)))
+                                       token_str)));


This semicolon addition can probably also be reverted

alexcrichton · 2016-05-31T20:05:51Z

Thanks @estebank! I wonder if we can perhaps get more targeted with the error messages here, though? It seems unfortunate that a blanket parse_ident will change errors here rather than just for having doc comments in the most common places. Perhaps just a few specific scenarios could be pattern matched to start off with, and then we could add more over time if necessary?

estebank · 2016-05-31T22:27:14Z

It seems unfortunate that a blanket parse_ident will change errors here rather than just for having doc comments in the most common places. Perhaps just a few specific scenarios could be pattern matched to start off with, and then we could add more over time if necessary?

I'm not sure I follow, @alexcrichton. Are you proposing pushing the logic slightly earlier in the processing of the AST? Identifying doc comments and looking ahead to check wether they are the last element?

alexcrichton · 2016-06-01T02:08:19Z

src/libsyntax/parse/parser.rs

+                                                      self.this_token_to_string()));
+                    if self.token == token::Underscore {
+                        err.note("`_` is a wildcard pattern, not an identifier");
+                    };


Ah this semicolon can be left off

alexcrichton · 2016-06-01T02:09:38Z

Ah yeah sure, I'll elaborate. I think we've definitely got a few particular scenarios in mind of what we'd like to warn about here (e.g. the tests you've written). For example adding this clause to check is_doc_comment makes sense to me as part of parse_single_struct_field because it catches mistakes like:

struct Foo {
    foo: u32 /// dox
}

Another location, when an item isn't found but attributes are, also makes sense to me as we can lint against something like:

mod foo {
    /// dox
}

The other case, however, in parse_ident, seems like it may be a bit too general. For example we'd emit a custom message in cases like:

fn /// foo
foo() {}

Where the previous error message of "expected identifier, found comment" would perhaps make more sense.

This is basically replacing the previous error message so we need to be sure that the error is always more informative than the previous one, or otherwise we're going to have regressions in some cases. Could you elaborate on which tests require the modification to parse_ident?

estebank · 2016-06-01T03:34:23Z

Could you elaborate on which tests require the modification to parse_ident?

parse-fail/issue-27429-2.rs fails if I remove the check in parse_ident:

struct X {
    a: u8,
    /// document
}

---- [parse-fail] parse-fail/issue-27429-2.rs stdout ----
error: /Users/ekuber/personal/rust/src/test/parse-fail/issue-27429-2.rs:14: unexpected "error": '14:5: 14:17: expected identifier, found `/// document`'
error: /Users/ekuber/personal/rust/src/test/parse-fail/issue-27429-2.rs:14: expected error not found: found a documentation comment that doesn't document anything

Added a few new tests (without changing the code so far).

This one goes through the same code path as the one above:

struct X {
    a: u8 /// document
    //~^ ERROR found a documentation comment that doesn't document anything
    //~^^ HELP maybe a comment was intended
}

I feel that the behaviour in the following tests is good acceptable and clear enough:

/// PASSES
fn /// document
foo() {} //~ expected identifier, found `/// document`

mod Foo {
    /// document
    //~^ ERROR expected item after doc comment
}

I'm updating the names of the tests to be descriptive of behaviour and not tied to the issue that prompted this PR.

alexcrichton · 2016-06-02T13:31:29Z

src/libsyntax/parse/parser.rs

-                                     &format!("expected `,`, or `}}`, found `{}`",
-                                             token_str),
-                                     "struct fields should be separated by commas"))
+                if self.is_doc_comment(span) {


Looking through the list of tokens again, we actually have token::DocComment which may be more applicable than adding a new method here

Pushing a new commit that removes is_doc_comment completely.

estebank · 2016-06-03T17:26:01Z

r? @alexcrichton @GuillaumeGomez

GuillaumeGomez · 2016-06-04T14:21:04Z

Good for me. I'll let @alexcrichton handles the r+ if you don't mind. 😉

alexcrichton · 2016-06-05T07:33:20Z

src/libsyntax/parse/parser.rs

-                }
-                Err(err)
+                let last_token = self.last_token.clone().map(|t| *t);
+                Err(match last_token {


Isn't this the same as self.token which is being matched on?

At this point last_token is the actual doc comment, but the failure is raised by the current self.token not being a valid element at this location, that's why we have to look back to check wether the failure is because the previous element was a doc comment or something else. This is also why we set error to self.last_span instead of self.span.

For example, with the following input file.rs:

fn foo(a: u8, b: u64) { /// document }

The type of self.last_token is Some(DocComment(/// document(61))) while self.token is CloseDelim(Brace) and you get the following output with the current code:

% ./rustc file.rs file.rs:2:5: 2:17 error: found a documentation comment that doesn't document anything file.rs:2 /// document ^~~~~~~~~~~~ file.rs:2:5: 2:17 help: doc comments must come before what they document, maybe a comment was intended with `//`?

bors · 2016-06-28T04:48:19Z

☔ The latest upstream changes (presumably #34424) made this pull request unmergeable. Please resolve the merge conflicts.

Identify when documetation comments have been missplaced in the following places: * After a struct element: ```rust // file.rs: struct X { a: u8 /** document a */, } ``` ```bash $ rustc file.rs file.rs:2:11: 2:28 error: found documentation comment that doesn't document anything file.rs:2 a: u8 /** document a */, ^~~~~~~~~~~~~~~~~ file.rs:2:11: 2:28 help: doc comments must come before what they document, maybe a comment was intended with `//`? ``` * As the last line of a struct: ```rust // file.rs: struct X { a: u8, /// incorrect documentation } ``` ```bash $ rustc file.rs file.rs:3:5: 3:27 error: found a documentation comment that doesn't document anything file.rs:3 /// incorrect documentation ^~~~~~~~~~~~~~~~~~~~~~ file.rs:3:5: 3:27 help: doc comments must come before what they document, maybe a comment was intended with `//`? ``` * As the last line of a `fn`: ```rust // file.rs: fn main() { let x = 1; /// incorrect documentation } ``` ```bash $ rustc file.rs file.rs:3:5: 3:27 error: found a documentation comment that doesn't document anything file.rs:3 /// incorrect documentation ^~~~~~~~~~~~~~~~~~~~~~ file.rs:3:5: 3:27 help: doc comments must come before what they document, maybe a comment was intended with `//`? ``` Fix rust-lang#27429, rust-lang#30322

mrhota · 2016-08-18T23:35:55Z

Ping @alexcrichton this seems to be ready for a final check

alexcrichton · 2016-08-19T18:59:01Z

@bors: r+ c8498cc

Oops! Sorry this fell under the radar...

bors · 2016-08-20T01:14:54Z

⌛ Testing commit c8498cc with merge 38fa82a...

Specific error message for missplaced doc comments Identify when documetation comments have been missplaced in the following places: * After a struct element: ```rust // file.rs: struct X { a: u8 /** document a */, } ``` ```bash $ rustc file.rs file.rs:2:11: 2:28 error: found documentation comment that doesn't document anything file.rs:2 a: u8 /** document a */, ^~~~~~~~~~~~~~~~~ file.rs:2:11: 2:28 help: doc comments must come before what they document, maybe a comment was intended with `//`? ``` * As the last line of a struct: ```rust // file.rs: struct X { a: u8, /// incorrect documentation } ``` ```bash $ rustc file.rs file.rs:3:5: 3:27 error: found a documentation comment that doesn't document anything file.rs:3 /// incorrect documentation ^~~~~~~~~~~~~~~~~~~~~~ file.rs:3:5: 3:27 help: doc comments must come before what they document, maybe a comment was intended with `//`? ``` * As the last line of a `fn`: ```rust // file.rs: fn main() { let x = 1; /// incorrect documentation } ``` ```bash $ rustc file.rs file.rs:3:5: 3:27 error: found a documentation comment that doesn't document anything file.rs:3 /// incorrect documentation ^~~~~~~~~~~~~~~~~~~~~~ file.rs:3:5: 3:27 help: doc comments must come before what they document, maybe a comment was intended with `//`? ``` Fix #27429, #30322

bors · 2016-08-20T05:56:28Z

rust-highfive assigned sfackler May 28, 2016

estebank force-pushed the doc-comment branch from 38ecda3 to 7765b1b Compare May 28, 2016 05:08

GuillaumeGomez reviewed May 28, 2016
View reviewed changes

estebank force-pushed the doc-comment branch from 1bc4376 to ffd9efc Compare May 30, 2016 22:57

estebank force-pushed the doc-comment branch from ffd9efc to 0ab33f3 Compare May 31, 2016 16:25

alexcrichton reviewed May 31, 2016
View reviewed changes

estebank force-pushed the doc-comment branch from 0ab33f3 to d3a0490 Compare May 31, 2016 19:07

alexcrichton reviewed May 31, 2016
View reviewed changes

estebank force-pushed the doc-comment branch from 56a340a to 0e3c0aa Compare May 31, 2016 22:51

alexcrichton reviewed Jun 1, 2016
View reviewed changes

estebank force-pushed the doc-comment branch 2 times, most recently from 74ba52c to 92e8799 Compare June 1, 2016 07:16

alexcrichton reviewed Jun 2, 2016
View reviewed changes

estebank force-pushed the doc-comment branch 2 times, most recently from 189da4f to 9e0afa8 Compare June 2, 2016 21:55

rust-highfive assigned alexcrichton and unassigned sfackler Jun 3, 2016

estebank force-pushed the doc-comment branch from 9e0afa8 to 47c1003 Compare June 4, 2016 20:03

alexcrichton reviewed Jun 5, 2016
View reviewed changes

estebank force-pushed the doc-comment branch from 47c1003 to c8498cc Compare July 6, 2016 06:09

bors merged commit c8498cc into rust-lang:master Aug 20, 2016

estebank deleted the doc-comment branch November 9, 2023 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specific error message for missplaced doc comments #33922

Specific error message for missplaced doc comments #33922

estebank commented May 28, 2016 •

edited

Loading

rust-highfive commented May 28, 2016

GuillaumeGomez May 28, 2016

GuillaumeGomez commented May 28, 2016

estebank commented May 28, 2016

GuillaumeGomez commented May 28, 2016

estebank commented May 28, 2016 •

edited

Loading

estebank commented May 30, 2016

GuillaumeGomez commented May 31, 2016

bors commented May 31, 2016

bors commented May 31, 2016

bors commented May 31, 2016

alexcrichton commented May 31, 2016

estebank commented May 31, 2016

estebank commented May 31, 2016

alexcrichton commented May 31, 2016

alexcrichton May 31, 2016

estebank May 31, 2016

alexcrichton May 31, 2016

alexcrichton commented May 31, 2016

estebank commented May 31, 2016

alexcrichton Jun 1, 2016

alexcrichton commented Jun 1, 2016

estebank commented Jun 1, 2016 •

edited

Loading

alexcrichton Jun 2, 2016

estebank Jun 2, 2016

estebank commented Jun 3, 2016

GuillaumeGomez commented Jun 4, 2016

alexcrichton Jun 5, 2016

estebank Jun 5, 2016

bors commented Jun 28, 2016

mrhota commented Aug 18, 2016

alexcrichton commented Aug 19, 2016

bors commented Aug 20, 2016

bors commented Aug 20, 2016

Specific error message for missplaced doc comments #33922

Specific error message for missplaced doc comments #33922

Conversation

estebank commented May 28, 2016 • edited Loading

rust-highfive commented May 28, 2016

Choose a reason for hiding this comment

GuillaumeGomez commented May 28, 2016

estebank commented May 28, 2016

GuillaumeGomez commented May 28, 2016

estebank commented May 28, 2016 • edited Loading

estebank commented May 30, 2016

GuillaumeGomez commented May 31, 2016

bors commented May 31, 2016

bors commented May 31, 2016

bors commented May 31, 2016

alexcrichton commented May 31, 2016

estebank commented May 31, 2016

estebank commented May 31, 2016

alexcrichton commented May 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented May 31, 2016

estebank commented May 31, 2016

Choose a reason for hiding this comment

alexcrichton commented Jun 1, 2016

estebank commented Jun 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jun 3, 2016

GuillaumeGomez commented Jun 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 28, 2016

mrhota commented Aug 18, 2016

alexcrichton commented Aug 19, 2016

bors commented Aug 20, 2016

bors commented Aug 20, 2016

estebank commented May 28, 2016 •

edited

Loading

estebank commented May 28, 2016 •

edited

Loading

estebank commented Jun 1, 2016 •

edited

Loading