Auto merge of #139154 - jhpratt:rollup-rv8f915, r=jhpratt

Rollup of 5 pull requests

Successful merges:

 - #139044 (bootstrap: Avoid cloning `change-id` list)
 - #139111 (Properly document FakeReads)
 - #139122 (Remove attribute `#[rustc_error]`)
 - #139132 (Improve hir_pretty for struct expressions.)
 - #139141 (Switch some rustc_on_unimplemented uses to diagnostic::on_unimplemented)

r? `@ghost`
`@rustbot` modify labels: rollup

Author: bors

Diff for: compiler/rustc_feature/src/builtin_attrs.rs

@@ -1087,9 +1087,9 @@ pub static BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
         WarnFollowing, EncodeCrossCrate::No
     ),
     rustc_attr!(
-        TEST, rustc_error, Normal,
-        template!(Word, List: "delayed_bug_from_inside_query"),
-        WarnFollowingWordOnly, EncodeCrossCrate::Yes
+        TEST, rustc_delayed_bug_from_inside_query, Normal,
+        template!(Word),
+        WarnFollowing, EncodeCrossCrate::No
     ),
     rustc_attr!(
         TEST, rustc_dump_user_args, Normal, template!(Word),

Diff for: compiler/rustc_hir_pretty/src/lib.rs

@@ -1193,7 +1193,8 @@ impl<'a> State<'a> {
         wth: hir::StructTailExpr<'_>,
     ) {
         self.print_qpath(qpath, true);
-        self.word("{");
+        self.nbsp();
+        self.word_space("{");
         self.commasep_cmnt(Consistent, fields, |s, field| s.print_expr_field(field), |f| f.span);
         match wth {
             hir::StructTailExpr::Base(expr) => {
@@ -1215,20 +1216,13 @@ impl<'a> State<'a> {
                 self.word("..");
                 self.end();
             }
-            hir::StructTailExpr::None => {
-                if !fields.is_empty() {
-                    self.word(",");
-                }
-            }
+            hir::StructTailExpr::None => {}
         }
-
+        self.space();
         self.word("}");
     }
 
     fn print_expr_field(&mut self, field: &hir::ExprField<'_>) {
-        if self.attrs(field.hir_id).is_empty() {
-            self.space();
-        }
         self.cbox(INDENT_UNIT);
         self.print_attrs_as_outer(self.attrs(field.hir_id));
         if !field.is_shorthand {

Diff for: compiler/rustc_interface/messages.ftl

@@ -50,11 +50,5 @@ interface_out_dir_error =
 interface_proc_macro_crate_panic_abort =
     building proc macro crate with `panic=abort` may crash the compiler should the proc-macro panic
 
-interface_rustc_error_fatal =
-    fatal error triggered by #[rustc_error]
-
-interface_rustc_error_unexpected_annotation =
-    unexpected annotation used with `#[rustc_error(...)]`!
-
 interface_temps_dir_error =
     failed to find or create the directory specified by `--temps-dir`

Diff for: compiler/rustc_interface/src/errors.rs

@@ -73,20 +73,6 @@ pub struct TempsDirError;
 #[diag(interface_out_dir_error)]
 pub struct OutDirError;
 
-#[derive(Diagnostic)]
-#[diag(interface_rustc_error_fatal)]
-pub struct RustcErrorFatal {
-    #[primary_span]
-    pub span: Span,
-}
-
-#[derive(Diagnostic)]
-#[diag(interface_rustc_error_unexpected_annotation)]
-pub struct RustcErrorUnexpectedAnnotation {
-    #[primary_span]
-    pub span: Span,
-}
-
 #[derive(Diagnostic)]
 #[diag(interface_failed_writing_file)]
 pub struct FailedWritingFile<'a> {

Diff for: compiler/rustc_interface/src/passes.rs

@@ -1067,48 +1067,18 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) {
     });
 }
 
-/// Check for the `#[rustc_error]` annotation, which forces an error in codegen. This is used
-/// to write UI tests that actually test that compilation succeeds without reporting
-/// an error.
-fn check_for_rustc_errors_attr(tcx: TyCtxt<'_>) {
-    let Some((def_id, _)) = tcx.entry_fn(()) else { return };
-    for attr in tcx.get_attrs(def_id, sym::rustc_error) {
-        match attr.meta_item_list() {
-            // Check if there is a `#[rustc_error(delayed_bug_from_inside_query)]`.
-            Some(list)
-                if list.iter().any(|list_item| {
-                    matches!(
-                        list_item.ident().map(|i| i.name),
-                        Some(sym::delayed_bug_from_inside_query)
-                    )
-                }) =>
-            {
-                tcx.ensure_ok().trigger_delayed_bug(def_id);
-            }
-
-            // Bare `#[rustc_error]`.
-            None => {
-                tcx.dcx().emit_fatal(errors::RustcErrorFatal { span: tcx.def_span(def_id) });
-            }
-
-            // Some other attribute.
-            Some(_) => {
-                tcx.dcx().emit_warn(errors::RustcErrorUnexpectedAnnotation {
-                    span: tcx.def_span(def_id),
-                });
-            }
-        }
-    }
-}
-
 /// Runs the codegen backend, after which the AST and analysis can
 /// be discarded.
 pub(crate) fn start_codegen<'tcx>(
     codegen_backend: &dyn CodegenBackend,
     tcx: TyCtxt<'tcx>,
 ) -> Box<dyn Any> {
-    // Hook for UI tests.
-    check_for_rustc_errors_attr(tcx);
+    // Hook for tests.
+    if let Some((def_id, _)) = tcx.entry_fn(())
+        && tcx.has_attr(def_id, sym::rustc_delayed_bug_from_inside_query)
+    {
+        tcx.ensure_ok().trigger_delayed_bug(def_id);
+    }
 
     // Don't run this test assertions when not doing codegen. Compiletest tries to build
     // build-fail tests in check mode first and expects it to not give an error in that case.

Diff for: compiler/rustc_middle/src/mir/syntax.rs

@@ -334,14 +334,19 @@ pub enum StatementKind<'tcx> {
     /// See [`Rvalue`] documentation for details on each of those.
     Assign(Box<(Place<'tcx>, Rvalue<'tcx>)>),
 
-    /// This represents all the reading that a pattern match may do (e.g., inspecting constants and
-    /// discriminant values), and the kind of pattern it comes from. This is in order to adapt
-    /// potential error messages to these specific patterns.
+    /// When executed at runtime, this is a nop.
     ///
-    /// Note that this also is emitted for regular `let` bindings to ensure that locals that are
-    /// never accessed still get some sanity checks for, e.g., `let x: ! = ..;`
+    /// During static analysis, a fake read:
+    /// - requires that the value being read is initialized (or, in the case
+    ///   of closures, that it was fully initialized at some point in the past)
+    /// - constitutes a use of a value for the purposes of NLL (i.e. if the
+    ///   value being fake-read is a reference, the lifetime of that reference
+    ///   will be extended to cover the `FakeRead`)
+    /// - but, unlike an actual read, does *not* invalidate any exclusive
+    ///   borrows.
     ///
-    /// When executed at runtime this is a nop.
+    /// See [`FakeReadCause`] for more details on the situations in which a
+    /// `FakeRead` is emitted.
     ///
     /// Disallowed after drop elaboration.
     FakeRead(Box<(FakeReadCause, Place<'tcx>)>),
@@ -518,28 +523,59 @@ pub enum RetagKind {
 /// The `FakeReadCause` describes the type of pattern why a FakeRead statement exists.
 #[derive(Copy, Clone, TyEncodable, TyDecodable, Debug, Hash, HashStable, PartialEq)]
 pub enum FakeReadCause {
-    /// Inject a fake read of the borrowed input at the end of each guards
-    /// code.
+    /// A fake read injected into a match guard to ensure that the discriminants
+    /// that are being matched on aren't modified while the match guard is being
+    /// evaluated.
+    ///
+    /// At the beginning of each match guard, a [fake borrow][FakeBorrowKind] is
+    /// inserted for each discriminant accessed in the entire `match` statement.
+    ///
+    /// Then, at the end of the match guard, a `FakeRead(ForMatchGuard)` is
+    /// inserted to keep the fake borrows alive until that point.
     ///
     /// This should ensure that you cannot change the variant for an enum while
     /// you are in the midst of matching on it.
     ForMatchGuard,
 
-    /// `let x: !; match x {}` doesn't generate any read of x so we need to
-    /// generate a read of x to check that it is initialized and safe.
+    /// Fake read of the scrutinee of a `match` or destructuring `let`
+    /// (i.e. `let` with non-trivial pattern).
+    ///
+    /// In `match x { ... }`, we generate a `FakeRead(ForMatchedPlace, x)`
+    /// and insert it into the `otherwise_block` (which is supposed to be
+    /// unreachable for irrefutable pattern-matches like `match` or `let`).
+    ///
+    /// This is necessary because `let x: !; match x {}` doesn't generate any
+    /// actual read of x, so we need to generate a `FakeRead` to check that it
+    /// is initialized.
     ///
-    /// If a closure pattern matches a Place starting with an Upvar, then we introduce a
-    /// FakeRead for that Place outside the closure, in such a case this option would be
-    /// Some(closure_def_id).
-    /// Otherwise, the value of the optional LocalDefId will be None.
+    /// If the `FakeRead(ForMatchedPlace)` is being performed with a closure
+    /// that doesn't capture the required upvars, the `FakeRead` within the
+    /// closure is omitted entirely.
+    ///
+    /// To make sure that this is still sound, if a closure matches against
+    /// a Place starting with an Upvar, we hoist the `FakeRead` to the
+    /// definition point of the closure.
+    ///
+    /// If the `FakeRead` comes from being hoisted out of a closure like this,
+    /// we record the `LocalDefId` of the closure. Otherwise, the `Option` will be `None`.
     //
     // We can use LocalDefId here since fake read statements are removed
     // before codegen in the `CleanupNonCodegenStatements` pass.
     ForMatchedPlace(Option<LocalDefId>),
 
-    /// A fake read of the RefWithinGuard version of a bind-by-value variable
-    /// in a match guard to ensure that its value hasn't change by the time
-    /// we create the OutsideGuard version.
+    /// A fake read injected into a match guard to ensure that the places
+    /// bound by the pattern are immutable for the duration of the match guard.
+    ///
+    /// Within a match guard, references are created for each place that the
+    /// pattern creates a binding for — this is known as the `RefWithinGuard`
+    /// version of the variables. To make sure that the references stay
+    /// alive until the end of the match guard, and properly prevent the
+    /// places in question from being modified, a `FakeRead(ForGuardBinding)`
+    /// is inserted at the end of the match guard.
+    ///
+    /// For details on how these references are created, see the extensive
+    /// documentation on `bind_matched_candidate_for_guard` in
+    /// `rustc_mir_build`.
     ForGuardBinding,
 
     /// Officially, the semantics of
@@ -552,22 +588,42 @@ pub enum FakeReadCause {
     /// However, if we see the simple pattern `let var = <expr>`, we optimize this to
     /// evaluate `<expr>` directly into the variable `var`. This is mostly unobservable,
     /// but in some cases it can affect the borrow checker, as in #53695.
-    /// Therefore, we insert a "fake read" here to ensure that we get
-    /// appropriate errors.
     ///
-    /// If a closure pattern matches a Place starting with an Upvar, then we introduce a
-    /// FakeRead for that Place outside the closure, in such a case this option would be
-    /// Some(closure_def_id).
-    /// Otherwise, the value of the optional DefId will be None.
+    /// Therefore, we insert a `FakeRead(ForLet)` immediately after each `let`
+    /// with a trivial pattern.
+    ///
+    /// FIXME: `ExprUseVisitor` has an entirely different opinion on what `FakeRead(ForLet)`
+    /// is supposed to mean. If it was accurate to what MIR lowering does,
+    /// would it even make sense to hoist these out of closures like
+    /// `ForMatchedPlace`?
     ForLet(Option<LocalDefId>),
 
-    /// If we have an index expression like
+    /// Currently, index expressions overloaded through the `Index` trait
+    /// get lowered differently than index expressions with builtin semantics
+    /// for arrays and slices — the latter will emit code to perform
+    /// bound checks, and then return a MIR place that will only perform the
+    /// indexing "for real" when it gets incorporated into an instruction.
+    ///
+    /// This is observable in the fact that the following compiles:
+    ///
+    /// ```
+    /// fn f(x: &mut [&mut [u32]], i: usize) {
+    ///     x[i][x[i].len() - 1] += 1;
+    /// }
+    /// ```
+    ///
+    /// However, we need to be careful to not let the user invalidate the
+    /// bound check with an expression like
+    ///
+    /// `(*x)[1][{ x = y; 4}]`
     ///
-    /// (*x)[1][{ x = y; 4}]
+    /// Here, the first bounds check would be invalidated when we evaluate the
+    /// second index expression. To make sure that this doesn't happen, we
+    /// create a fake borrow of `x` and hold it while we evaluate the second
+    /// index.
     ///
-    /// then the first bounds check is invalidated when we evaluate the second
-    /// index expression. Thus we create a fake borrow of `x` across the second
-    /// indexer, which will cause a borrow check error.
+    /// This borrow is kept alive by a `FakeRead(ForIndex)` at the end of its
+    /// scope.
     ForIndex,
 }
 

Diff for: compiler/rustc_middle/src/util/bug.rs

@@ -49,7 +49,7 @@ fn opt_span_bug_fmt<S: Into<MultiSpan>>(
 pub fn trigger_delayed_bug(tcx: TyCtxt<'_>, key: rustc_hir::def_id::DefId) {
     tcx.dcx().span_delayed_bug(
         tcx.def_span(key),
-        "delayed bug triggered by #[rustc_error(delayed_bug_from_inside_query)]",
+        "delayed bug triggered by #[rustc_delayed_bug_from_inside_query]",
     );
 }
 

Diff for: compiler/rustc_span/src/symbol.rs

@@ -1774,6 +1774,7 @@ symbols! {
         rustc_deallocator,
         rustc_def_path,
         rustc_default_body_unstable,
+        rustc_delayed_bug_from_inside_query,
         rustc_deny_explicit_impl,
         rustc_deprecated_safe_2024,
         rustc_diagnostic_item,
@@ -1790,7 +1791,6 @@ symbols! {
         rustc_dump_user_args,
         rustc_dump_vtable,
         rustc_effective_visibility,
-        rustc_error,
         rustc_evaluate_where_clauses,
         rustc_expected_cgu_reuse,
         rustc_force_inline,

Diff for: library/alloc/src/vec/mod.rs

@@ -3360,10 +3360,6 @@ impl<T: Hash, A: Allocator> Hash for Vec<T, A> {
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
-#[rustc_on_unimplemented(
-    message = "vector indices are of type `usize` or ranges of `usize`",
-    label = "vector indices are of type `usize` or ranges of `usize`"
-)]
 impl<T, I: SliceIndex<[T]>, A: Allocator> Index<I> for Vec<T, A> {
     type Output = I::Output;
 
@@ -3374,10 +3370,6 @@ impl<T, I: SliceIndex<[T]>, A: Allocator> Index<I> for Vec<T, A> {
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
-#[rustc_on_unimplemented(
-    message = "vector indices are of type `usize` or ranges of `usize`",
-    label = "vector indices are of type `usize` or ranges of `usize`"
-)]
 impl<T, I: SliceIndex<[T]>, A: Allocator> IndexMut<I> for Vec<T, A> {
     #[inline]
     fn index_mut(&mut self, index: I) -> &mut Self::Output {

Diff for: library/core/src/iter/traits/accum.rs

@@ -10,7 +10,7 @@ use crate::num::Wrapping;
 /// [`sum()`]: Iterator::sum
 /// [`FromIterator`]: iter::FromIterator
 #[stable(feature = "iter_arith_traits", since = "1.12.0")]
-#[rustc_on_unimplemented(
+#[diagnostic::on_unimplemented(
     message = "a value of type `{Self}` cannot be made by summing an iterator over elements of type `{A}`",
     label = "value of type `{Self}` cannot be made by summing a `std::iter::Iterator<Item={A}>`"
 )]
@@ -31,7 +31,7 @@ pub trait Sum<A = Self>: Sized {
 /// [`product()`]: Iterator::product
 /// [`FromIterator`]: iter::FromIterator
 #[stable(feature = "iter_arith_traits", since = "1.12.0")]
-#[rustc_on_unimplemented(
+#[diagnostic::on_unimplemented(
     message = "a value of type `{Self}` cannot be made by multiplying all elements of type `{A}` from an iterator",
     label = "value of type `{Self}` cannot be made by multiplying all elements from a `std::iter::Iterator<Item={A}>`"
 )]

Diff for: src/bootstrap/src/bin/main.rs

@@ -191,7 +191,7 @@ fn check_version(config: &Config) -> Option<String> {
     }
 
     msg.push_str("There have been changes to x.py since you last updated:\n");
-    msg.push_str(&human_readable_changes(&changes));
+    msg.push_str(&human_readable_changes(changes));
 
     msg.push_str("NOTE: to silence this warning, ");
     msg.push_str(&format!(

Diff for: src/bootstrap/src/core/config/config.rs

@@ -1381,7 +1381,7 @@ impl Config {
                     if !changes.is_empty() {
                         println!(
                             "WARNING: There have been changes to x.py since you last updated:\n{}",
-                            crate::human_readable_changes(&changes)
+                            crate::human_readable_changes(changes)
                         );
                     }
                 }