EXPERIMENT: Recover on stmts/exprs at module level, suggesting to wrap in function #69445
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
Centril
commented
Feb 24, 2020
| = note: for more on functions and how to structure your program, see https://doc.rust-lang.org/book/ch03-03-how-functions-work.html | ||
| help: consider moving the statements into a function | ||
| | | ||
| LL | fn my_function() { x.y(); let x = 0; x; } |
There was a problem hiding this comment.
Not sure why this is saying my_function as opposed to main.
Comment on lines
+351
to
+364
| // We're in statement-as-module-item recovery mode. | ||
| // To avoid "stealing" syntax from e.g. `x.f()` as a module-level statement, | ||
| // we backtrack if we failed to parse `$path!`; after we have, we commit firmly. | ||
| // This is only done when `mod_stmt` holds to avoid backtracking inside functions. | ||
| let snapshot = self.clone(); | ||
| match parse_prefix(self) { | ||
| Ok(path) => path, | ||
| Err(mut err) => { | ||
| // Assert that this is only for diagnostics! | ||
| // This is a safeguard against breaking LL(k) accidentally in the spec, | ||
| // assuming no one has gated the syntax with something like `#[cfg(FALSE)]`. | ||
| err.delay_as_bug(); | ||
| *self = snapshot; | ||
| return Ok(None); |
There was a problem hiding this comment.
This use of backtracking bears some discussion. It only happens outside of function bodies to avoid regressing perf or accepting more syntax. In the case of module level, if we hit EOF we will not even attempt parsing an item so it shouldn't have a perf implication that way?
There was a problem hiding this comment.
An alternative to this might be bounded look-ahead for some fixed k, but that is ostensibly less good for diagnostics and trickier to encode correctly. Path parsing is also rarely long and $path! is a fairly unique sequence of tokens.
There was a problem hiding this comment.
I would add another closure or fn can_begin_macro that checks one token for is_ident and two tokens for either :: or !, and only clone if neither are true. That would keep the cost of cloning down for the happy path at the cost of marginally worse diagnostics.
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
bors
added a commit
that referenced
this pull request
Feb 24, 2020
EXPERIMENT: Recover on stmts/exprs at module level, suggesting to wrap in function TODO: write description r? @petrochenkov @estebank
estebank
reviewed
Feb 25, 2020
Comment on lines
+1
to
+12
| error: statements cannot reside in modules | ||
| --> $DIR/issue-49040.rs:1:28 | ||
| | | ||
| LL | #![allow(unused_variables)]; | ||
| | ^ help: remove this semicolon | ||
| | ^ | ||
| | | ||
| = note: the program entry point starts in `fn main() { ... }`, defined in `main.rs` | ||
| = note: for more on functions and how to structure your program, see https://doc.rust-lang.org/book/ch03-03-how-functions-work.html | ||
| help: consider moving the statements into a function | ||
| | | ||
| LL | #![allow(unused_variables)]fn my_function() { } | ||
| | ^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
This is enough of a regression that it should be accounted for, IMO.
There was a problem hiding this comment.
Would suggest #69445 (comment) to fix it.
estebank
reviewed
Feb 25, 2020
| --> $DIR/pub-restricted-error-fn.rs:1:12 | ||
| | | ||
| LL | pub(crate) () fn foo() {} | ||
| | ^ expected item | ||
| | ^^ |
There was a problem hiding this comment.
This seems less than ideal. I think when we're midway some item head, we should not emit the suggestion.
There was a problem hiding this comment.
But is this a common case? And if it is, does the first error not suggest what the problem is and how to fix it? And is it not better to recover than to fatally error? Otherwise we could have some flag when calling parse_item_common_ which signifies that we've parsed at least the visibility / defaultness, if you would prefer the error as-is, though that is adding code complexity, so consider whether this is common enough to be worth it.
There was a problem hiding this comment.
I read your comment again; it makes more sense now since you're talking only about the suggestion -- I think we could use the same flag and pass it in to avoid everything except for the primary message, and instead do "remove the statements".
Comment on lines
+221
to
226
| } else { | ||
| None | ||
| } { | ||
| // MACRO INVOCATION ITEM | ||
| (Ident::invalid(), ItemKind::Mac(self.parse_item_macro(vis)?)) | ||
| (Ident::invalid(), ItemKind::Mac(kind)) | ||
| } else { |
There was a problem hiding this comment.
This doesn't seem right (} else { None } { (...) } else {)
There was a problem hiding this comment.
I would have preferred to write this with let-chains but they ain't implemented yet sadly.
Centril
commented
Feb 25, 2020
| // MACRO INVOCATION ITEM | ||
| (Ident::invalid(), ItemKind::Mac(self.parse_item_macro(vis)?)) | ||
| (Ident::invalid(), ItemKind::Mac(kind)) | ||
| } else { |
There was a problem hiding this comment.
@estebank we could do something like:
Suggested change
| } else { | |
| } else if self.maybe_consume_incorrect_semicolon(&[]) { | |
| let lo = self.token.span; | |
| self.parse_item_kind(attrs, macros_allowed, lo, vis, def, req_name, mod_stmt)? | |
| } else { |
estebank
reviewed
Feb 25, 2020
| let span = lo.to(self.prev_span); | ||
| let spans: MultiSpan = match &*stmts { | ||
| [] | [_] => span.into(), | ||
| [x, .., y] => vec![x.span, y.span].into(), |
There was a problem hiding this comment.
I'm guessing that you didn't like the multiline span output for these?
There was a problem hiding this comment.
Not sure what you mean; in the first arm the span spans as long as [first.span] would and in the case of [] it's unreachable (but I didn't unreachable!() because it's more code).
There was a problem hiding this comment.
I mean that the alternative was a single x.span.until(y.span).
There was a problem hiding this comment.
Why do we want .until(...) though? That would leave out e.g. the end of the last statement? I would expect x.to(y) as the alternative, but that's equivalent to span.
There was a problem hiding this comment.
Switching to just span in all cases then. :)
estebank
reviewed
Feb 25, 2020
| visitor.visit_stmt(stmt); | ||
| } | ||
| if let StmtKind::Expr(_) = &stmts.last().unwrap().kind { | ||
| visitor.1 = true; // The tail expression. |
There was a problem hiding this comment.
The tail expression will be somewhat arbitrary in real code. Don't know if this part is worth it or it will be noisier than we'd wish.
There was a problem hiding this comment.
Up to a point, after we've shown the user how to write a function, if they forget to add a return type we'll guide them in the right direction in subsequent errors.
There was a problem hiding this comment.
My thinking is that worst case you get -> _ and you write that, and the compiler tells you to write -> () cause it's inferring the return type.
estebank
reviewed
Feb 25, 2020
src/librustc_parse/parser/item.rs
Outdated
Comment on lines
680
to
682
| fn parse_assoc_item(&mut self, req_name: ReqName) -> PResult<'a, Option<Option<P<AssocItem>>>> { | ||
| Ok(self.parse_item_(req_name)?.map(|Item { attrs, id, span, vis, ident, kind, tokens }| { | ||
| let item = self.parse_item_(req_name)?; | ||
| Ok(item.map(|Item { attrs, id, span, vis, ident, kind, tokens }| { |
There was a problem hiding this comment.
This should handle the missing closing brace case somehow.
There was a problem hiding this comment.
Isn't that handled in parse_item_list? Not sure what the relation to this PR is though.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-author
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Feb 25, 2020
…bank Minor refactoring of statement parsing Extracted out of rust-lang#69445. r? @estebank
petrochenkov
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Feb 26, 2020
…bank Minor refactoring of statement parsing Extracted out of rust-lang#69445. r? @estebank
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #69555) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I expected parsing the contents of a module/trait/impl/extern-block as a list of statements, then filtering-repackaging it into free/associated/foreign items, not trying to do recovery as a statement for something that can't be parsed as an item (which would of course require backtracking). My goal here is unifying parsing first of all, then thinking about diagnostics, so my suggested staging here would be
|
petrochenkov
added
S-waiting-on-author
|
[triage] This PR's status wasn't updated in two weeks or more, if it isn't updated in two more weeks the PR will be closed due to inactivity. |
Dylan-DPC-zz
added
S-inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suppose we have the following file, let's call it
foo.rs:In that case, we will attempt to parse these contiguous sequence of statements as-if they were part of a function body, recover, and suggest that the user move the statements into a function:
When emitting the error, we also attempt lightweight return type inference based on the parsed list of statements. In particular, if we detect
returnwe assume a default return type (fn foo() { ... }), if we findreturn $expror a tail expression, we assume not-()and suggest-> _, if we find$expr?we suggest-> Result<_, _>. Finally, we also attempt to account forfn mainas the function name by considering the inference result as well as the file name.After parsing the sequence of statements, we will continue by parsing an item, and if that fails, we again attempt statement parsing as above, and so on, and so forth. That is, we interleave item parsing with statement-list-as-function-body parsing.
This PR implements the "super goal" in #69366 (comment).
r? @petrochenkov @estebank