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

better error message for trailing doc comment in traits (issue #56766) #57333

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@lcnr
Copy link
Contributor

lcnr commented Jan 4, 2019

closes #56766
r? @estebank

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 4, 2019

⚠️ Warning ⚠️

  • These commits modify submodules.

@lcnr lcnr changed the title for trailing doc comment (issue #56766 for trailing doc comment (issue #56766) Jan 4, 2019

@lcnr lcnr changed the title for trailing doc comment (issue #56766) better error message for trailing doc comment (issue #56766) Jan 4, 2019

@lcnr lcnr changed the title better error message for trailing doc comment (issue #56766) better error message for trailing doc comment in traits (issue #56766) Jan 4, 2019

@estebank
Copy link
Contributor

estebank left a comment

Did you mean to modify src/libbacktrace in your PR? If you didn't, could you rewrite your history to not include it? Also, could you squash your commits?

Other than that it looks good to me.

@lcnr lcnr force-pushed the lcnr:doc-comment branch 2 times, most recently from 164ca62 to 101e91e Jan 8, 2019

@lcnr lcnr force-pushed the lcnr:doc-comment branch from 101e91e to 512b3d0 Jan 8, 2019

@lcnr

This comment has been minimized.

Copy link
Contributor

lcnr commented Jan 8, 2019

@estebank Done. It seems like I somehow automatically created src/libbacktrace as the folder is was completely new. 🤔

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 19, 2019

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

@estebank
Copy link
Contributor

estebank left a comment

@lcnr sorry for the delay in reviewing this. You will need to rebase against master and fix the merge conflict. I have one nitpick, but otherwise, the change looks great! Thank you!

@@ -5940,6 +5940,14 @@ impl<'a> Parser<'a> {
self.expect(&token::OpenDelim(token::Brace))?;
let mut trait_items = vec![];
while !self.eat(&token::CloseDelim(token::Brace)) {
if let token::DocComment(_) = self.token {
if self.look_ahead(1, |tok| tok == &token::Token::CloseDelim(token::Brace)) {
self.span_fatal_err(self.span, Error::UselessDocComment).emit();

This comment has been minimized.

@estebank

estebank Jan 19, 2019

Contributor

It'd be nice if you used struct_span_err(...).emit() instead, and then consume the doc (as you're doing here with self.bump(). That way the parser will "succeed" and continue to type checking in the face of this.

This comment has been minimized.

@lcnr

lcnr Jan 23, 2019

Contributor

I tried doing that, but I was unable to find a way to emit an Error without using span_fatal_err. All other methods of Parsercurrently use a &str instead. I might look deeper into the diagnostics system once I have the time. Might take a while though 😢

This comment has been minimized.

@estebank

estebank Jan 23, 2019

Contributor

I think there's struct_span_err_with_code which will make you duplicate some of the work. That being said 1) a very small amount of duplication is potentially ok and 2) it might be very easy to unify all uses in a single method.

Also, take your time. We're here to help if you need us.

}

fn main() {

This comment has been minimized.

@estebank

estebank Jan 19, 2019

Contributor

Add a let _: usize =""; line to make sure the compiler recovers from the incorrect doc comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment