Rollup merge of #126818 - estebank:suggestions-fix, r=wesleywiser

Better handle suggestions for the already present code and fix some suggestions When a suggestion part is for code that is already present, skip it. If all the suggestion parts for a suggestion are for code that is already there, do not emit the suggestion. Fix two suggestions that treat `span_suggestion` as if it were `span_help`.
rust-lang · Aug 2, 2024 · d1d57bd · d1d57bd
2 parents 425ae69 + 8ce8c42
commit d1d57bd
Show file tree

Hide file tree

Showing 12 changed files with 75 additions and 58 deletions.
diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs
@@ -1767,7 +1767,10 @@ impl HumanEmitter {
         debug!(?suggestions);
 
         if suggestions.is_empty() {
-            // Suggestions coming from macros can have malformed spans. This is a heavy handed
+            // Here we check if there are suggestions that have actual code changes. We sometimes
+            // suggest the same code that is already there, instead of changing how we produce the
+            // suggestions and filtering there, we just don't emit the suggestion.
+            // Suggestions coming from macros can also have malformed spans. This is a heavy handed
             // approach to avoid ICEs by ignoring the suggestion outright.
             return Ok(());
         }
@@ -2046,7 +2049,9 @@ impl HumanEmitter {
                     assert!(underline_start >= 0 && underline_end >= 0);
                     let padding: usize = max_line_num_len + 3;
                     for p in underline_start..underline_end {
-                        if let DisplaySuggestion::Underline = show_code_change {
+                        if let DisplaySuggestion::Underline = show_code_change
+                            && is_different(sm, &part.snippet, part.span)
+                        {
                             // If this is a replacement, underline with `~`, if this is an addition
                             // underline with `+`.
                             buffer.putc(
@@ -2824,6 +2829,18 @@ impl Style {
     }
 }
 
+/// Whether the original and suggested code are the same.
+pub fn is_different(sm: &SourceMap, suggested: &str, sp: Span) -> bool {
+    let found = match sm.span_to_snippet(sp) {
+        Ok(snippet) => snippet,
+        Err(e) => {
+            warn!(error = ?e, "Invalid span {:?}", sp);
+            return true;
+        }
+    };
+    found != suggested
+}
+
 /// Whether the original and suggested code are visually similar enough to warrant extra wording.
 pub fn is_case_difference(sm: &SourceMap, suggested: &str, sp: Span) -> bool {
     // FIXME: this should probably be extended to also account for `FO0` → `FOO` and unicode.

diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
@@ -50,7 +50,7 @@ pub use diagnostic_impls::{
     IndicateAnonymousLifetime, SingleLabelManySpans,
 };
 pub use emitter::ColorConfig;
-use emitter::{is_case_difference, DynEmitter, Emitter};
+use emitter::{is_case_difference, is_different, DynEmitter, Emitter};
 use registry::Registry;
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
 use rustc_data_structures::stable_hasher::{Hash128, StableHasher};
@@ -357,10 +357,16 @@ impl CodeSuggestion {
                             _ => 1,
                         })
                         .sum();
-                    line_highlight.push(SubstitutionHighlight {
-                        start: (cur_lo.col.0 as isize + acc) as usize,
-                        end: (cur_lo.col.0 as isize + acc + len) as usize,
-                    });
+                    if !is_different(sm, &part.snippet, part.span) {
+                        // Account for cases where we are suggesting the same code that's already
+                        // there. This shouldn't happen often, but in some cases for multipart
+                        // suggestions it's much easier to handle it here than in the origin.
+                    } else {
+                        line_highlight.push(SubstitutionHighlight {
+                            start: (cur_lo.col.0 as isize + acc) as usize,
+                            end: (cur_lo.col.0 as isize + acc + len) as usize,
+                        });
+                    }
                     buf.push_str(&part.snippet);
                     let cur_hi = sm.lookup_char_pos(part.span.hi());
                     // Account for the difference between the width of the current code and the
@@ -392,7 +398,11 @@ impl CodeSuggestion {
                 while buf.ends_with('\n') {
                     buf.pop();
                 }
-                Some((buf, substitution.parts, highlights, only_capitalization))
+                if highlights.iter().all(|parts| parts.is_empty()) {
+                    None
+                } else {
+                    Some((buf, substitution.parts, highlights, only_capitalization))
+                }
             })
             .collect()
     }

diff --git a/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs b/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs
@@ -145,7 +145,9 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
                 (
                     "this function's return value is unnecessary".to_string(),
                     "remove the return type...".to_string(),
-                    snippet(cx, fn_decl.output.span(), "..").to_string(),
+                    // FIXME: we should instead get the span including the `->` and suggest an
+                    // empty string for this case.
+                    "()".to_string(),
                     "...and then remove returned values",
                 )
             } else {

diff --git a/src/tools/clippy/tests/ui/unnecessary_literal_unwrap.stderr b/src/tools/clippy/tests/ui/unnecessary_literal_unwrap.stderr
@@ -63,7 +63,7 @@ LL |     let _val = None::<()>.expect("this always happens");
 help: remove the `None` and `expect()`
    |
 LL |     let _val = panic!("this always happens");
-   |                ~~~~~~~                     ~
+   |                ~~~~~~~
 
 error: used `unwrap_or_default()` on `None` value
   --> tests/ui/unnecessary_literal_unwrap.rs:22:24
@@ -134,7 +134,7 @@ LL |     None::<()>.expect("this always happens");
 help: remove the `None` and `expect()`
    |
 LL |     panic!("this always happens");
-   |     ~~~~~~~                     ~
+   |     ~~~~~~~
 
 error: used `unwrap_or_default()` on `None` value
   --> tests/ui/unnecessary_literal_unwrap.rs:30:5

diff --git a/src/tools/clippy/tests/ui/unnecessary_wraps.stderr b/src/tools/clippy/tests/ui/unnecessary_wraps.stderr
@@ -118,8 +118,8 @@ LL | | }
    |
 help: remove the return type...
    |
-LL | fn issue_6640_1(a: bool, b: bool) -> Option<()> {
-   |                                      ~~~~~~~~~~
+LL | fn issue_6640_1(a: bool, b: bool) -> () {
+   |                                      ~~
 help: ...and then remove returned values
    |
 LL ~         return ;
@@ -145,8 +145,8 @@ LL | | }
    |
 help: remove the return type...
    |
-LL | fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
-   |                                      ~~~~~~~~~~~~~~~
+LL | fn issue_6640_2(a: bool, b: bool) -> () {
+   |                                      ~~
 help: ...and then remove returned values
    |
 LL ~         return ;

diff --git a/tests/ui/const-generics/generic_const_exprs/issue-105608.stderr b/tests/ui/const-generics/generic_const_exprs/issue-105608.stderr
@@ -4,10 +4,6 @@ error[E0282]: type annotations needed
 LL |     Combination::<0>.and::<_>().and::<_>();
    |                      ^^^ cannot infer type of the type parameter `M` declared on the method `and`
    |
-help: consider specifying the generic argument
-   |
-LL |     Combination::<0>.and::<_>().and::<_>();
-   |                         ~~~~~
 
 error: aborting due to 1 previous error
 

diff --git a/tests/ui/imports/issue-55884-2.stderr b/tests/ui/imports/issue-55884-2.stderr
@@ -24,10 +24,6 @@ note: ...and refers to the struct `ParseOptions` which is defined here
    |
 LL |     pub struct ParseOptions {}
    |     ^^^^^^^^^^^^^^^^^^^^^^^ you could import this directly
-help: import `ParseOptions` through the re-export
-   |
-LL | pub use parser::ParseOptions;
-   |         ~~~~~~~~~~~~~~~~~~~~
 
 error: aborting due to 1 previous error
 

diff --git a/tests/ui/lint/wide_pointer_comparisons.stderr b/tests/ui/lint/wide_pointer_comparisons.stderr
@@ -74,7 +74,7 @@ LL |     let _ = PartialEq::eq(&a, &b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |     let _ = std::ptr::addr_eq(a, b);
-   |             ~~~~~~~~~~~~~~~~~~ ~  ~
+   |             ~~~~~~~~~~~~~~~~~~ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:35:13
@@ -85,7 +85,7 @@ LL |     let _ = PartialEq::ne(&a, &b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |     let _ = !std::ptr::addr_eq(a, b);
-   |             ~~~~~~~~~~~~~~~~~~~ ~  ~
+   |             ~~~~~~~~~~~~~~~~~~~ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:37:13
@@ -96,7 +96,7 @@ LL |     let _ = a.eq(&b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |     let _ = std::ptr::addr_eq(a, b);
-   |             ++++++++++++++++++ ~  ~
+   |             ++++++++++++++++++ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:39:13
@@ -107,7 +107,7 @@ LL |     let _ = a.ne(&b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |     let _ = !std::ptr::addr_eq(a, b);
-   |             +++++++++++++++++++ ~  ~
+   |             +++++++++++++++++++ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:41:13
@@ -283,7 +283,7 @@ LL |         let _ = PartialEq::eq(a, b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = std::ptr::addr_eq(*a, *b);
-   |                 ~~~~~~~~~~~~~~~~~~~ ~~~ ~
+   |                 ~~~~~~~~~~~~~~~~~~~ ~~~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:85:17
@@ -294,7 +294,7 @@ LL |         let _ = PartialEq::ne(a, b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = !std::ptr::addr_eq(*a, *b);
-   |                 ~~~~~~~~~~~~~~~~~~~~ ~~~ ~
+   |                 ~~~~~~~~~~~~~~~~~~~~ ~~~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:87:17
@@ -305,7 +305,7 @@ LL |         let _ = PartialEq::eq(&a, &b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = std::ptr::addr_eq(*a, *b);
-   |                 ~~~~~~~~~~~~~~~~~~~ ~~~ ~
+   |                 ~~~~~~~~~~~~~~~~~~~ ~~~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:89:17
@@ -316,7 +316,7 @@ LL |         let _ = PartialEq::ne(&a, &b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = !std::ptr::addr_eq(*a, *b);
-   |                 ~~~~~~~~~~~~~~~~~~~~ ~~~ ~
+   |                 ~~~~~~~~~~~~~~~~~~~~ ~~~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:91:17
@@ -327,7 +327,7 @@ LL |         let _ = a.eq(b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = std::ptr::addr_eq(*a, *b);
-   |                 +++++++++++++++++++ ~~~ ~
+   |                 +++++++++++++++++++ ~~~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:93:17
@@ -338,7 +338,7 @@ LL |         let _ = a.ne(b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = !std::ptr::addr_eq(*a, *b);
-   |                 ++++++++++++++++++++ ~~~ ~
+   |                 ++++++++++++++++++++ ~~~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:95:17
@@ -519,11 +519,11 @@ LL |         let _ = PartialEq::eq(&a, &b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = std::ptr::addr_eq(a, b);
-   |                 ~~~~~~~~~~~~~~~~~~ ~  ~
+   |                 ~~~~~~~~~~~~~~~~~~ ~
 help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
 LL |         let _ = std::ptr::eq(a, b);
-   |                 ~~~~~~~~~~~~~ ~  ~
+   |                 ~~~~~~~~~~~~~ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:133:17
@@ -534,11 +534,11 @@ LL |         let _ = PartialEq::ne(&a, &b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = !std::ptr::addr_eq(a, b);
-   |                 ~~~~~~~~~~~~~~~~~~~ ~  ~
+   |                 ~~~~~~~~~~~~~~~~~~~ ~
 help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
 LL |         let _ = !std::ptr::eq(a, b);
-   |                 ~~~~~~~~~~~~~~ ~  ~
+   |                 ~~~~~~~~~~~~~~ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:135:17
@@ -549,11 +549,11 @@ LL |         let _ = a.eq(&b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = std::ptr::addr_eq(a, b);
-   |                 ++++++++++++++++++ ~  ~
+   |                 ++++++++++++++++++ ~
 help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
 LL |         let _ = std::ptr::eq(a, b);
-   |                 +++++++++++++ ~  ~
+   |                 +++++++++++++ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:137:17
@@ -564,11 +564,11 @@ LL |         let _ = a.ne(&b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         let _ = !std::ptr::addr_eq(a, b);
-   |                 +++++++++++++++++++ ~  ~
+   |                 +++++++++++++++++++ ~
 help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
 LL |         let _ = !std::ptr::eq(a, b);
-   |                 ++++++++++++++ ~  ~
+   |                 ++++++++++++++ ~
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:142:9
@@ -594,7 +594,7 @@ LL |         cmp!(a, b);
 help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
 LL |         cmp!(std::ptr::addr_eq(a, b));
-   |              ++++++++++++++++++ ~  +
+   |              ++++++++++++++++++    +
 
 warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
   --> $DIR/wide_pointer_comparisons.rs:159:39

diff --git a/tests/ui/privacy/issue-75907.stderr b/tests/ui/privacy/issue-75907.stderr
@@ -14,7 +14,7 @@ LL |     let Bar(x, y, Foo(z)) = make_bar();
 help: consider making the fields publicly accessible
    |
 LL |     pub(crate) struct Bar(pub u8, pub u8, pub Foo);
-   |                           ~~~     ~~~     +++
+   |                                   ~~~     +++
 
 error[E0532]: cannot match against a tuple struct which contains private fields
   --> $DIR/issue-75907.rs:15:19