Auto merge of rust-lang#13567 - y21:span_debug_assertions, r=flip1995

Add debug assertions for empty replacements and overlapping spans

rustc has debug assertions [^1] [^2] that check that a substitution doesn't have an empty suggestion string and an empty span at the same time, as well as that spans in multipart suggestions don't overlap.
However, since we link to the rustc-dev distributed compiler, these debug assertions are always disabled and so we never actually run them.

This leads to the problem that the debug ICE is not necessarily caught in the PR and only triggered in the rust repo sync, and in one of the last syncs this was a blocker and delayed the sync by several weeks because the fix was not obvious.

So this PR essentially copies the checks over and runs them in clippy debug builds as well, so that we can catch these errors in PRs directly.

-----
As for the second commit, this also *did* cause an ICE in a sync before and was fixed in the sync PR (see rust-lang#120345 (comment)), but it seems like that commit didn't make it back into the clippy repo (cc `@flip1995),` so the fixed code is in the rust repo but not in the clippy repo.

changelog: none

[^1]: https://doc.rust-lang.org/1.82.0/nightly-rustc/src/rustc_errors/diagnostic.rs.html#1019
[^2]: https://doc.rust-lang.org/1.82.0/nightly-rustc/src/rustc_errors/diagnostic.rs.html#932

Author: bors

clippy_lints/src/from_over_into.rs

@@ -181,9 +181,6 @@ fn convert_to_from(
     let from = self_ty.span.get_source_text(cx)?;
     let into = target_ty.span.get_source_text(cx)?;
 
-    let return_type = matches!(sig.decl.output, FnRetTy::Return(_))
-        .then_some(String::from("Self"))
-        .unwrap_or_default();
     let mut suggestions = vec![
         // impl Into<T> for U  ->  impl From<T> for U
         //      ~~~~                    ~~~~
@@ -200,10 +197,13 @@ fn convert_to_from(
         // fn into([mut] self) -> T  ->  fn into([mut] v: T) -> T
         //               ~~~~                          ~~~~
         (self_ident.span, format!("val: {from}")),
+    ];
+
+    if let FnRetTy::Return(_) = sig.decl.output {
         // fn into(self) -> T  ->  fn into(self) -> Self
         //                  ~                       ~~~~
-        (sig.decl.output.span(), return_type),
-    ];
+        suggestions.push((sig.decl.output.span(), String::from("Self")));
+    }
 
     let mut finder = SelfFinder {
         cx,

clippy_lints/src/semicolon_block.rs

@@ -101,17 +101,21 @@ impl SemicolonBlock {
         );
     }
 
-    fn semicolon_outside_block(
-        &self,
-        cx: &LateContext<'_>,
-        block: &Block<'_>,
-        tail_stmt_expr: &Expr<'_>,
-        semi_span: Span,
-    ) {
+    fn semicolon_outside_block(&self, cx: &LateContext<'_>, block: &Block<'_>, tail_stmt_expr: &Expr<'_>) {
         let insert_span = block.span.with_lo(block.span.hi());
-        // account for macro calls
-        let semi_span = cx.sess().source_map().stmt_span(semi_span, block.span);
-        let remove_span = semi_span.with_lo(tail_stmt_expr.span.source_callsite().hi());
+
+        // For macro call semicolon statements (`mac!();`), the statement's span does not actually
+        // include the semicolon itself, so use `mac_call_stmt_semi_span`, which finds the semicolon
+        // based on a source snippet.
+        // (Does not use `stmt_span` as that requires `.from_expansion()` to return true,
+        // which is not the case for e.g. `line!();` and `asm!();`)
+        let Some(remove_span) = cx
+            .sess()
+            .source_map()
+            .mac_call_stmt_semi_span(tail_stmt_expr.span.source_callsite())
+        else {
+            return;
+        };
 
         if self.semicolon_outside_block_ignore_multiline && get_line(cx, remove_span) != get_line(cx, insert_span) {
             return;
@@ -150,13 +154,12 @@ impl LateLintPass<'_> for SemicolonBlock {
                 };
                 let &Stmt {
                     kind: StmtKind::Semi(expr),
-                    span,
                     ..
                 } = stmt
                 else {
                     return;
                 };
-                self.semicolon_outside_block(cx, block, expr, span);
+                self.semicolon_outside_block(cx, block, expr);
             },
             StmtKind::Semi(Expr {
                 kind: ExprKind::Block(block @ Block { expr: Some(tail), .. }, _),

clippy_utils/src/diagnostics.rs

@@ -8,7 +8,9 @@
 //! Thank you!
 //! ~The `INTERNAL_METADATA_COLLECTOR` lint
 
-use rustc_errors::{Applicability, Diag, DiagMessage, MultiSpan, SubdiagMessage};
+use rustc_errors::{
+    Applicability, Diag, DiagMessage, EmissionGuarantee, MultiSpan, SubdiagMessage, SubstitutionPart, Suggestions,
+};
 use rustc_hir::HirId;
 use rustc_lint::{LateContext, Lint, LintContext};
 use rustc_span::Span;
@@ -28,6 +30,42 @@ fn docs_link(diag: &mut Diag<'_, ()>, lint: &'static Lint) {
     }
 }
 
+/// Makes sure that a diagnostic is well formed.
+///
+/// rustc debug asserts a few properties about spans,
+/// but the clippy repo uses a distributed rustc build with debug assertions disabled,
+/// so this has historically led to problems during subtree syncs where those debug assertions
+/// only started triggered there.
+///
+/// This function makes sure we also validate them in debug clippy builds.
+fn validate_diag(diag: &Diag<'_, impl EmissionGuarantee>) {
+    let suggestions = match &diag.suggestions {
+        Suggestions::Enabled(suggs) => &**suggs,
+        Suggestions::Sealed(suggs) => &**suggs,
+        Suggestions::Disabled => return,
+    };
+
+    for substitution in suggestions.iter().flat_map(|s| &s.substitutions) {
+        assert_eq!(
+            substitution
+                .parts
+                .iter()
+                .find(|SubstitutionPart { snippet, span }| snippet.is_empty() && span.is_empty()),
+            None,
+            "span must not be empty and have no suggestion"
+        );
+
+        assert_eq!(
+            substitution
+                .parts
+                .array_windows()
+                .find(|[a, b]| a.span.overlaps(b.span)),
+            None,
+            "suggestion must not have overlapping parts"
+        );
+    }
+}
+
 /// Emit a basic lint message with a `msg` and a `span`.
 ///
 /// This is the most primitive of our lint emission methods and can
@@ -64,6 +102,9 @@ pub fn span_lint<T: LintContext>(cx: &T, lint: &'static Lint, sp: impl Into<Mult
     cx.span_lint(lint, sp, |diag| {
         diag.primary_message(msg);
         docs_link(diag, lint);
+
+        #[cfg(debug_assertions)]
+        validate_diag(diag);
     });
 }
 
@@ -118,6 +159,9 @@ pub fn span_lint_and_help<T: LintContext>(
             diag.help(help.into());
         }
         docs_link(diag, lint);
+
+        #[cfg(debug_assertions)]
+        validate_diag(diag);
     });
 }
 
@@ -175,6 +219,9 @@ pub fn span_lint_and_note<T: LintContext>(
             diag.note(note.into());
         }
         docs_link(diag, lint);
+
+        #[cfg(debug_assertions)]
+        validate_diag(diag);
     });
 }
 
@@ -208,6 +255,9 @@ where
         diag.primary_message(msg);
         f(diag);
         docs_link(diag, lint);
+
+        #[cfg(debug_assertions)]
+        validate_diag(diag);
     });
 }
 
@@ -240,6 +290,9 @@ pub fn span_lint_hir(cx: &LateContext<'_>, lint: &'static Lint, hir_id: HirId, s
     cx.tcx.node_span_lint(lint, hir_id, sp, |diag| {
         diag.primary_message(msg);
         docs_link(diag, lint);
+
+        #[cfg(debug_assertions)]
+        validate_diag(diag);
     });
 }
 
@@ -280,6 +333,9 @@ pub fn span_lint_hir_and_then(
         diag.primary_message(msg);
         f(diag);
         docs_link(diag, lint);
+
+        #[cfg(debug_assertions)]
+        validate_diag(diag);
     });
 }
 
@@ -316,7 +372,7 @@ pub fn span_lint_hir_and_then(
 ///     |
 ///     = note: `-D fold-any` implied by `-D warnings`
 /// ```
-#[expect(clippy::collapsible_span_lint_calls)]
+#[cfg_attr(not(debug_assertions), expect(clippy::collapsible_span_lint_calls))]
 pub fn span_lint_and_sugg<T: LintContext>(
     cx: &T,
     lint: &'static Lint,
@@ -328,5 +384,8 @@ pub fn span_lint_and_sugg<T: LintContext>(
 ) {
     span_lint_and_then(cx, lint, sp, msg.into(), |diag| {
         diag.span_suggestion(sp, help.into(), sugg, applicability);
+
+        #[cfg(debug_assertions)]
+        validate_diag(diag);
     });
 }

clippy_utils/src/lib.rs

@@ -9,6 +9,7 @@
 #![feature(rustc_private)]
 #![feature(assert_matches)]
 #![feature(unwrap_infallible)]
+#![feature(array_windows)]
 #![recursion_limit = "512"]
 #![allow(
     clippy::missing_errors_doc,

tests/ui/semicolon_outside_block.fixed

@@ -80,5 +80,13 @@ fn main() {
 
     { unit_fn_block(); };
 
+    unsafe {
+        std::arch::asm!("")
+    };
+
+    {
+        line!()
+    };
+
     unit_fn_block()
 }

tests/ui/semicolon_outside_block.rs

@@ -80,5 +80,13 @@ fn main() {
 
     { unit_fn_block(); };
 
+    unsafe {
+        std::arch::asm!("");
+    }
+
+    {
+        line!();
+    }
+
     unit_fn_block()
 }

tests/ui/semicolon_outside_block.stderr

@@ -51,5 +51,33 @@ LL -     { m!(()); }
 LL +     { m!(()) };
    |
 
-error: aborting due to 4 previous errors
+error: consider moving the `;` outside the block for consistent formatting
+  --> tests/ui/semicolon_outside_block.rs:83:5
+   |
+LL | /     unsafe {
+LL | |         std::arch::asm!("");
+LL | |     }
+   | |_____^
+   |
+help: put the `;` here
+   |
+LL ~         std::arch::asm!("")
+LL ~     };
+   |
+
+error: consider moving the `;` outside the block for consistent formatting
+  --> tests/ui/semicolon_outside_block.rs:87:5
+   |
+LL | /     {
+LL | |         line!();
+LL | |     }
+   | |_____^
+   |
+help: put the `;` here
+   |
+LL ~         line!()
+LL ~     };
+   |
+
+error: aborting due to 6 previous errors