Make inline suggestions no longer be the default #127282

estebank · 2024-07-03T18:30:44Z

Make the default suggestions show the full patch output instead of trying to render inline.

Before

error: almost complete ascii range
  --> tests/ui/almost_complete_range.rs:17:17
   |
LL |         let _ = ('a') ..'z';
   |                 ^^^^^^--^^^
   |                       |
   |                       help: use an inclusive range: `..=`
   |
   = note: `-D clippy::almost-complete-range` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::almost_complete_range)]`

After

error: almost complete ascii range
  --> tests/ui/almost_complete_range.rs:17:17
   |
LL |         let _ = ('a') ..'z';
   |                 ^^^^^^^^^^^
   |
   = note: `-D clippy::almost-complete-range` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::almost_complete_range)]`
help: use an inclusive range
   |
LL |         let _ = ('a') ..='z';
   |                       ~~~

The inline suggestions output has worse readability than the "verbose" output, and it also (as evidenced in the diff of this PR) tends to hide when the suggestion span is slightly off. Backing off of the inline suggestions will also make it easier to migrate to annotate-snippets, as it will be one fewer special case to handle.

rustbot · 2024-07-03T18:30:54Z

r? @fmease

rustbot has assigned @fmease.
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 · 2024-07-03T19:19:06Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

estebank · 2024-07-03T21:04:23Z

I noticed a bunch of mistakes in current suggestions thanks to the new output, but all of those can go on other PRs.

estebank · 2024-07-03T21:07:38Z

src/tools/clippy/tests/ui/almost_complete_range.stderr

+help: use an inclusive range
+   |
+LL |         'a'...'z' => 1,
+   |            ~~~


This clippy suggestions are just plain wrong, right?

It's the old pattern syntax for when MSRV is set to the ancient times

rust/src/tools/clippy/tests/ui/almost_complete_range.rs

Lines 70 to 78 in 2b90614

#[clippy::msrv = "1.25"]

fn _under_msrv() {

let _ = match 'a' {

'a'..'z' => 1,

'A'..'Z' => 2,

'0'..'9' => 3,

_ => 4,

};

}

estebank · 2024-07-03T21:11:34Z

src/tools/clippy/tests/ui/async_yields_async.stderr

+LL |         CustomFutureType.await
+   |


Interesting, we're missing an underline here.

Does this happen, because we're replacing the entire line? The clippy suggestion probably doesn't add the .await, but suggests to remove the CustomFutureType and add CustomFutureType.await.

estebank · 2024-07-03T21:11:51Z

src/tools/clippy/tests/ui/bind_instead_of_map.stderr

-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.map(|o| o + 1)`
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: try


Clippyy needs to have better messages for these suggestions

https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/On.20short.20help.20messages

Tweak some structured suggestions to be more verbose and accurate Addressing some issues I found while working on rust-lang#127282. ``` error: this URL is not a hyperlink --> $DIR/auxiliary/include-str-bare-urls.md:1:11 | LL | HEADS UP! https://example.com MUST SHOW UP IN THE STDERR FILE! | ^^^^^^^^^^^^^^^^^^^ | = note: bare URLs are not automatically turned into clickable links note: the lint level is defined here --> $DIR/include-str-bare-urls.rs:14:9 | LL | #![deny(rustdoc::bare_urls)] | ^^^^^^^^^^^^^^^^^^ help: use an automatic link instead | LL | HEADS UP! <https://example.com> MUST SHOW UP IN THE STDERR FILE! | + + ``` ``` error[E0384]: cannot assign twice to immutable variable `v` --> $DIR/assign-imm-local-twice.rs:7:5 | LL | v = 1; | ----- first assignment to `v` LL | println!("v={}", v); LL | v = 2; | ^^^^^ cannot assign twice to immutable variable | help: consider making this binding mutable | LL | let mut v: isize; | +++ ``` ``` error[E0393]: the type parameter `Rhs` must be explicitly specified --> $DIR/issue-22560.rs:9:23 | LL | trait Sub<Rhs=Self> { | ------------------- type parameter `Rhs` must be specified for this ... LL | type Test = dyn Add + Sub; | ^^^ | = note: because of the default `Self` reference, type parameters must be specified on object types help: set the type parameter to the desired type | LL | type Test = dyn Add + Sub<Rhs>; | +++++ ``` ``` error[E0596]: cannot borrow `v` as mutable, as it is not declared as mutable --> $DIR/issue-33819.rs:4:34 | LL | Some(ref v) => { let a = &mut v; }, | ^^^^^^ cannot borrow as mutable | help: try removing `&mut` here | LL - Some(ref v) => { let a = &mut v; }, LL + Some(ref v) => { let a = v; }, | ``` ``` help: remove the invocation before committing it to a version control system | LL - dbg!(); | ``` ``` error[E0308]: mismatched types --> $DIR/issue-39974.rs:1:21 | LL | const LENGTH: f64 = 2; | ^ expected `f64`, found integer | help: use a float literal | LL | const LENGTH: f64 = 2.0; | ++ ``` ``` error[E0529]: expected an array or slice, found `Vec<i32>` --> $DIR/match-ergonomics.rs:8:9 | LL | [&v] => {}, | ^^^^ pattern cannot match with input type `Vec<i32>` | help: consider slicing here | LL | match x[..] { | ++++ ``` ``` error[E0609]: no field `0` on type `[u32; 1]` --> $DIR/parenthesized-deref-suggestion.rs:10:21 | LL | (x as [u32; 1]).0; | ^ unknown field | help: instead of using tuple indexing, use array indexing | LL | (x as [u32; 1])[0]; | ~ + ```

Rollup merge of rust-lang#127301 - estebank:fix-suggestions, r=Urgau Tweak some structured suggestions to be more verbose and accurate Addressing some issues I found while working on rust-lang#127282. ``` error: this URL is not a hyperlink --> $DIR/auxiliary/include-str-bare-urls.md:1:11 | LL | HEADS UP! https://example.com MUST SHOW UP IN THE STDERR FILE! | ^^^^^^^^^^^^^^^^^^^ | = note: bare URLs are not automatically turned into clickable links note: the lint level is defined here --> $DIR/include-str-bare-urls.rs:14:9 | LL | #![deny(rustdoc::bare_urls)] | ^^^^^^^^^^^^^^^^^^ help: use an automatic link instead | LL | HEADS UP! <https://example.com> MUST SHOW UP IN THE STDERR FILE! | + + ``` ``` error[E0384]: cannot assign twice to immutable variable `v` --> $DIR/assign-imm-local-twice.rs:7:5 | LL | v = 1; | ----- first assignment to `v` LL | println!("v={}", v); LL | v = 2; | ^^^^^ cannot assign twice to immutable variable | help: consider making this binding mutable | LL | let mut v: isize; | +++ ``` ``` error[E0393]: the type parameter `Rhs` must be explicitly specified --> $DIR/issue-22560.rs:9:23 | LL | trait Sub<Rhs=Self> { | ------------------- type parameter `Rhs` must be specified for this ... LL | type Test = dyn Add + Sub; | ^^^ | = note: because of the default `Self` reference, type parameters must be specified on object types help: set the type parameter to the desired type | LL | type Test = dyn Add + Sub<Rhs>; | +++++ ``` ``` error[E0596]: cannot borrow `v` as mutable, as it is not declared as mutable --> $DIR/issue-33819.rs:4:34 | LL | Some(ref v) => { let a = &mut v; }, | ^^^^^^ cannot borrow as mutable | help: try removing `&mut` here | LL - Some(ref v) => { let a = &mut v; }, LL + Some(ref v) => { let a = v; }, | ``` ``` help: remove the invocation before committing it to a version control system | LL - dbg!(); | ``` ``` error[E0308]: mismatched types --> $DIR/issue-39974.rs:1:21 | LL | const LENGTH: f64 = 2; | ^ expected `f64`, found integer | help: use a float literal | LL | const LENGTH: f64 = 2.0; | ++ ``` ``` error[E0529]: expected an array or slice, found `Vec<i32>` --> $DIR/match-ergonomics.rs:8:9 | LL | [&v] => {}, | ^^^^ pattern cannot match with input type `Vec<i32>` | help: consider slicing here | LL | match x[..] { | ++++ ``` ``` error[E0609]: no field `0` on type `[u32; 1]` --> $DIR/parenthesized-deref-suggestion.rs:10:21 | LL | (x as [u32; 1]).0; | ^ unknown field | help: instead of using tuple indexing, use array indexing | LL | (x as [u32; 1])[0]; | ~ + ```

bors · 2024-07-04T19:31:31Z

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

Before ``` error: almost complete ascii range --> tests/ui/almost_complete_range.rs:17:17 | LL | let _ = ('a') ..'z'; | ^^^^^^--^^^ | | | help: use an inclusive range: `..=` | = note: `-D clippy::almost-complete-range` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::almost_complete_range)]` ``` After ``` error: almost complete ascii range --> tests/ui/almost_complete_range.rs:17:17 | LL | let _ = ('a') ..'z'; | ^^^^^^^^^^^ | = note: `-D clippy::almost-complete-range` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::almost_complete_range)]` help: use an inclusive range | LL | let _ = ('a') ..='z'; | ~~~ ```

rustbot · 2024-07-05T00:27:35Z

HIR ty lowering was modified

cc @fmease

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred in tests/ui/check-cfg

cc @Urgau

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in match checking

cc @Nadrieril

estebank · 2024-07-05T00:28:24Z

This PR is too big as is, but the diff helps identify specific suggestions that are currently not-perfect, or flat-out wrong. I'll try to fix those things one at a time in separate PRs, making this PR smaller and smaller over time.

bors · 2024-07-05T12:06:05Z

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

Tweak some structured suggestions to be more verbose and accurate Addressing some issues I found while working on rust-lang#127282. ``` error: this URL is not a hyperlink --> $DIR/auxiliary/include-str-bare-urls.md:1:11 | LL | HEADS UP! https://example.com MUST SHOW UP IN THE STDERR FILE! | ^^^^^^^^^^^^^^^^^^^ | = note: bare URLs are not automatically turned into clickable links note: the lint level is defined here --> $DIR/include-str-bare-urls.rs:14:9 | LL | #![deny(rustdoc::bare_urls)] | ^^^^^^^^^^^^^^^^^^ help: use an automatic link instead | LL | HEADS UP! <https://example.com> MUST SHOW UP IN THE STDERR FILE! | + + ``` ``` error[E0384]: cannot assign twice to immutable variable `v` --> $DIR/assign-imm-local-twice.rs:7:5 | LL | v = 1; | ----- first assignment to `v` LL | println!("v={}", v); LL | v = 2; | ^^^^^ cannot assign twice to immutable variable | help: consider making this binding mutable | LL | let mut v: isize; | +++ ``` ``` error[E0393]: the type parameter `Rhs` must be explicitly specified --> $DIR/issue-22560.rs:9:23 | LL | trait Sub<Rhs=Self> { | ------------------- type parameter `Rhs` must be specified for this ... LL | type Test = dyn Add + Sub; | ^^^ | = note: because of the default `Self` reference, type parameters must be specified on object types help: set the type parameter to the desired type | LL | type Test = dyn Add + Sub<Rhs>; | +++++ ``` ``` error[E0596]: cannot borrow `v` as mutable, as it is not declared as mutable --> $DIR/issue-33819.rs:4:34 | LL | Some(ref v) => { let a = &mut v; }, | ^^^^^^ cannot borrow as mutable | help: try removing `&mut` here | LL - Some(ref v) => { let a = &mut v; }, LL + Some(ref v) => { let a = v; }, | ``` ``` help: remove the invocation before committing it to a version control system | LL - dbg!(); | ``` ``` error[E0308]: mismatched types --> $DIR/issue-39974.rs:1:21 | LL | const LENGTH: f64 = 2; | ^ expected `f64`, found integer | help: use a float literal | LL | const LENGTH: f64 = 2.0; | ++ ``` ``` error[E0529]: expected an array or slice, found `Vec<i32>` --> $DIR/match-ergonomics.rs:8:9 | LL | [&v] => {}, | ^^^^ pattern cannot match with input type `Vec<i32>` | help: consider slicing here | LL | match x[..] { | ++++ ``` ``` error[E0609]: no field `0` on type `[u32; 1]` --> $DIR/parenthesized-deref-suggestion.rs:10:21 | LL | (x as [u32; 1]).0; | ^ unknown field | help: instead of using tuple indexing, use array indexing | LL | (x as [u32; 1])[0]; | ~ + ```

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

rustbot assigned fmease Jul 3, 2024

rustbot added S-waiting-on-review T-compiler labels Jul 3, 2024

estebank changed the title ~~Experiment with not using inline suggestions~~ Make inline suggestions no longer be the default Jul 3, 2024

estebank force-pushed the experiment-for-annotate-snippets branch 2 times, most recently from 8ef3d49 to 1791206 Compare July 3, 2024 19:18

estebank marked this pull request as ready for review July 3, 2024 19:19

This comment has been minimized.

Sign in to view

estebank force-pushed the experiment-for-annotate-snippets branch from 1791206 to ba252ed Compare July 3, 2024 19:52

estebank commented Jul 3, 2024

View reviewed changes

estebank mentioned this pull request Jul 4, 2024

Tweak some structured suggestions to be more verbose and accurate #127301

Merged

estebank force-pushed the experiment-for-annotate-snippets branch from 20bf5b1 to eb6c593 Compare July 5, 2024 00:27

estebank marked this pull request as draft July 12, 2024 18:47

Alexendoo mentioned this pull request Aug 21, 2024

Wrong help message suggested for unwrap_or_else rust-lang/rust-clippy#13293

Closed

fmease added S-waiting-on-author and removed S-waiting-on-review labels Sep 18, 2024

estebank mentioned this pull request Feb 20, 2025

preserve colors in diagnostics rust-lang/rust-playground#1117

Open

estebank mentioned this pull request Feb 20, 2025

Make some suggestions "verbose" #137343

Open

fmease added S-experimental and removed S-waiting-on-author labels Feb 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make inline suggestions no longer be the default #127282

Make inline suggestions no longer be the default #127282

estebank commented Jul 3, 2024 •

edited

Loading

rustbot commented Jul 3, 2024

rustbot commented Jul 3, 2024

This comment has been minimized.

estebank commented Jul 3, 2024

estebank Jul 3, 2024

Alexendoo Jul 3, 2024

estebank Jul 3, 2024

flip1995 Jul 4, 2024

estebank Jul 3, 2024

blyxyas Jul 3, 2024

bors commented Jul 4, 2024

rustbot commented Jul 5, 2024

estebank commented Jul 5, 2024

bors commented Jul 5, 2024

	#[clippy::msrv = "1.25"]
	fn _under_msrv() {
	let _ = match 'a' {
	'a'..'z' => 1,
	'A'..'Z' => 2,
	'0'..'9' => 3,
	_ => 4,
	};
	}

Make inline suggestions no longer be the default #127282

Are you sure you want to change the base?

Make inline suggestions no longer be the default #127282

Conversation

estebank commented Jul 3, 2024 • edited Loading

rustbot commented Jul 3, 2024

rustbot commented Jul 3, 2024

This comment has been minimized.

estebank commented Jul 3, 2024

estebank Jul 3, 2024

Choose a reason for hiding this comment

Alexendoo Jul 3, 2024

Choose a reason for hiding this comment

estebank Jul 3, 2024

Choose a reason for hiding this comment

flip1995 Jul 4, 2024

Choose a reason for hiding this comment

estebank Jul 3, 2024

Choose a reason for hiding this comment

blyxyas Jul 3, 2024

Choose a reason for hiding this comment

bors commented Jul 4, 2024

rustbot commented Jul 5, 2024

estebank commented Jul 5, 2024

bors commented Jul 5, 2024

estebank commented Jul 3, 2024 •

edited

Loading