Omit suggestions when spans are invalid

compiler/rustc_errors/src/emitter.rs

@@ -2042,20 +2042,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::hygiene::ExpnData;
 use rustc_span::source_map::{FilePathMapping, SourceMap};
 use serde::Serialize;
 use termcolor::{ColorSpec, WriteColor};
+use tracing::debug;
 
 use crate::diagnostic::IsLint;
 use crate::emitter::{
@@ -554,6 +555,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

@@ -304,19 +304,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<SubstitutionPart>, Vec<Vec<SubstitutionHighlight>>, ConfusionType)> {
+    ) -> (
+        Vec<(String, Vec<SubstitutionPart>, 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
@@ -365,13 +373,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
@@ -382,11 +392,16 @@ impl CodeSuggestion {
                 // are disjoint. Sort in ascending order.
                 substitution.parts.sort_by_key(|part| part.span.lo());
                 // Verify the assumption that all spans are disjoint
-                assert_eq!(
-                    substitution.parts.array_windows().find(|[a, b]| a.span.overlaps(b.span)),
-                    None,
-                    "all spans must be disjoint",
-                );
+                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
@@ -524,7 +539,9 @@ impl CodeSuggestion {
                     Some((buf, substitution.parts, highlights, confusion_type))
                 }
             })
-            .collect()
+            .collect();
+
+        (res, SuggestionsOmitted(num_omitted.get()))
     }
 }
 

src/tools/compiletest/src/directives.rs

@@ -181,6 +181,8 @@ pub struct TestProps {
     pub run_rustfix: bool,
     // If true, `rustfix` will only apply `MachineApplicable` suggestions.
     pub rustfix_only_machine_applicable: bool,
+    // If true, don't test the fixed program. It's still broken in some way.
+    pub rustfix_dont_test_fixed: bool,
     pub assembly_output: Option<String>,
     // If true, the test is expected to ICE
     pub should_ice: bool,
@@ -245,6 +247,7 @@ mod directives {
     pub const DONT_CHECK_FAILURE_STATUS: &'static str = "dont-check-failure-status";
     pub const RUN_RUSTFIX: &'static str = "run-rustfix";
     pub const RUSTFIX_ONLY_MACHINE_APPLICABLE: &'static str = "rustfix-only-machine-applicable";
+    pub const RUSTFIX_DONT_TEST_FIXED: &'static str = "rustfix-dont-test-fixed";
     pub const ASSEMBLY_OUTPUT: &'static str = "assembly-output";
     pub const STDERR_PER_BITWIDTH: &'static str = "stderr-per-bitwidth";
     pub const INCREMENTAL: &'static str = "incremental";
@@ -304,6 +307,7 @@ impl TestProps {
             dont_check_failure_status: false,
             run_rustfix: false,
             rustfix_only_machine_applicable: false,
+            rustfix_dont_test_fixed: false,
             assembly_output: None,
             should_ice: false,
             stderr_per_bitwidth: false,
@@ -573,6 +577,11 @@ impl TestProps {
                         RUSTFIX_ONLY_MACHINE_APPLICABLE,
                         &mut self.rustfix_only_machine_applicable,
                     );
+                    config.set_name_directive(
+                        ln,
+                        RUSTFIX_DONT_TEST_FIXED,
+                        &mut self.rustfix_dont_test_fixed,
+                    );
                     config.set_name_value_directive(
                         ln,
                         ASSEMBLY_OUTPUT,

src/tools/compiletest/src/directives/directive_names.rs

@@ -259,6 +259,7 @@ pub(crate) const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
     "run-pass",
     "run-rustfix",
     "rustc-env",
+    "rustfix-dont-test-fixed",
     "rustfix-only-machine-applicable",
     "should-fail",
     "should-ice",

src/tools/compiletest/src/runtest/ui.rs

@@ -220,7 +220,10 @@ impl TestCx<'_> {
         self.check_all_error_patterns(&output_to_check, pattern_proc_res);
         self.check_forbid_output(&output_to_check, pattern_proc_res);
 
-        if self.props.run_rustfix && self.config.compare_mode.is_none() {
+        if self.props.run_rustfix
+            && self.config.compare_mode.is_none()
+            && !self.props.rustfix_dont_test_fixed
+        {
             // And finally, compile the fixed code and make sure it both
             // succeeds and has no diagnostics.
             let mut rustc = self.make_compile_args(

tests/ui/parser/attribute/auxiliary/all_spans_same.rs

@@ -0,0 +1,26 @@
+extern crate proc_macro;
+use proc_macro::*;
+
+fn spans_callsite(ts: TokenStream) -> TokenStream {
+    let mut new_ts = TokenStream::new();
+
+    for i in ts {
+        let new_token = i.clone();
+        match new_token {
+            TokenTree::Group(g) => {
+                new_ts.extend([Group::new(g.delimiter(), spans_callsite(g.stream()))])
+            }
+            mut other => {
+                other.set_span(Span::call_site());
+                new_ts.extend([other]);
+            }
+        }
+    }
+
+    new_ts
+}
+
+#[proc_macro_attribute]
+pub fn all_spans_same(_: TokenStream, ts: TokenStream) -> TokenStream {
+    spans_callsite(ts)
+}

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

@@ -0,0 +1,13 @@
+// regression test for #146808
+//@ proc-macro: all_spans_same.rs
+//@ run-rustfix
+// the fixed program is still broken, but rustfix didn't crash!
+// that's what we want to test here.
+//@ rustfix-dont-test-fixed
+
+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

@@ -0,0 +1,13 @@
+// regression test for #146808
+//@ proc-macro: all_spans_same.rs
+//@ run-rustfix
+// the fixed program is still broken, but rustfix didn't crash!
+// that's what we want to test here.
+//@ rustfix-dont-test-fixed
+
+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:10: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
+