don't emit suggestions with overlapping spans and count omitted suggestions

Author: jdonszelmann

compiler/rustc_errors/src/emitter.rs

@@ -2038,20 +2038,31 @@ impl HumanEmitter {
         };
 
         // Render the replacements for each suggestion
-        let suggestions = suggestion.splice_lines(sm);
+        let (suggestions, num_omitted) = suggestion.splice_lines(sm);
         debug!(?suggestions);
 
+        let mut buffer = StyledBuffer::new();
+
+        if num_omitted.0 > 0 {
+            // in this case, do push that we omitted some
+            buffer.append(
+                0,
+                &format!("rendering error: omitted {} suggestion{} that failed to render, likely because of macro expansions", num_omitted.0, if num_omitted.0 > 1 {"s"} else {""}),
+                Style::Level(Level::Error),
+            );
+        }
+
         if suggestions.is_empty() {
             // 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.
+
+            emit_to_destination(&buffer.render(), &Level::Note, &mut self.dst, self.short_message)?;
             return Ok(());
         }
 
-        let mut buffer = StyledBuffer::new();
-
         // Render the suggestion message
         buffer.append(0, level.to_str(), Style::Level(*level));
         buffer.append(0, ": ", Style::HeaderMsg);

compiler/rustc_errors/src/json.rs

@@ -24,6 +24,7 @@ use rustc_span::Span;
 use rustc_span::hygiene::ExpnData;
 use rustc_span::source_map::{FilePathMapping, SourceMap};
 use serde::Serialize;
+use tracing::debug;
 
 use crate::diagnostic::IsLint;
 use crate::emitter::{
@@ -543,6 +544,21 @@ impl DiagnosticSpan {
         suggestion
             .substitutions
             .iter()
+            .filter_map(|substitution| {
+                let mut parts = substitution.parts.clone();
+                // check that all spans are distjoint, otherwise rustfix doesn't know what to do.
+                // suggestions that don't have that have a hint about omitted suggestions in them
+                parts.sort_by_key(|part| part.span.lo());
+                // Verify the assumption that all spans are disjoint
+                if parts.array_windows().find(|[a, b]| a.span.overlaps(b.span)).is_some() {
+                    debug!(
+                        "from_suggestion: suggestion contains an invalid span: {substitution:?}"
+                    );
+                    None
+                } else {
+                    Some(substitution)
+                }
+            })
             .flat_map(|substitution| {
                 substitution.parts.iter().map(move |suggestion_inner| {
                     let span_label =

compiler/rustc_errors/src/lib.rs

@@ -322,20 +322,27 @@ fn as_substr<'a>(original: &'a str, suggestion: &'a str) -> Option<(usize, &'a s
     }
 }
 
+/// Represents suggestions that were emitted because their span was ambiguous.
+pub(crate) struct SuggestionsOmitted(usize);
+
 impl CodeSuggestion {
     /// Returns the assembled code suggestions, whether they should be shown with an underline
     /// and whether the substitution only differs in capitalization.
     pub(crate) fn splice_lines(
         &self,
         sm: &SourceMap,
-    ) -> Vec<(String, Vec<TrimmedSubstitutionPart>, Vec<Vec<SubstitutionHighlight>>, ConfusionType)>
-    {
+    ) -> (
+        Vec<(String, Vec<TrimmedSubstitutionPart>, Vec<Vec<SubstitutionHighlight>>, ConfusionType)>,
+        SuggestionsOmitted,
+    ) {
         // For the `Vec<Vec<SubstitutionHighlight>>` value, the first level of the vector
         // corresponds to the output snippet's lines, while the second level corresponds to the
         // substrings within that line that should be highlighted.
 
         use rustc_span::{CharPos, Pos};
 
+        let num_omitted = Cell::new(0);
+
         /// Extracts a substring from the provided `line_opt` based on the specified low and high
         /// indices, appends it to the given buffer `buf`, and returns the count of newline
         /// characters in the substring for accurate highlighting. If `line_opt` is `None`, a
@@ -384,13 +391,15 @@ impl CodeSuggestion {
 
         assert!(!self.substitutions.is_empty());
 
-        self.substitutions
+        let res = self
+            .substitutions
             .iter()
             .filter(|subst| {
                 // Suggestions coming from macros can have malformed spans. This is a heavy
                 // handed approach to avoid ICEs by ignoring the suggestion outright.
                 let invalid = subst.parts.iter().any(|item| sm.is_valid_span(item.span).is_err());
                 if invalid {
+                    num_omitted.update(|i| i + 1);
                     debug!("splice_lines: suggestion contains an invalid span: {:?}", subst);
                 }
                 !invalid
@@ -400,6 +409,21 @@ impl CodeSuggestion {
                 // Assumption: all spans are in the same file, and all spans
                 // are disjoint. Sort in ascending order.
                 substitution.parts.sort_by_key(|part| part.span.lo());
+                if substitution
+                    .parts
+                    .array_windows()
+                    .find(|[a, b]| a.span.overlaps(b.span))
+                    .is_some()
+                {
+                    debug!("splice_lines: suggestion contains an invalid span: {substitution:?}");
+                    num_omitted.update(|i| i + 1);
+                    return None;
+                }
+
+                // 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.
+                substitution.parts.retain(|p| is_different(sm, &p.snippet, p.span));
 
                 // Find the bounding span.
                 let lo = substitution.parts.iter().map(|part| part.span.lo()).min()?;
@@ -541,7 +565,9 @@ impl CodeSuggestion {
                     Some((buf, trimmed_parts, highlights, confusion_type))
                 }
             })
-            .collect()
+            .collect();
+
+        (res, SuggestionsOmitted(num_omitted.get()))
     }
 }
 

tests/ui/parser/attribute/invalid-delimeter-ice-146808.fixed

@@ -0,0 +1,11 @@
+// regression test for #146808
+//@ proc-macro: all_spans_same.rs
+//@ run-rustfix
+//@ rustfix-only-machine-applicable
+
+extern crate all_spans_same;
+
+#[all_spans_same::all_spans_same]
+//~^ ERROR wrong meta list delimiters
+#[allow{}]
+fn main() {}

tests/ui/parser/attribute/invalid-delimeter-ice-146808.rs

@@ -1,7 +1,11 @@
 // regression test for #146808
 //@ proc-macro: all_spans_same.rs
+//@ run-rustfix
+//@ rustfix-only-machine-applicable
+
 extern crate all_spans_same;
 
 #[all_spans_same::all_spans_same]
+//~^ ERROR wrong meta list delimiters
 #[allow{}]
 fn main() {}

tests/ui/parser/attribute/invalid-delimeter-ice-146808.stderr

@@ -0,0 +1,11 @@
+error: wrong meta list delimiters
+  --> $DIR/invalid-delimeter-ice-146808.rs:8:1
+   |
+LL | #[all_spans_same::all_spans_same]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this error originates in the attribute macro `all_spans_same::all_spans_same` (in Nightly builds, run with -Z macro-backtrace for more info)
+rendering error: omitted 1 suggestion that failed to render, likely because of macro expansions
+
+error: aborting due to 1 previous error
+