Rollup merge of #123654 - jieyouxu:question-mark-span, r=Nadrieril

typeck: fix `?` suggestion span

Noticed in <https://github.com/rust-lang/rust/pull/112043#issuecomment-2043565292>, if the

```
use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller
```

suggestion is applied to a macro that comes from a non-local crate (e.g. the stdlib), the suggestion span can become non-local, which will cause newer rustfix versions to fail.

This PR tries to remedy the problem by recursively probing ancestors of the expression span, trying to identify the most ancestor span that is (1) still local, and (2) still shares the same syntax context as the expression.

This is the same strategy used in https://github.com/rust-lang/rust/pull/112043.

The test unfortunately cannot `//@ run-rustfix` because there are two conflicting MaybeIncorrect suggestions that when collectively applied, cause the fixed source file to become non-compilable.

Also avoid running `//@ run-rustfix` for `tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs` because that also contains conflicting suggestions.

cc `@ehuss` who noticed this. This question mark span fix + not running rustfix on the tests containing conflicting MaybeIncorrect suggestions should hopefully unblock rustfix from updating.

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

@@ -1287,12 +1287,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 ty::TraitRef::new(self.tcx, into_def_id, [expr_ty, expected_ty]),
             ))
         {
-            let mut span = expr.span;
+            let span = expr.span.find_oldest_ancestor_in_same_ctxt();
-            while expr.span.eq_ctxt(span)
-                && let Some(parent_callsite) = span.parent_callsite()
-            {
-                span = parent_callsite;
-            }
 
             let mut sugg = if expr.precedence().order() >= PREC_POSTFIX {
                 vec![(span.shrink_to_hi(), ".into()".to_owned())]
@@ -1897,12 +1892,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             None => sugg.to_string(),
         };
 
-        err.span_suggestion_verbose(
+        let span = expr.span.find_oldest_ancestor_in_same_ctxt();
-            expr.span.shrink_to_hi(),
+        err.span_suggestion_verbose(span.shrink_to_hi(), msg, sugg, Applicability::HasPlaceholders);
-            msg,
-            sugg,
-            Applicability::HasPlaceholders,
-        );
         return true;
     }
 

compiler/rustc_span/src/lib.rs

@@ -743,6 +743,45 @@ impl Span {
         Some(self)
     }
 
+    /// Recursively walk down the expansion ancestors to find the oldest ancestor span with the same
+    /// [`SyntaxContext`] the initial span.
+    ///
+    /// This method is suitable for peeling through *local* macro expansions to find the "innermost"
+    /// span that is still local and shares the same [`SyntaxContext`]. For example, given
+    ///
+    /// ```ignore (illustrative example, contains type error)
+    ///  macro_rules! outer {
+    ///      ($x: expr) => {
+    ///          inner!($x)
+    ///      }
+    ///  }
+    ///
+    ///  macro_rules! inner {
+    ///      ($x: expr) => {
+    ///          format!("error: {}", $x)
+    ///          //~^ ERROR mismatched types
+    ///      }
+    ///  }
+    ///
+    ///  fn bar(x: &str) -> Result<(), Box<dyn std::error::Error>> {
+    ///      Err(outer!(x))
+    ///  }
+    /// ```
+    ///
+    /// if provided the initial span of `outer!(x)` inside `bar`, this method will recurse
+    /// the parent callsites until we reach `format!("error: {}", $x)`, at which point it is the
+    /// oldest ancestor span that is both still local and shares the same [`SyntaxContext`] as the
+    /// initial span.
+    pub fn find_oldest_ancestor_in_same_ctxt(self) -> Span {
+        let mut cur = self;
+        while cur.eq_ctxt(self)
+            && let Some(parent_callsite) = cur.parent_callsite()
+        {
+            cur = parent_callsite;
+        }
+        cur
+    }
+
     /// Edition of the crate from which this span came.
     pub fn edition(self) -> edition::Edition {
         self.ctxt().edition()

tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.fixed

@@ -1,37 +0,0 @@
-//@ run-rustfix
-#![allow(dead_code)]
-
-// https://github.com/rust-lang/rust/issues/112007
-fn bug_report<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
-    if true {
-        writeln!(w, "`;?` here ->")?;
-    } else {
-        return writeln!(w, "but not here");
-        //~^ ERROR mismatched types
-    };
-    Ok(())
-}
-
-macro_rules! baz {
-    ($w: expr) => {
-        bar!($w)
-    }
-}
-
-macro_rules! bar {
-    ($w: expr) => {
-        writeln!($w, "but not here")
-        //~^ ERROR mismatched types
-    }
-}
-
-fn foo<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
-    if true {
-        writeln!(w, "`;?` here ->")?;
-    } else {
-        return baz!(w);
-    };
-    Ok(())
-}
-
-fn main() {}

tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs

@@ -1,8 +1,16 @@
-//@ run-rustfix
+// Check that we don't leak stdlib implementation details through suggestions.
-#![allow(dead_code)]
+// Also check that the suggestion provided tries as hard as it can to see through local macros.
+//
+// FIXME(jieyouxu): this test is NOT run-rustfix because this test contains conflicting
+// MaybeIncorrect suggestions:
+//
+// 1. `return ... ;`
+// 2. `?`
+//
+// when the suggestions are applied to the same file, it becomes uncompilable.
 
 // https://github.com/rust-lang/rust/issues/112007
-fn bug_report<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
+pub fn bug_report<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
     if true {
         writeln!(w, "`;?` here ->")?;
     } else {
@@ -25,7 +33,7 @@ macro_rules! bar {
     }
 }
 
-fn foo<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
+pub fn foo<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
     if true {
         writeln!(w, "`;?` here ->")?;
     } else {
@@ -34,4 +42,4 @@ fn foo<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
     Ok(())
 }
 
-fn main() {}
+pub fn main() {}

tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.stderr

@@ -1,5 +1,5 @@
 error[E0308]: mismatched types
-  --> $DIR/issue-112007-leaked-writeln-macro-internals.rs:9:9
+  --> $DIR/issue-112007-leaked-writeln-macro-internals.rs:17:9
    |
 LL | /     if true {
 LL | |         writeln!(w, "`;?` here ->")?;
@@ -21,9 +21,13 @@ help: you might have meant to return this value
    |
 LL |         return writeln!(w, "but not here");
    |         ++++++                            +
+help: use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller
+   |
+LL |         writeln!(w, "but not here")?
+   |                                    +
 
 error[E0308]: mismatched types
-  --> $DIR/issue-112007-leaked-writeln-macro-internals.rs:32:9
+  --> $DIR/issue-112007-leaked-writeln-macro-internals.rs:40:9
    |
 LL | /     if true {
 LL | |         writeln!(w, "`;?` here ->")?;
@@ -44,6 +48,10 @@ help: you might have meant to return this value
    |
 LL |         return baz!(w);
    |         ++++++        +
+help: use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller
+   |
+LL |         writeln!($w, "but not here")?
+   |                                     +
 
 error: aborting due to 2 previous errors
 

tests/ui/typeck/question-mark-operator-suggestion-span.rs

@@ -0,0 +1,22 @@
+// Check that we don't construct a span for `?` suggestions that point into non-local macros
+// like into the stdlib where the user has no control over.
+//
+// FIXME(jieyouxu): this test is currently NOT run-rustfix because there are conflicting
+// MaybeIncorrect suggestions:
+//
+// 1. adding `return ... ;`, and
+// 2. adding `?`.
+//
+// When rustfix puts those together, the fixed file now contains uncompilable code.
+
+#![crate_type = "lib"]
+
+pub fn bug_report<W: std::fmt::Write>(w: &mut W) -> std::fmt::Result {
+    if true {
+        writeln!(w, "`;?` here ->")?;
+    } else {
+        writeln!(w, "but not here")
+        //~^ ERROR mismatched types
+    }
+    Ok(())
+}

tests/ui/typeck/question-mark-operator-suggestion-span.stderr

@@ -0,0 +1,31 @@
+error[E0308]: mismatched types
+  --> $DIR/question-mark-operator-suggestion-span.rs:18:9
+   |
+LL | /     if true {
+LL | |         writeln!(w, "`;?` here ->")?;
+LL | |     } else {
+LL | |         writeln!(w, "but not here")
+   | |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `Result<(), Error>`
+LL | |
+LL | |     }
+   | |_____- expected this to be `()`
+   |
+   = note: expected unit type `()`
+                   found enum `Result<(), std::fmt::Error>`
+   = note: this error originates in the macro `writeln` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: consider using a semicolon here
+   |
+LL |     };
+   |      +
+help: you might have meant to return this value
+   |
+LL |         return writeln!(w, "but not here");
+   |         ++++++                            +
+help: use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller
+   |
+LL |         writeln!(w, "but not here")?
+   |                                    +
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0308`.