Trying to show reasonable errors for trailing doc comments...

[wafflespeanut]

fixes #2789

[wafflespeanut]

r? @Manishearth

[rust-highfive]

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

[wafflespeanut]

This is where the whole thing's going on. We could either bump the parser, so that it neglects this "event" (which I've done now), or we could span an error here.

[Manishearth]

r? @eddyb

or some other parser person

[wafflespeanut]

I really hoped that I could fix this elsewhere, but while parsing the trait, we're filling the parser's expected_tokens vector with the things we expect (const, fn, etc.), and so it finally throws the error before the bump (before even consuming the semicolon, I guess). So, we check for the trailing case explicitly and bump again if needed (which is issue-specific and perhaps, not the clean way).

I also guess that the real issue is behind real_tokens, where the DocComments aren't checked. I'm not sure about the reason, but even if we include the case there, we can't emit an error for these cases.

[Manishearth]

why doesn't this throw an error?

[wafflespeanut]

ah, whoops, it should. The goal I had at first was to not throw an error :)

[Manishearth]

I'll have a closer look at the PR later, but you need review from someone more familiar with the parser than I 😄

[nagisa]

You likely want some sort of loop, because lexer may emit multiple DocComment in a row.

That being said… I do not think lookahead is the correct way or this is the place to do this kind of thing. Lets see what other people have to say about this.

[nrc]

Why don't you think it is the best way to do it?

[nagisa]

While look ahead would be a viable approach if you were guaranteed to only get a single DocComment token, it doesn’t work here, because you effectively have to do infinite look-ahead now. This also is a very uncommonly used strategy in our parser: we just parse ahead and figure out what’s wrong when we hit a token we do not expect. I believe it should be possible to do the same in this situation as well.

[Manishearth]

Anyone want to review? @nrc @eddyb

[nrc]

This seems to only address the issue for trait items, not for top-level items or impl items (or I guess doc comments in random positions, like inside a struct).

[nrc]

Seems like a better approach (as you kind of suggest earlier) is for real_token to skip doc comments unless they're specifically asked for, but issue a warning or error if one is encountered unexpectedly.

[wafflespeanut]

So, I'll summarize the problem I'm experiencing right now.

1. struct: This is okay, because it was simple enough to add another match arm to check whether we're parsing a DocComment

2. enum: Currently, we're looping over the tokens until we encounter a closing brace. So, even if we skip on the first doc comment, there's no way for us to know whether the next one's an expected token or not, because there could be a series of doc comments, just like @nagisa said.

3. trait: While parsing the traits, we use a closure that consumes the final closing brace (which is sent to a looping function), which means we have the same problem of not knowing what's in the future.

4. impl: Just like the last two, we loop until we get a closing brace, same problem.

Even if we add a flag to the parser to ignore the doc comments all the time (except those areas checked by the lint), the problem remains unchanged (except that it will be a "when to enable the doc comments check" instead of "when to ignore them"). Any ideas people?

[bors]

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

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase!