Skip to content
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

Recover from pub let #107047

Closed
wants to merge 4 commits into from
Closed

Recover from pub let #107047

wants to merge 4 commits into from

Conversation

obeis
Copy link
Contributor

@obeis obeis commented Jan 18, 2023

Closes #101622

@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2023

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 18, 2023
@Noratrieb
Copy link
Member

The issue has the following idea:

Detect pub let specifically, and suggest either let or pub static/const. (This might be part of a general improvement to handling confusion between items and statements.

Presumably because some people try to use let for global variables (when they should use static instead).
I think it would make sense to recover pub let as pub static. For this, you'd have to modify the item parsing code instead of the visibility parsing code to also catch let just like static. You can then emit an error and actually recover to a ItemKind::Static.
You have to make sure to not break normal let local variables with it :D (for example by only recovering let when not in a statement context). You might even be able to fix #92615 with it depending on how you do it.

@michaelwoerister
Copy link
Member

r? diagnostics

@Noratrieb Noratrieb changed the title Reconver from pub let Recover from pub let Jan 19, 2023
@Noratrieb Noratrieb 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 Jan 19, 2023
@obeis
Copy link
Contributor Author

obeis commented Jan 20, 2023

@rustbot ready

@rustbot rustbot 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 Jan 20, 2023
@compiler-errors
Copy link
Member

I'm still not confident that this is the right place to put this code.

Have you tried placing the recovery into parse_item_kind instead? There's basically a big if-else chain that parses every item kind. You could probably add a recovery for let there?

Ideally, we'd consolidate this code into that as well:

let label = if self.is_kw_followed_by_ident(kw::Let) {
"consider using `const` or `static` instead of `let` for global variables"
} else {
"expected item"
};

@obeis
Copy link
Contributor Author

obeis commented Jan 22, 2023

if !self.eat(term) {
let token_str = super::token_descr(&self.token);
if !self.maybe_consume_incorrect_semicolon(&items) {
let msg = &format!("expected item, found {token_str}");
let mut err = self.struct_span_err(self.token.span, msg);
let label = if self.is_kw_followed_by_ident(kw::Let) {
"consider using `const` or `static` instead of `let` for global variables"
} else {
"expected item"
};
err.span_label(self.token.span, label);
return Err(err);
}
}

When i converted the code above to this:

 if !self.eat(term) { 
     self.maybe_consume_incorrect_semicolon(&items); 
 } 

I got this weird error when main() function is exists

error[E0601]: `main` function not found in crate `r#main`
 --> main.rs:1:4
  |
1 | pub let x = 0;
  |    ^ consider adding a `main` function to `main.rs`

Also when moving the recover code to parse_item_kind, we need to figure out is the let inside function or outside function, I don't know how this can be done.

@bors
Copy link
Contributor

bors commented Feb 2, 2023

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

@compiler-errors
Copy link
Member

compiler-errors commented Feb 2, 2023

When i converted the code above to this [...] I got this weird error when main() function exists.

(edited) I'm not sure. You need to share an example of the code :/

Also when moving the recover code to parse_item_kind, we need to figure out is the let inside function or outside function, I don't know how this can be done.

I guess that's a good point :/

@obeis obeis force-pushed the pub-let branch 2 times, most recently from eda2aaf to 85a20ab Compare February 3, 2023 07:51
@obeis obeis requested a review from compiler-errors February 3, 2023 10:11
@bors
Copy link
Contributor

bors commented Feb 9, 2023

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

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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 1, 2023
@obeis
Copy link
Contributor Author

obeis commented Mar 3, 2023

@rustbot ready

@rustbot rustbot 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 Mar 3, 2023
@bors
Copy link
Contributor

bors commented Mar 12, 2023

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

@compiler-errors
Copy link
Member

r? @Nilstrieb since you like reviewing parser recoveries 😸 and i've given this pr enough review, and don't want to block it

@rustbot rustbot assigned Noratrieb and unassigned compiler-errors Mar 26, 2023
"expected item"
};
err.span_label(self.token.span, label);
if self.is_kw_followed_by_ident(kw::Let) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

@@ -111,6 +111,11 @@ impl<'a> Parser<'a> {
// Do not attempt to parse an expression if we're done here.
self.error_outer_attrs(attrs);
self.mk_stmt(lo, StmtKind::Empty)
} else if self.prev_token.is_keyword(kw::Pub) && self.token.is_keyword(kw::Let) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the function is parse_stmt_without_recovery, so I don't think it's a good idea to do recovery here. I think there was some kind of other function that gets called on invalid statements (I forgot the name but -Ztreat-err-as-bug may guide you there). Maybe you could check there?

@Noratrieb Noratrieb 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 Apr 2, 2023
@Dylan-DPC
Copy link
Member

@obeis any updates on this?

@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC closed this Aug 1, 2023
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2023
@obeis obeis deleted the pub-let branch January 14, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor recovery from pub let x = ...
7 participants