Doc comments present after a particular syntax error cause an unhelpful error message to be output. #48946

PramodBisht · 2018-03-12T01:40:37Z

rust-highfive · 2018-03-12T01:40:43Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (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.

estebank

Sorry for the delay in reviewing. Could you add a test for this case?

Also, could you rebase against master? We shouldn't submit PRs with merge commits in them.

estebank · 2018-03-16T20:53:35Z

src/libsyntax/parse/parser.rs

+                                    &format!("expected `,`, or `}}`, found `{}`", current_token),
+                                    "struct fields should be separated by commas");
+                    err.emit();
+                    return Err(comma_err);
                }
            }


Could you add a test in src/test/ui for this case?

struct S { x: u8 /// The id of the parent core y: u8, }

estebank · 2018-03-16T20:57:42Z

src/libsyntax/parse/parser.rs

+                                    &format!("expected `,`, or `}}`, found `{}`", current_token),
+                                    "struct fields should be separated by commas");
+                    err.emit();
+                    return Err(comma_err);


I'm not convinced that this code will perform the expected behavior. current_token would, if I'm reading this correctly, be the doc comment.

I think that what you'd want to do is keep a mut seen_comma: bool that gets set to true either if self.token is Comma or if the comma is eaten after consuming the doc comment. If that happens, then you perform the logic in the else branch above. Makes sense?

@estebank : If we are inside token::DocComment(_) block instead of token::Comma then it means current token is not comma but doc, from here we can imply that comma is missing between doc comment and struct field and if next token is not comma then we are going inside else block, this is where, we are emitting comma missing message. This is my current approach, I will go with your approach because that looks much cleaner version :)

Now problem is my testcase is failing because of unknown reasons. Problem is test case will fail even if we write testcase for default block of switch case. Any tips how I can fix my testcase?

Let me know if I am getting all this wrong :)

The test is failing due to missing whitespace in the missing main error:

[00:43:14] [00:43:14] ---- [ui] ui/issue-48636.rs stdout ---- [00:43:14] diff of stderr: [00:43:14] [00:43:14] 15 = help: struct fields should be separated by commas [00:43:14] 16 [00:43:14] 17 error[E0601]: `main` function not found in crate `issue_48636` [00:43:14] - | [00:43:14] - = note: consider adding a `main` function to `$DIR/issue-48636.rs` [00:43:14] + | [00:43:14] + = note: consider adding a `main` function to `$DIR/issue-48636.rs` [00:43:14] 20 [00:43:14] 21 error: aborting due to 3 previous errors [00:43:14] 22

Add an empty fn main() {} in the file so that we reduce the amount of text in the stderr file only to the relevant diagnostic. Also, you'll have to annotate the rs test file with a comment in line 16 as follows:

//~^^^ ERROR found a documentation comment that doesn't document anything

Each ^ makes the check happen one line up, and then it just matches the text against the diagnostics emitted.

shepmaster · 2018-03-23T15:23:37Z

Ping from triage, @PramodBisht ! Looks like your new test is causing a build failure:

[00:43:14] {"message":"found a documentation comment that doesn't document anything","code":{"code":"E0585","explanation":"\nA documentation comment that doesn't document anything was found.\n\nErroneous code example:\n\n```compile_fail,E0585\nfn main() {\n    // The following doc comment will fail:\n    /// This is a useless doc comment!\n}\n```\n\nDocumentation comments need to be followed by items, including functions,\ntypes, modules, etc. Examples:\n\n```\n/// I'm documenting the following struct:\nstruct Foo;\n\n/// I'm documenting the following function:\nfn foo() {}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/issue-48636.rs","byte_start":492,"byte_end":521,"line_start":13,"line_end":13,"column_start":5,"column_end":34,"is_primary":true,"text":[{"text":"    /// The id of the parent core","highlight_start":5,"highlight_end":34}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"doc comments must come before what they document, maybe a comment was intended with `//`?","code":null,"level":"help","spans":[],"children":[],"rendered":null}],"rendered":"error[E0585]: found a documentation comment that doesn't document anything\n  --> /checkout/src/test/ui/issue-48636.rs:13:5\n   |\nLL |     /// The id of the parent core\n   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n   |\n   = help: doc comments must come before what they document, maybe a comment was intended with `//`?\n\n"}
[00:43:14] {"message":"expected `,`, or `}`, found `/// The id of the parent core`","code":null,"level":"error","spans":[{"file_name":"/checkout/src/test/ui/issue-48636.rs","byte_start":492,"byte_end":521,"line_start":13,"line_end":13,"column_start":5,"column_end":34,"is_primary":true,"text":[{"text":"    /// The id of the parent core","highlight_start":5,"highlight_end":34}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"struct fields should be separated by commas","code":null,"level":"help","spans":[],"children":[],"rendered":null}],"rendered":"error: expected `,`, or `}`, found `/// The id of the parent core`\n  --> /checkout/src/test/ui/issue-48636.rs:13:5\n   |\nLL |     /// The id of the parent core\n   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n   |\n   = help: struct fields should be separated by commas\n\n"}
[00:43:14] {"message":"`main` function not found in crate `issue_48636`","code":{"code":"E0601","explanation":"\nNo `main` function was found in a binary crate. To fix this error, add a\n`main` function. For example:\n\n```\nfn main() {\n    // Your program will start here.\n    println!(\"Hello world!\");\n}\n```\n\nIf you don't know the basics of Rust, you can go look to the Rust Book to get\nstarted: https://doc.rust-lang.org/book/\n"},"level":"error","spans":[],"children":[{"message":"consider adding a `main` function to `/checkout/src/test/ui/issue-48636.rs`","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0601]: `main` function not found in crate `issue_48636`\n   |\n   = note: consider adding a `main` function to `/checkout/src/test/ui/issue-48636.rs`\n\n"}
[00:43:14] {"message":"aborting due to 3 previous errors","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 3 previous errors\n\n"}
[00:43:14] {"message":"Some errors occurred: E0585, E0601.","code":null,"level":"","spans":[],"children":[],"rendered":"Some errors occurred: E0585, E0601.\n"}
[00:43:14] {"message":"For more information about an error, try `rustc --explain E0585`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about an error, try `rustc --explain E0585`.\n"}

PramodBisht · 2018-03-30T01:48:13Z

@shepmaster will check that today or tomorrow, I could not pick this up because of bandwidth issue. Sorry for the delay.

shepmaster · 2018-04-07T19:28:27Z

Weekly ping from triage, @PramodBisht ! Are you still actively working on this issue?

PramodBisht · 2018-04-12T17:24:16Z

@shepmaster sorry for the delay, I will work on this during a weekend.

estebank · 2018-04-19T17:10:52Z

src/libsyntax/parse/parser.rs

                self.bump(); // consume the doc comment
                if self.eat(&token::Comma) || self.token == token::CloseDelim(token::Brace) {
                    err.emit();
                } else {
-                    return Err(err);
+                    let mut comma_err = self.span_fatal_help(current_span,
+                                    &format!("expected `,`, or `}}`, found `{}`", current_token),


current_token could be a really long doc string, use instead the token description.

estebank · 2018-04-19T17:13:14Z

src/test/ui/issue-48636.stderr

+LL |     /// The id of the parent core
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: struct fields should be separated by commas


I feel that in this case the desired full output would be:

error[E0585]: found a documentation comment that doesn't document anything --> $DIR/issue-48636.rs:13:5 | LL | x: u8 | - help: missing comma here: `,` LL | /// The id of the parent core | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: doc comments must come before what they document, maybe a comment was intended with `//`?

using a span_suggestion.

PramodBisht · 2018-04-21T17:04:33Z

@estebank could you review again?

estebank

Looking great! Just one last change needed.

estebank · 2018-04-22T00:05:07Z

src/libsyntax/parse/parser.rs

                    err.emit();
                } else {
+                    if seen_comma== false {
+                        err.span_suggestion(previous_span, "missing comma here: `,`", "".into());


this should be

err.span_suggestion(sp, "missing comma here", ",".into());

The output will be the same, but now third party tools, like VSCode can apply the suggestion.

sp should be the result of calling codemap.next_point(previous_span), otherwise it is pointing at the type, instead of pointing after the type, where the comma is supposed to be.

estebank · 2018-04-22T00:05:32Z

src/libsyntax/parse/parser.rs

+                let comma_after_doc_seen = self.eat(&token::Comma);
+                // `seen_comma` is always false, because we are inside doc block
+                // condition is here to make code more readable
+                if seen_comma == false && comma_after_doc_seen == true{


missing space before the brace

estebank · 2018-04-22T00:05:58Z

src/libsyntax/parse/parser.rs

                    err.emit();
                } else {
+                    if seen_comma== false {


missing space before the ==.

estebank · 2018-04-22T00:12:50Z

src/test/ui/issue-48636.stderr

+  --> $DIR/issue-48636.rs:13:5
+   |
+LL |     x: u8
+   |        -- help: missing comma here: `,`


As pointed out above, this suggestion should be pointing at the span after u8.

2) Changed position of help message, incase comma is missing 3) added few missing spaces and handled span_suggestion for vscode 4) updated stderr file

PramodBisht · 2018-04-22T07:51:41Z

@estebank could you please check again?

estebank · 2018-04-23T01:36:46Z

@bors r+ rollup

bors · 2018-04-23T01:36:47Z

📌 Commit 1bed654 has been approved by estebank

bors · 2018-04-23T01:36:58Z

⌛ Testing commit 1bed654 with merge 2c57826...

@estebank

Doc comments present after a particular syntax error cause an unhelpful error message to be output. fixed: #48636 r? @estebank

bors · 2018-04-23T04:23:38Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 2c57826 to master...

rust-highfive assigned estebank Mar 12, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 12, 2018

estebank requested changes Mar 16, 2018

View reviewed changes

estebank 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 16, 2018

PramodBisht force-pushed the issues/48636 branch 3 times, most recently from 074cfe0 to d9a9ead Compare March 18, 2018 08:11

pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 16, 2018

estebank reviewed Apr 19, 2018

View reviewed changes

PramodBisht force-pushed the issues/48636 branch 2 times, most recently from ac68aa1 to f72a756 Compare April 21, 2018 15:19

estebank requested changes Apr 22, 2018

View reviewed changes

1) Addresses rust-lang#48636

1bed654

2) Changed position of help message, incase comma is missing 3) added few missing spaces and handled span_suggestion for vscode 4) updated stderr file

PramodBisht force-pushed the issues/48636 branch from acdf2ce to 1bed654 Compare April 22, 2018 06:14

PramodBisht 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 Apr 22, 2018

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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2018

bors added a commit that referenced this pull request Apr 23, 2018

Auto merge of #48946 - PramodBisht:issues/48636, r=estebank

2c57826

Doc comments present after a particular syntax error cause an unhelpful error message to be output. fixed: #48636 r? @estebank

bors merged commit 1bed654 into rust-lang:master Apr 23, 2018

PramodBisht deleted the issues/48636 branch April 23, 2018 05:22

Doc comments present after a particular syntax error cause an unhelpful error message to be output. #48946

Doc comments present after a particular syntax error cause an unhelpful error message to be output. #48946

Uh oh!

Conversation

PramodBisht commented Mar 12, 2018

Uh oh!

rust-highfive commented Mar 12, 2018

Uh oh!

estebank left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

shepmaster commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PramodBisht commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shepmaster commented Apr 7, 2018

Uh oh!

PramodBisht commented Apr 12, 2018

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!

PramodBisht commented Apr 21, 2018

Uh oh!

estebank 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!

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!

PramodBisht commented Apr 22, 2018

Uh oh!

estebank commented Apr 23, 2018

Uh oh!

bors commented Apr 23, 2018

Uh oh!

bors commented Apr 23, 2018

Uh oh!

bors commented Apr 23, 2018

Uh oh!

Uh oh!

estebank left a comment •

edited

Loading

shepmaster commented Mar 23, 2018 •

edited

Loading

PramodBisht commented Mar 30, 2018 •

edited

Loading