Account for missing keyword in fn/struct definition #45997

estebank · 2017-11-15T01:50:26Z

estebank · 2017-11-15T02:01:18Z

nikomatsakis

Actually, looking more closely, I think this is overkill. I'll leave a more detailed comment with an alternative shortly.

nikomatsakis · 2017-11-15T17:18:37Z

Currently, after failing to parse, the parser invokes this commonly used method:

rust/src/libsyntax/parse/parser.rs

Lines 6267 to 6275 in ce2b8a4

    
           /// This is the fall-through for parsing items. 
        
           fn parse_macro_use_or_failure( 
        
               &mut self, 
        
               attrs: Vec<Attribute> , 
        
               macros_allowed: bool, 
        
               attributes_allowed: bool, 
        
               lo: Span, 
        
               visibility: Visibility 
        
           ) -> PResult<'a, Option<P<Item>>> {

This is intended to look for a macro use like path! -- but right now it decides it has seen a macro quite early, based only on whether the next token can start a path. Presuming it can, we error if we saw a pub, parse the path, and then expect a !. This is where the error will occur in our bad examples.

I think we should customize what happens when we "expect" this ! but do not find it:

To start, we can look at the path. If it was multipart, like a::b, then I think this could only be a macro-use, perhaps one where the ! was forgotten, and we could diagnose it that way. (Or, at least, we could do this if the next token is (.)

If the path is a single identifier, we can then examine the next character: if it is (, we could suggest a function. If it is {, we could suggest a struct. Or, we could just issue something generic like "expected an item like a fn or a macro use like foo!".

estebank · 2017-11-15T21:28:40Z

It is reasonable to go ahead and do that change, but I also feel there's value in modifying the current PR to keep doing what it is doing and returning Ok with the fn or struct that could be parsed, instead of the Err. That way parsing can continue and present other errors down the line, instead of the current behavior of having a failure parsing a macro and then a failure parsing the opening brace or paren. What do you think?

nikomatsakis · 2017-11-16T19:32:20Z

@estebank that's a good point. On the other hand, I wonder, could we get a similar effect by just skipping over entire token trees?

I feel like only being able to recover when the input is perfect is perhaps not ideal.

nikomatsakis · 2017-11-16T19:33:01Z

though man but mismatched braces are the worst sort of parse errors =)

nikomatsakis · 2017-11-16T19:34:16Z

In any case, I could go either way. But I'd like to see us able to recover even from something like this:

pub foo(x: u32) {
    x->foo(); // whoops, too much C++
}

petrochenkov · 2017-11-18T23:09:21Z

#41282 already implemented this recovery for trait and impl items, it can be adapted to normal items as well.
Saving/restoring parser state should be avoided if possible.

estebank · 2017-11-22T17:15:53Z

I have modified the code to not perform the rewind. The outwards behavior now is similar to what it already was (the parser will not let the compiler to continue, but it will try to consume the continuing code in order to identify what it was meant (fn or struct), regardless of code errors (just looks for enclosed (...) and {...} sections).

nikomatsakis

This all looks quite cool. I do have one concern about the recovery condition I'd like to get resolved though.

nikomatsakis · 2017-11-22T21:11:47Z

src/test/ui/suggestions/pub-ident-fn-2.stderr

+   |
+help: add `fn` here to parse `foo` as a public method
+   |
+11 | pub fn foo(s: usize) { bar() }


that is pretty slick =)

nikomatsakis · 2017-11-22T21:12:50Z

src/libsyntax/parse/parser.rs

+        // Verify wether we have encountered a struct or method definition where the user forgot to
+        // add the `struct` or `fn` keyword after writing `pub`: `pub S {}`
+        if visibility == Visibility::Public &&
+            self.check_ident() &&


Isn't it possible these days to have paths to macros? e.g., foo::bar!?

cc @jseyfried @petrochenkov

The way the fall back happens, if the code isn't either pub foo ( or pub foo {, the current macro handling will happen. In the case of pub foo::bar this new branch will not be executed. As long as parse_macro_use_or_failure does the right thing with paths, this should be fine :)

When encountering `pub ident`, attempt to identify the code that comes afterwards, wether it is a brace block (assume it is a struct), a paren list followed by a colon (assume struct) or a paren list followed by a block (assume a fn). Consume those blocks to avoid any further parser errors and return a `Placeholder` item in order to allow the parser to continue. In the case of unenclosed blocks, the behavior is the same as it is currently: no further errors are processed.

Try to identify the following code in order to provide better diagnostics, but return the error to bail out early during the parse.

…ields

estebank · 2017-11-25T14:22:22Z

@bors try

bors · 2017-11-25T14:22:48Z

⌛ Trying commit f103342 with merge 1026513cc276e201297365f43014ee01b2df9b06...

estebank · 2017-11-25T16:08:39Z

@nikomatsakis this PR is ballooning in size with multiple independent changes that are kind or related but not entirely. There's a very small one liner suggestion underline fix, emitting inappropriate DocComment location error but allow it for the parser to continue parsing the struct, making block parsing errors recoverable, other than the original intention of the PR which was to add a suggestion if the parser encounters pub ident ( or pub ident {.

Because of these changes, in the case of compile-fail-fulldeps/proc-macro/derive-bad.rs (which uses compile-fail-fulldeps/proc-macro/auxiliary/derive-bad.rs) the parsing errors from the derive are emitted and not seen by libproc_macro as a failure, so we get the internal parsing errors, that somebody who can't modify the proc macro can't do anything about, and we don't cause the actual expected message. For the second issue I'll try to find a way to emit it, for the first, is it ok to leak these messages?

As for the first paragraph, should I try to split this in a bunch of different dependent PRs?

Edit: I have fixed the issue for all proc macros now: made it check the error count before and after and emit the appropriate diagnostic if it has increased. This should be resilient to further changes in the parser, although it will leak parsing errors from the proc macro to the user. I feel that this is not a bad thing, even if it was an unintended effect originally.

estebank · 2017-11-29T18:17:12Z

@nikomatsakis let me know if the output changes are reasonable or if I should keep working on this PR to get to minimal changes from the status quo.

nikomatsakis · 2017-11-30T20:15:28Z

@bors r+

@estebank seems good to me.

bors · 2017-11-30T20:15:29Z

📌 Commit cf9283e has been approved by nikomatsakis

bors · 2017-12-01T06:06:13Z

⌛ Testing commit cf9283e with merge d1364a6...

Account for missing keyword in fn/struct definition Fix #38911.

bors · 2017-12-01T08:44:36Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing d1364a6 to master...

…henkov Suggest an appropriate token when encountering `pub Ident<'a>` Fix rust-lang#55403. Follow up to rust-lang#45997.

Xiretza · 2022-10-07T05:37:11Z

src/libsyntax/parse/parser.rs

+                    self.check(&token::OpenDelim(token::Brace))
+                {
+                    ("fn", "method", false)
+                } else if self.check(&token::Colon) {


@estebank sorry for the enormous necropost, but do you know what the idea behind this was? It matches pub foo(stuff): and suggests adding struct, but where does that colon come from? Is it possible this was intended to be token::Semi instead? The behaviour isn't covered by any tests, unfortunately.

estebank changed the title ~~Pub ident~~ Account for missing keyword in fn/struct definition Nov 15, 2017

rust-highfive assigned nikomatsakis Nov 15, 2017

estebank force-pushed the pub-ident branch from 7cd1213 to 5f976e3 Compare November 15, 2017 02:30

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2017

petrochenkov self-assigned this Nov 15, 2017

nikomatsakis approved these changes Nov 15, 2017

View reviewed changes

nikomatsakis requested changes Nov 15, 2017

View reviewed changes

petrochenkov 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 Nov 18, 2017

kennytm 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 Nov 22, 2017

nikomatsakis reviewed Nov 22, 2017

View reviewed changes

estebank force-pushed the pub-ident branch 3 times, most recently from 610cf72 to 35fbdd4 Compare November 23, 2017 15:31

estebank added 7 commits November 24, 2017 07:34

Fix underline in suggestions

1737d69

Account for missing keyword in fn/struct definition

547873a

Do not attemt to continue parsing after pub ident

c82e9e8

Try to identify the following code in order to provide better diagnostics, but return the error to bail out early during the parse.

Suggest macro call when not sure that it is fn definition

df357b2

Emit DocComment in bad location error but continue parsing struct f…

4e2d1b9

…ields

Revert to correct recovery behavior

0e93b75

estebank force-pushed the pub-ident branch from 287e078 to e0870af Compare November 24, 2017 16:21

Consume trailing doc comments to avoid parse errors

f103342

estebank force-pushed the pub-ident branch from e0870af to f103342 Compare November 24, 2017 16:32

Add inline compile-fail markers to tests

0e241d0

Fix proc_macro output with struct parse error

cf9283e

estebank force-pushed the pub-ident branch from 9eeab2d to cf9283e Compare November 25, 2017 18:15

petrochenkov removed their assignment Nov 26, 2017

nikomatsakis approved these changes Nov 30, 2017

View reviewed changes

bors added a commit that referenced this pull request Dec 1, 2017

Auto merge of #45997 - estebank:pub-ident, r=nikomatsakis

d1364a6

Account for missing keyword in fn/struct definition Fix #38911.

bors merged commit cf9283e into rust-lang:master Dec 1, 2017

This was referenced Dec 1, 2017

WIP - return-type Lifetime elision #46245

Closed

trait alias infrastructure #45047

Merged

estebank mentioned this pull request Dec 12, 2017

rustc is too eager to see macros where there are not #41273

Closed

estebank mentioned this pull request Nov 26, 2018

Suggest an appropriate token when encountering pub Ident<'a> #56248

Merged

Centril added a commit to Centril/rust that referenced this pull request Dec 8, 2018

Rollup merge of rust-lang#56248 - estebank:suggest-bare-pub, r=petroc…

ac15b4f

…henkov Suggest an appropriate token when encountering `pub Ident<'a>` Fix rust-lang#55403. Follow up to rust-lang#45997.

Xiretza reviewed Oct 7, 2022

View reviewed changes

estebank deleted the pub-ident branch November 9, 2023 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for missing keyword in fn/struct definition #45997

Account for missing keyword in fn/struct definition #45997

estebank commented Nov 15, 2017

estebank commented Nov 15, 2017

nikomatsakis left a comment

nikomatsakis commented Nov 15, 2017

estebank commented Nov 15, 2017

nikomatsakis commented Nov 16, 2017

nikomatsakis commented Nov 16, 2017

nikomatsakis commented Nov 16, 2017

petrochenkov commented Nov 18, 2017

estebank commented Nov 22, 2017

nikomatsakis left a comment

nikomatsakis Nov 22, 2017

nikomatsakis Nov 22, 2017

estebank Nov 22, 2017

estebank commented Nov 25, 2017

bors commented Nov 25, 2017

estebank commented Nov 25, 2017 •

edited

Loading

estebank commented Nov 29, 2017

nikomatsakis commented Nov 30, 2017

bors commented Nov 30, 2017

bors commented Dec 1, 2017

bors commented Dec 1, 2017

Xiretza Oct 7, 2022

Account for missing keyword in fn/struct definition #45997

Account for missing keyword in fn/struct definition #45997

Conversation

estebank commented Nov 15, 2017

estebank commented Nov 15, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 15, 2017

estebank commented Nov 15, 2017

nikomatsakis commented Nov 16, 2017

nikomatsakis commented Nov 16, 2017

nikomatsakis commented Nov 16, 2017

petrochenkov commented Nov 18, 2017

estebank commented Nov 22, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Nov 22, 2017

Choose a reason for hiding this comment

nikomatsakis Nov 22, 2017

Choose a reason for hiding this comment

estebank Nov 22, 2017

Choose a reason for hiding this comment

estebank commented Nov 25, 2017

bors commented Nov 25, 2017

estebank commented Nov 25, 2017 • edited Loading

estebank commented Nov 29, 2017

nikomatsakis commented Nov 30, 2017

bors commented Nov 30, 2017

bors commented Dec 1, 2017

bors commented Dec 1, 2017

Xiretza Oct 7, 2022

Choose a reason for hiding this comment

estebank commented Nov 25, 2017 •

edited

Loading