From a809ec96f3cf5f5a73db24129392e64c8285bda8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Mon, 8 Apr 2024 21:18:51 +0000 Subject: [PATCH 1/3] test: check that `?` suggestion has local span This can cause rustfix to crash because the `?` suggestion previously could point into non-local spans, such as into the stdlib. --- .../question-mark-operator-suggestion-span.rs | 22 +++++++++++++ ...stion-mark-operator-suggestion-span.stderr | 31 +++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 tests/ui/typeck/question-mark-operator-suggestion-span.rs create mode 100644 tests/ui/typeck/question-mark-operator-suggestion-span.stderr diff --git a/tests/ui/typeck/question-mark-operator-suggestion-span.rs b/tests/ui/typeck/question-mark-operator-suggestion-span.rs new file mode 100644 index 0000000000000..7aea6e63dd1f2 --- /dev/null +++ b/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: &mut W) -> std::fmt::Result { + if true { + writeln!(w, "`;?` here ->")?; + } else { + writeln!(w, "but not here") + //~^ ERROR mismatched types + } + Ok(()) +} diff --git a/tests/ui/typeck/question-mark-operator-suggestion-span.stderr b/tests/ui/typeck/question-mark-operator-suggestion-span.stderr new file mode 100644 index 0000000000000..089b3bcd1988a --- /dev/null +++ b/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`. From 4606485d0c36676f08a61c524dbc2ee05bbfb049 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Mon, 8 Apr 2024 21:42:06 +0000 Subject: [PATCH 2/3] typeck: fix `?` operator suggestion span --- .../src/fn_ctxt/suggestions.rs | 15 ++----- compiler/rustc_span/src/lib.rs | 39 +++++++++++++++++++ 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index 442bfd75746ee..4a956595f1521 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -1291,12 +1291,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ty::TraitRef::new(self.tcx, into_def_id, [expr_ty, expected_ty]), )) { - let mut span = expr.span; - while expr.span.eq_ctxt(span) - && let Some(parent_callsite) = span.parent_callsite() - { - span = parent_callsite; - } + let span = expr.span.find_oldest_ancestor_in_same_ctxt(); let mut sugg = if expr.precedence().order() >= PREC_POSTFIX { vec![(span.shrink_to_hi(), ".into()".to_owned())] @@ -1901,12 +1896,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { None => sugg.to_string(), }; - err.span_suggestion_verbose( - expr.span.shrink_to_hi(), - msg, - sugg, - Applicability::HasPlaceholders, - ); + let span = expr.span.find_oldest_ancestor_in_same_ctxt(); + err.span_suggestion_verbose(span.shrink_to_hi(), msg, sugg, Applicability::HasPlaceholders); return true; } diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs index 7ce879807cae1..c1e1175b4bd6b 100644 --- a/compiler/rustc_span/src/lib.rs +++ b/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> { + /// 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() From 420e3f1d5cf8e156d6f4ae636ce5bbaa6b7c9c70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Mon, 8 Apr 2024 23:17:18 +0000 Subject: [PATCH 3/3] test: avoid running rustfix on leaked writeln macro internals test This test contains conflicting MaybeIncorrect suggestions which will cause the fixed file to not compile. --- ...12007-leaked-writeln-macro-internals.fixed | 37 ------------------- ...e-112007-leaked-writeln-macro-internals.rs | 18 ++++++--- ...2007-leaked-writeln-macro-internals.stderr | 12 +++++- 3 files changed, 23 insertions(+), 44 deletions(-) delete mode 100644 tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.fixed diff --git a/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.fixed b/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.fixed deleted file mode 100644 index dcb256de18f17..0000000000000 --- a/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.fixed +++ /dev/null @@ -1,37 +0,0 @@ -//@ run-rustfix -#![allow(dead_code)] - -// https://github.com/rust-lang/rust/issues/112007 -fn bug_report(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: &mut W) -> std::fmt::Result { - if true { - writeln!(w, "`;?` here ->")?; - } else { - return baz!(w); - }; - Ok(()) -} - -fn main() {} diff --git a/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs b/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs index 58cd6cbf20c57..7ec9f0d4cdb9c 100644 --- a/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs +++ b/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs @@ -1,8 +1,16 @@ -//@ run-rustfix -#![allow(dead_code)] +// Check that we don't leak stdlib implementation details through suggestions. +// 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: &mut W) -> std::fmt::Result { +pub fn bug_report(w: &mut W) -> std::fmt::Result { if true { writeln!(w, "`;?` here ->")?; } else { @@ -25,7 +33,7 @@ macro_rules! bar { } } -fn foo(w: &mut W) -> std::fmt::Result { +pub fn foo(w: &mut W) -> std::fmt::Result { if true { writeln!(w, "`;?` here ->")?; } else { @@ -34,4 +42,4 @@ fn foo(w: &mut W) -> std::fmt::Result { Ok(()) } -fn main() {} +pub fn main() {} diff --git a/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.stderr b/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.stderr index df2e06e8f3b76..889d2c94d0cfb 100644 --- a/tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.stderr +++ b/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