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 when unclosed char literal is parsed as a lifetime in some positions #101293

Merged
merged 1 commit into from
Oct 23, 2022

Conversation

compiler-errors
Copy link
Member

Fixes #101278

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 1, 2022
@rustbot

This comment was marked as outdated.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2022
@compiler-errors

This comment was marked as outdated.

@@ -1516,6 +1518,15 @@ impl<'a> Parser<'a> {
|| self.token.is_whole_block()
{
self.parse_block_expr(label, lo, BlockCheckMode::Default)
} else if !ate_colon
&& (matches!(self.token.kind, token::CloseDelim(_) | token::Comma)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using "close delim or comma or operator" here as a heuristic, because if we use something like "next token isn't colon" then we suggest 'a 0 => 'a' 0 which is nonsense.

@@ -394,6 +394,21 @@ impl<'a> Parser<'a> {
} else {
PatKind::Path(qself, path)
}
} else if matches!(self.token.kind, token::Lifetime(_))
&& !self.look_ahead(1, |token| matches!(token.kind, token::Colon))
Copy link
Member Author

@compiler-errors compiler-errors Sep 1, 2022

Choose a reason for hiding this comment

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

In pattern position, we're totally fine with using "next token isn't colon" as a heuristic. We could probably just always try to recover if it's a lifetime, because we never have 'a: label {} in a pattern position anyways, but it does keep us from suggesting something like let 'a: Ty = .. => let 'a': Ty = ..

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we include this as a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do

|
help: add `'` to close the char literal
|
LL | f<'a',>
Copy link
Member Author

@compiler-errors compiler-errors Sep 1, 2022

Choose a reason for hiding this comment

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

This is somewhat unfortunate, but teaching the parser to avoid suggesting this is just as hard as solving the turbofish problem I think, because at this point all the context we have is that we're parsing f < $EXPR where $EXPR begins with 'a

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, maybe I can avoid this if the prev token is <. Or maybe this suggestion is just nonsense and the PR needs to be scrapped.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine, this is best effort.

@compiler-errors
Copy link
Member Author

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned lcnr Sep 1, 2022
@bors
Copy link
Contributor

bors commented Sep 21, 2022

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

@compiler-errors
Copy link
Member Author

I'm gonna re-roll this since it's been almost two months.

r? compiler

@compiler-errors
Copy link
Member Author

r? compiler

rustbot wake up

@rust-highfive rust-highfive assigned jackh726 and unassigned estebank Oct 20, 2022
@compiler-errors
Copy link
Member Author

@jackh726 is also a busy person 🤣

r? diagnostics

@rust-highfive rust-highfive assigned davidtwco and unassigned jackh726 Oct 20, 2022
err: impl FnOnce(&mut Self) -> DiagnosticBuilder<'a, ErrorGuaranteed>,
) -> ast::Lit {
if let Some(mut diag) =
self.sess.span_diagnostic.steal_diagnostic(lifetime.span, StashKey::LifetimeIsChar)
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed the introduction of this more principled version of this mechanism :)

@@ -394,6 +394,21 @@ impl<'a> Parser<'a> {
} else {
PatKind::Path(qself, path)
}
} else if matches!(self.token.kind, token::Lifetime(_))
&& !self.look_ahead(1, |token| matches!(token.kind, token::Colon))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we include this as a comment?

|
help: add `'` to close the char literal
|
LL | f<'a',>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine, this is best effort.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 21, 2022

📌 Commit 0a92038fa1ae8d75ab1cd76e1ed535674799c6e7 has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2022
@compiler-errors
Copy link
Member Author

@bors r- i'll address that comment nit then re-r+

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 21, 2022
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 21, 2022
@davidtwco davidtwco assigned estebank and unassigned davidtwco Oct 21, 2022
@compiler-errors
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Oct 22, 2022

📌 Commit 0270b50 has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 22, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#101293 (Recover when unclosed char literal is parsed as a lifetime in some positions)
 - rust-lang#101908 (Suggest let for assignment, and some code refactor)
 - rust-lang#103192 (rustdoc: Eliminate uses of `EarlyDocLinkResolver::all_traits`)
 - rust-lang#103226 (Check `needs_infer` before `needs_drop` during HIR generator analysis)
 - rust-lang#103249 (resolve: Revert "Set effective visibilities for imports more precisely")
 - rust-lang#103305 (Move some tests to more reasonable places)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e029c1f into rust-lang:master Oct 23, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 23, 2022
@compiler-errors compiler-errors deleted the lt-is-actually-char branch November 2, 2022 03:00
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 30, 2024
…-errors

Be more careful about interpreting a label/lifetime as a mistyped char literal.

Currently the parser interprets any label/lifetime in certain positions as a mistyped char literal, on the assumption that the trailing single quote was accidentally omitted. In such cases it gives an error with a suggestion to add the trailing single quote, and then puts the appropriate char literal into the AST. This behaviour was introduced in rust-lang#101293.

This is reasonable for a case like this:
```
let c = 'a;
```
because `'a'` is a valid char literal. It's less reasonable for a case like this:
```
let c = 'abc;
```
because `'abc'` is not a valid char literal.

Prior to rust-lang#120329 this could result in some sub-optimal suggestions in error messages, but nothing else. But rust-lang#120329 changed `LitKind::from_token_lit` to assume that the char/byte/string literals it receives are valid, and to assert if not. This is reasonable because the lexer does not produce invalid char/byte/string literals in general. But in this "interpret label/lifetime as unclosed char literal" case the parser can produce an invalid char literal with contents such as `abc`, which triggers an assertion failure.

This PR changes the parser so it's more cautious about interpreting labels/lifetimes as unclosed char literals.

Fixes rust-lang#120397.

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2024
Rollup merge of rust-lang#120460 - nnethercote:fix-120397, r=compiler-errors

Be more careful about interpreting a label/lifetime as a mistyped char literal.

Currently the parser interprets any label/lifetime in certain positions as a mistyped char literal, on the assumption that the trailing single quote was accidentally omitted. In such cases it gives an error with a suggestion to add the trailing single quote, and then puts the appropriate char literal into the AST. This behaviour was introduced in rust-lang#101293.

This is reasonable for a case like this:
```
let c = 'a;
```
because `'a'` is a valid char literal. It's less reasonable for a case like this:
```
let c = 'abc;
```
because `'abc'` is not a valid char literal.

Prior to rust-lang#120329 this could result in some sub-optimal suggestions in error messages, but nothing else. But rust-lang#120329 changed `LitKind::from_token_lit` to assume that the char/byte/string literals it receives are valid, and to assert if not. This is reasonable because the lexer does not produce invalid char/byte/string literals in general. But in this "interpret label/lifetime as unclosed char literal" case the parser can produce an invalid char literal with contents such as `abc`, which triggers an assertion failure.

This PR changes the parser so it's more cautious about interpreting labels/lifetimes as unclosed char literals.

Fixes rust-lang#120397.

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unterminated char literal complains about lifetimes
8 participants