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.

Sign up for GitHub

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

Jump to bottom

Make some suggestions "verbose" #137343

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make some suggestions "verbose" #137343

wants to merge 1 commit into from
+1,585 −517

Conversation

estebank
Copy link
Contributor

The verbose "diff" suggestion format is easier to read (specially since #136958), and the only one supported by annotate-snippets (which we want to migrate to).

Making verbose the default in one go would result in a huge PR that is too hard to land: #127282.

CC rust-lang/rust-playground#1117

@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2025

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2025

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 20, 2025
estebank
estebank commented Feb 20, 2025
View reviewed changes
Copy link
Contributor Author

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Going over the diff, the new output makes some mistakes in the existing suggestions more obvious. Annotated them to address them later.

tests/ui/issues/issue-28433.stderr
Comment on lines +21 to +25
help: remove the qualifier
|
LL - pub(crate) Dove
LL + Dove
|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover leading whitespace

tests/ui/lazy-type-alias/leading-where-clause.stderr
Comment on lines +51 to +55
help: remove this `where`
|
LL - pub type EmptyLeading1<T> where = T where T: Copy;
LL + pub type EmptyLeading1<T> = T where T: Copy;
|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover whitespace

tests/ui/parser/foreign-ty-semantic-fail.stderr
Comment on lines +94 to +98
help: remove the `where` clause
|
LL - type E: where;
LL + type E: ;
|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover whitespace and :

tests/ui/privacy/sysroot-private.default.stderr
Comment on lines +31 to +35
help: `Trait` has the following associated type
|
LL - type AssociatedTy = dyn Trait<ExpressionStack = i32, Bar = i32>;
LL + type AssociatedTy = dyn Trait<Bar = i32, Bar = i32>;
|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggesting associated type that is already there.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member

I feel like the justification here is still somewhat missing here 🤔

and the only one supported by annotate-snippets (which we want to migrate to).

This seems like a limitation of annotate-snippets. I kinda don't think we should be doing major migrations of rustc diagnostics just because annotate-snippets is missing a functionality that rustc has. Especially without any justification for why annotate-snippets doesn't support inline suggestions. Can you please elaborate here?

Also, what does this have to do with rust-lang/rust-playground#1117?

While some of these diagnostics improvements are neutral, I think a lot of them make the error message quite a lot longer and harder to read. #137348 fixes some of them, but not sure how many still are a bit too verbose and what we need to do to fix them further.

@compiler-errors
Copy link
Member

I'm happy to review any of the fixes called by all of the above inline comments as separate PR(s), prior to converting them being verbose. They're broken on the master branch after all.

estebank added a commit to estebank/rust that referenced this pull request Feb 22, 2025
@estebank
Fix incorrect suggestion for never patterns with bodies or guard
d112fca 
When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         ! if true => {}
   |                      ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         ! if true => {}
   |
```

Noticed in rust-lang#137343 (comment).
@estebank estebank mentioned this pull request Feb 22, 2025
Open
estebank added a commit to estebank/rust that referenced this pull request Feb 22, 2025
@estebank
Fix incorrect suggestion for never patterns with bodies or guard
15b16fe 
When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         Some(!) if true => {}
   |                            ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         Some(!) if true => {}
LL +         Some(!),
   |
```

Noticed in rust-lang#137343 (comment).
estebank added a commit to estebank/rust that referenced this pull request Feb 22, 2025
@estebank
Fix incorrect suggestion for never patterns with bodies or guard
720a64a 
When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         Some(!) if true => {}
   |                            ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         Some(!) if true => {}
LL +         Some(!),
   |
```

Noticed in rust-lang#137343 (comment).
@bors
Copy link
Collaborator

bors commented Feb 23, 2025

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

estebank added a commit to estebank/rust that referenced this pull request Feb 25, 2025
@estebank
Fix incorrect suggestion for never patterns with bodies or guard
e5d92bb 
When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         Some(!) if true => {}
   |                            ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         Some(!) if true => {}
LL +         Some(!),
   |
```

Noticed in rust-lang#137343 (comment).
@estebank
Make some suggestions "verbose"
0af7fa2 
The verbose "diff" suggestion format is easier to read, and the only one supported by annotate-snippets.

Making verbose the default in one go would result in a huge PR that is too hard to land: rust-lang#127282.

CC rust-lang/rust-playground#1117
estebank added a commit to estebank/rust that referenced this pull request Feb 28, 2025
@estebank
Fix incorrect cfg structured suggestion
849f73a 
Keep a span for the attribute *within* the square brackets as part of
the `AttrKind`.

```
error: `cfg` is not followed by parentheses
  --> $DIR/cfg-attr-syntax-validation.rs:4:1
   |
LL | #[cfg = 10]
   | ^^^^^^^^^^^
   |
help: expected syntax is
   |
LL - #[cfg = 10]
LL + #[cfg(/* predicate */)]
   |
```

Noticed in rust-lang#137343 (comment).
@estebank estebank mentioned this pull request Feb 28, 2025
Open
estebank added a commit to estebank/rust that referenced this pull request Feb 28, 2025
@estebank
Fix incorrect cfg structured suggestion
747b7b3 
Keep a span for the attribute *within* the square brackets as part of
the `AttrKind`.

```
error: `cfg` is not followed by parentheses
  --> $DIR/cfg-attr-syntax-validation.rs:4:1
   |
LL | #[cfg = 10]
   | ^^^^^^^^^^^
   |
help: expected syntax is
   |
LL - #[cfg = 10]
LL + #[cfg(/* predicate */)]
   |
```

Noticed in rust-lang#137343 (comment).
estebank added a commit to estebank/rust that referenced this pull request Feb 28, 2025
@estebank
Fix incorrect suggestion for never patterns with bodies or guard
76bac19 
When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         Some(!) if true => {}
   |                            ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         Some(!) if true => {}
LL +         Some(!),
   |
```

Noticed in rust-lang#137343 (comment).
@estebank estebank added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2025
@matthiaskrgr
Rollup merge of rust-lang#137409 - estebank:match-arm, r=compiler-errors
a50a89f 
Tweak comma handling of "missing match arm" suggestion and fix "remove this arm" suggestion, and make suggestion verbose

Better track trailing commas in match arms. Do not suggest adding trailing comma to match arm with block body. Better heuristic for "is this match in one line".

When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         ! if true => {}
   |                      ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         ! if true => {}
   |
```

Noticed in rust-lang#137343 (comment).

r? `@compiler-errors`

The first commit is independent of the second, but to make the second one produce accurate suggestions the span needs to include the trailing comma, hence the grouping of both changes in this PR.
estebank added a commit to estebank/rust that referenced this pull request Mar 1, 2025
@estebank
Fix incorrect suggestion for never patterns with bodies or guard
49e776a 
When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         Some(!) if true => {}
   |                            ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         Some(!) if true => {}
LL +         Some(!),
   |
```

Noticed in rust-lang#137343 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@estebank @rustbot @rust-log-analyzer @compiler-errors @bors @oli-obk