Do not underline suggestions for code that is already there

When a suggestion part is for already present code, do not highlight it. If after that there are no highlights left, do not show the suggestion at all. Fix clippy lint suggestion incorrectly treated as `span_help`.
rust-lang · Aug 1, 2024 · 8ce8c42 · 8ce8c42
1 parent e60ebb2
commit 8ce8c42
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