make error emitted on `impl &Trait` nicer #106712

Ezrashaw · 2023-01-11T09:15:56Z

Turned out to be simpler than I thought, also added UI test.

error: expected one of `:`, `@`, or `|`, found `)`
 --> src/main.rs:2:22
  |
2 | fn foo(_: impl &Trait) {}
  |                      ^ expected one of `:`, `@`, or `|`
  |
  = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
help: if this is a parameter name, give it a type
  |
2 | fn foo(_: impl Trait: &TypeName) {}
  |                ~~~~~~~~~~~~~~~~
help: if this is a type, explicitly ignore the parameter name
  |
2 | fn foo(_: impl _: &Trait) {}
  |                ++

error: expected one of `!`, `(`, `)`, `,`, `?`, `for`, `~`, lifetime, or path, found `&`
 --> src/main.rs:2:16
  |
2 | fn foo(_: impl &Trait) {}
  |               -^ expected one of 9 possible tokens
  |               |
  |               help: missing `,`

error: expected one of `!`, `(`, `,`, `=`, `>`, `?`, `for`, `~`, lifetime, or path, found `&`
 --> src/main.rs:3:11
  |
3 | fn bar<T: &Trait>(_: T) {}
  |           ^ expected one of 10 possible tokens

After:

error: expected a trait, found type
 --> <anon>:2:16
  |
2 | fn foo(_: impl &Trait) {}
  |                -^^^^^
  |                |
  |                help: consider removing the indirection

error: expected a trait, found type
 --> <anon>:3:11
  |
3 | fn bar<T: &Trait>(_: T) {}
  |           -^^^^^
  |           |
  |           help: consider removing the indirection

rustbot · 2023-01-11T09:16:06Z

r? @cjgillot

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

Ezrashaw · 2023-01-11T09:16:13Z

r? @estebank

compiler/rustc_parse/src/parser/ty.rs

estebank · 2023-01-11T18:55:25Z

Please address the comments above, but you'll have to rebase on top of a fresh master (we've moved tests from src/test to tests.

Ezrashaw · 2023-01-12T00:09:59Z

@estebank I assume you just want me to address fmease's comments, you didn't have any?

~~I think that this is blocked until the tests are moved.~~ I think we're good to go.

rustbot · 2023-01-12T01:20:09Z

Some changes occurred in src/tools/cargo

cc @ehuss

compiler/rustc_parse/src/parser/ty.rs

estebank · 2023-01-12T06:30:51Z

r=me after addressing the last comment (that I hadn't noticed earlier)

Ezrashaw · 2023-01-12T06:51:27Z

@estebank Also, the tests are failing, I have to rewrite my impl. It'll be a bit.

Ezrashaw · 2023-01-14T08:24:00Z

@estebank @fmease I've rewritten this PR, it's a lot better now, we don't have those pesky "added to expected tokens" issues. Another review would be great :).

and I seem have no idea how the "re-request review" button works??

estebank

One last nitpick before merging

estebank · 2023-01-14T18:46:47Z

compiler/rustc_parse/src/parser/ty.rs

+                // Just get the indirection part of the type.
+                let span = ty.span.until(path.span);
+
+                err.span_suggestion(


Suggested change

err.span_suggestion(

err.span_suggestion_verbose(

and then re--bless.

Done, thanks!

estebank · 2023-01-14T18:47:52Z

compiler/rustc_parse/src/parser/ty.rs

+            path
+        } else if !self.token.is_path_start() && self.token.can_begin_type() {
+            // Instead of finding a path (a trait), we found a type.
+            let mut err = self.struct_span_err(self.token.span, "expected a trait, found type");


Suggested change

let mut err = self.struct_span_err(self.token.span, "expected a trait, found type");

let mut err = self.struct_span_err(self.token.span, "expected a trait, found type start");

Specify "type start" because we haven't parsed a type yet, and the span points at & only, so it would be a "lie" (and potentially confusing) to talk about a type.

I've done this slightly differently. Instead we will only emit the nice error is there is a proper type, "found type start" seems a bit confusing to me.

Ezrashaw · 2023-01-15T09:59:26Z

@estebank Can we merge this now? Also could you r+ #106591? I don't have perms. Thanks!

estebank · 2023-01-15T20:12:26Z

compiler/rustc_parse/src/parser/ty.rs

@@ -938,6 +940,35 @@ impl<'a> Parser<'a> {
            && self.look_ahead(1, |tok| tok.kind == TokenKind::OpenDelim(Delimiter::Parenthesis))
            && let Some(path) = self.recover_path_from_fn()
        {
+            path
+        } else if !self.token.is_path_start() && self.token.can_begin_type() && let Ok(ty) = self.parse_ty_no_plus() {


The issue I'm seeing is that let Ok(ty) = self.parse_ty_no_plus() can fail parsing the type and will be returning an Err(Diagnostic), which gets discarded. If this happens, an ICE will occur. We need to either bubble it up (wich self.parse_ty_no_plus()?, which is fine if slightly misleading) or err.delay_as_bug() it (which isn't great without another mechanism to complain about the parse failure).

Hmm, what about matching on error and cancelling the diagnostic. I didn't properly realize that it returned a diagnostic, not just an error to be later turned into a diagnostic.

With my current code the following ICEs:

fn foo<T: &match>() {}

if we ? the parse call then we get:

error: expected type, found keyword `match` --> <anon>:1:12 | 1 | fn foo<T: &match>() {} | ^^^^^ expected type

I'm happy with this, I don't think it's confusing. We could however add a note, something along the lines of: this error was encountered when trying to recover from a "expected a trait, found type" error before bubbling it up.

You can do map_err(|mut err| { err.note(..); err })?, but I think even without the extra context this output would be fine.

estebank · 2023-01-17T02:03:46Z

compiler/rustc_parse/src/parser/ty.rs

+            // to recover from errors, not make more).
+            let path = if self.may_recover()
+                && matches!(ty.kind, TyKind::Ptr(..) | TyKind::Ref(..))
+                && let TyKind::Path(_, path) = &ty.peel_refs().kind {


Suggested change

&& let TyKind::Path(_, path) = &ty.peel_refs().kind {

&& let TyKind::Path(_, path) = &ty.peel_refs().kind

{

estebank · 2023-01-17T02:04:12Z

@bors r+

bors · 2023-01-17T02:04:14Z

📌 Commit fcd5ed2 has been approved by estebank

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#106591 (suggestion for attempted integer identifier in patterns) - rust-lang#106712 (make error emitted on `impl &Trait` nicer) - rust-lang#106829 (Unify `Opaque`/`Projection` handling in region outlives code) - rust-lang#106869 (rustdoc: remove redundant item kind class from `.item-decl > pre`) - rust-lang#106949 (ConstBlocks are poly if their substs are poly) - rust-lang#106953 (Document `EarlyBinder::subst_identity` and `skip_binder`) - rust-lang#106958 (Don't add A-bootstrap to PRs modifying Cargo.lock) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…macro-is-ok, r=estebank Do not eagerly recover for bad `impl Trait` types in macros Fixes rust-lang#107796 cc rust-lang#106712, `@estebank` and `@Ezrashaw` please make sure to use [`Parser::may_recover`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_parse/parser/struct.Parser.html#method.may_recover) for all eager-token-consuming parser recoveries. This also fixes a separate regression from rust-lang#99915, that was introduced before we added `may_recover` though.

…macro-is-ok, r=estebank Do not eagerly recover for bad `impl Trait` types in macros Fixes rust-lang#107796 cc rust-lang#106712, ``@estebank`` and ``@Ezrashaw`` please make sure to use [`Parser::may_recover`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_parse/parser/struct.Parser.html#method.may_recover) for all eager-token-consuming parser recoveries. This also fixes a separate regression from rust-lang#99915, that was introduced before we added `may_recover` though.

…macro-is-ok, r=estebank Do not eagerly recover for bad `impl Trait` types in macros Fixes rust-lang#107796 cc rust-lang#106712, ```@estebank``` and ```@Ezrashaw``` please make sure to use [`Parser::may_recover`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_parse/parser/struct.Parser.html#method.may_recover) for all eager-token-consuming parser recoveries. This also fixes a separate regression from rust-lang#99915, that was introduced before we added `may_recover` though.

rustbot assigned cjgillot Jan 11, 2023

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 11, 2023

rustbot assigned estebank and unassigned cjgillot Jan 11, 2023

Ezrashaw force-pushed the impl-ref-trait branch 4 times, most recently from e311816 to 068d03d Compare January 11, 2023 10:30

fmease reviewed Jan 11, 2023

View reviewed changes

compiler/rustc_parse/src/parser/ty.rs Outdated Show resolved Hide resolved

fmease reviewed Jan 11, 2023

View reviewed changes

compiler/rustc_parse/src/parser/ty.rs Outdated Show resolved Hide resolved

fmease reviewed Jan 11, 2023

View reviewed changes

compiler/rustc_parse/src/parser/ty.rs Outdated Show resolved Hide resolved

Ezrashaw force-pushed the impl-ref-trait branch from 068d03d to 32b95d7 Compare January 12, 2023 00:06

Ezrashaw force-pushed the impl-ref-trait branch 2 times, most recently from b316295 to b244273 Compare January 12, 2023 01:20

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 12, 2023

Ezrashaw force-pushed the impl-ref-trait branch from b244273 to c3c6760 Compare January 12, 2023 01:21

estebank reviewed Jan 12, 2023

View reviewed changes

compiler/rustc_parse/src/parser/ty.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Ezrashaw force-pushed the impl-ref-trait branch from c3c6760 to 46e7961 Compare January 14, 2023 08:20

Ezrashaw requested review from estebank and fmease and removed request for estebank January 14, 2023 08:21

Ezrashaw requested review from estebank and fmease and removed request for fmease and estebank January 14, 2023 08:21

estebank reviewed Jan 14, 2023

View reviewed changes

make error emitted on impl &Trait nicer

e590b93

Ezrashaw force-pushed the impl-ref-trait branch from 46e7961 to e590b93 Compare January 14, 2023 23:24

estebank reviewed Jan 15, 2023

View reviewed changes

fix dropping diagnostic without emit

fcd5ed2

estebank reviewed Jan 17, 2023

View reviewed changes

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 Jan 17, 2023

matthiaskrgr mentioned this pull request Jan 17, 2023

Rollup of 7 pull requests #106966

Merged

bors merged commit 9cda9e0 into rust-lang:master Jan 17, 2023

rustbot added this to the 1.68.0 milestone Jan 17, 2023

Ezrashaw deleted the impl-ref-trait branch January 17, 2023 08:27

compiler-errors mentioned this pull request Feb 8, 2023

Do not eagerly recover for bad impl Trait types in macros #107813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make error emitted on `impl &Trait` nicer #106712

make error emitted on `impl &Trait` nicer #106712

Ezrashaw commented Jan 11, 2023

rustbot commented Jan 11, 2023

Ezrashaw commented Jan 11, 2023

estebank commented Jan 11, 2023

Ezrashaw commented Jan 12, 2023 •

edited

Loading

rustbot commented Jan 12, 2023

estebank commented Jan 12, 2023

Ezrashaw commented Jan 12, 2023 •

edited

Loading

This comment has been minimized.

Ezrashaw commented Jan 14, 2023 •

edited

Loading

estebank left a comment

estebank Jan 14, 2023

Ezrashaw Jan 14, 2023

estebank Jan 14, 2023

Ezrashaw Jan 14, 2023

Ezrashaw commented Jan 15, 2023

estebank Jan 15, 2023

Ezrashaw Jan 15, 2023

Ezrashaw Jan 16, 2023

estebank Jan 17, 2023

estebank Jan 17, 2023

estebank commented Jan 17, 2023

bors commented Jan 17, 2023

	let mut err = self.struct_span_err(self.token.span, "expected a trait, found type");
	let mut err = self.struct_span_err(self.token.span, "expected a trait, found type start");

	&& let TyKind::Path(_, path) = &ty.peel_refs().kind {
	&& let TyKind::Path(_, path) = &ty.peel_refs().kind
	{

make error emitted on impl &Trait nicer #106712

make error emitted on impl &Trait nicer #106712

Conversation

Ezrashaw commented Jan 11, 2023

rustbot commented Jan 11, 2023

Ezrashaw commented Jan 11, 2023

estebank commented Jan 11, 2023

Ezrashaw commented Jan 12, 2023 • edited Loading

rustbot commented Jan 12, 2023

estebank commented Jan 12, 2023

Ezrashaw commented Jan 12, 2023 • edited Loading

This comment has been minimized.

Ezrashaw commented Jan 14, 2023 • edited Loading

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezrashaw commented Jan 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jan 17, 2023

bors commented Jan 17, 2023

make error emitted on `impl &Trait` nicer #106712

make error emitted on `impl &Trait` nicer #106712

Ezrashaw commented Jan 12, 2023 •

edited

Loading

Ezrashaw commented Jan 12, 2023 •

edited

Loading

Ezrashaw commented Jan 14, 2023 •

edited

Loading