rust-lang · bors · Dec 14, 2022 · Dec 2, 2022 · Dec 2, 2022 · Dec 3, 2022
diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs
@@ -31,6 +31,44 @@ impl<'hir> LoweringContext<'_, 'hir> {
 
     pub(super) fn lower_expr_mut(&mut self, e: &Expr) -> hir::Expr<'hir> {
         ensure_sufficient_stack(|| {
+            match &e.kind {
+                // Paranthesis expression does not have a HirId and is handled specially.
+                ExprKind::Paren(ex) => {
+                    let mut ex = self.lower_expr_mut(ex);
+                    // Include parens in span, but only if it is a super-span.
+                    if e.span.contains(ex.span) {
+                        ex.span = self.lower_span(e.span);
+                    }
+                    // Merge attributes into the inner expression.
+                    if !e.attrs.is_empty() {
+                        let old_attrs =
+                            self.attrs.get(&ex.hir_id.local_id).map(|la| *la).unwrap_or(&[]);
+                        self.attrs.insert(
+                            ex.hir_id.local_id,
+                            &*self.arena.alloc_from_iter(
+                                e.attrs
+                                    .iter()
+                                    .map(|a| self.lower_attr(a))
+                                    .chain(old_attrs.iter().cloned()),
+                            ),
+                        );
+                    }
+                    return ex;
+                }
+                // Desugar `ExprForLoop`
+                // from: `[opt_ident]: for <pat> in <head> <body>`
+                //
+                // This also needs special handling because the HirId of the returned `hir::Expr` will not
+                // correspond to the `e.id`, so `lower_expr_for` handles attribute lowering itself.
+                ExprKind::ForLoop(pat, head, body, opt_label) => {
+                    return self.lower_expr_for(e, pat, head, body, *opt_label);
+                }
+                _ => (),
+            }
+
+            let hir_id = self.lower_node_id(e.id);
+            self.lower_attrs(hir_id, &e.attrs);
+
             let kind = match &e.kind {
                 ExprKind::Box(inner) => hir::ExprKind::Box(self.lower_expr(inner)),
                 ExprKind::Array(exprs) => hir::ExprKind::Array(self.lower_exprs(exprs)),
@@ -48,7 +86,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
                     if e.attrs.get(0).map_or(false, |a| a.has_name(sym::rustc_box)) {
                         if let [inner] = &args[..] && e.attrs.len() == 1 {
                             let kind = hir::ExprKind::Box(self.lower_expr(&inner));
-                            let hir_id = self.lower_node_id(e.id);
                             return hir::Expr { hir_id, kind, span: self.lower_span(e.span) };
                         } else {
                             self.tcx.sess.emit_err(RustcBoxAttributeError { span: e.span });
@@ -147,7 +184,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 ),
                 ExprKind::Async(capture_clause, closure_node_id, block) => self.make_async_expr(
                     *capture_clause,
-                    None,
+                    hir_id,
                     *closure_node_id,
                     None,
                     e.span,
@@ -184,6 +221,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
                             binder,
                             *capture_clause,
                             e.id,
+                            hir_id,
                             *closure_id,
                             fn_decl,
                             body,
@@ -279,39 +317,12 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 ExprKind::Yield(opt_expr) => self.lower_expr_yield(e.span, opt_expr.as_deref()),
                 ExprKind::Err => hir::ExprKind::Err,
                 ExprKind::Try(sub_expr) => self.lower_expr_try(e.span, sub_expr),
-                ExprKind::Paren(ex) => {
-                    let mut ex = self.lower_expr_mut(ex);
-                    // Include parens in span, but only if it is a super-span.
-                    if e.span.contains(ex.span) {
-                        ex.span = self.lower_span(e.span);
-                    }
-                    // Merge attributes into the inner expression.
-                    if !e.attrs.is_empty() {
-                        let old_attrs =
-                            self.attrs.get(&ex.hir_id.local_id).map(|la| *la).unwrap_or(&[]);
-                        self.attrs.insert(
-                            ex.hir_id.local_id,
-                            &*self.arena.alloc_from_iter(
-                                e.attrs
-                                    .iter()
-                                    .map(|a| self.lower_attr(a))
-                                    .chain(old_attrs.iter().cloned()),
-                            ),
-                        );
-                    }
-                    return ex;
-                }
 
-                // Desugar `ExprForLoop`
-                // from: `[opt_ident]: for <pat> in <head> <body>`
-                ExprKind::ForLoop(pat, head, body, opt_label) => {
-                    return self.lower_expr_for(e, pat, head, body, *opt_label);
-                }
+                ExprKind::Paren(_) | ExprKind::ForLoop(..) => unreachable!("already handled"),
+
                 ExprKind::MacCall(_) => panic!("{:?} shouldn't exist here", e.span),
             };
 
-            let hir_id = self.lower_node_id(e.id);
-            self.lower_attrs(hir_id, &e.attrs);
             hir::Expr { hir_id, kind, span: self.lower_span(e.span) }
         })
     }
@@ -576,7 +587,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
     pub(super) fn make_async_expr(
         &mut self,
         capture_clause: CaptureBy,
-        outer_hir_id: Option<hir::HirId>,
+        outer_hir_id: hir::HirId,
         closure_node_id: NodeId,
         ret_ty: Option<hir::FnRetTy<'hir>>,
         span: Span,
@@ -669,8 +680,9 @@ impl<'hir> LoweringContext<'_, 'hir> {
             hir::ExprKind::Closure(c)
         };
 
-        let track_caller = outer_hir_id
-            .and_then(|id| self.attrs.get(&id.local_id))
+        let track_caller = self
+            .attrs
+            .get(&outer_hir_id.local_id)
             .map_or(false, |attrs| attrs.into_iter().any(|attr| attr.has_name(sym::track_caller)));
 
         let hir_id = self.lower_node_id(closure_node_id);
@@ -985,6 +997,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         binder: &ClosureBinder,
         capture_clause: CaptureBy,
         closure_id: NodeId,
+        closure_hir_id: hir::HirId,
         inner_closure_id: NodeId,
         decl: &FnDecl,
         body: &Expr,
@@ -1018,9 +1031,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 
                 let async_body = this.make_async_expr(
                     capture_clause,
-                    // FIXME(nbdd0121): This should also use a proper HIR id so `#[track_caller]`
-                    // can be applied on async closures as well.
-                    None,
+                    closure_hir_id,
                     inner_closure_id,
                     async_ret_ty,
                     body.span,

diff --git a/compiler/rustc_ast_lowering/src/item.rs b/compiler/rustc_ast_lowering/src/item.rs
@@ -1139,7 +1139,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 
             let async_expr = this.make_async_expr(
                 CaptureBy::Value,
-                Some(fn_id),
+                fn_id,
                 closure_id,
                 None,
                 body.span,

diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs
@@ -1059,17 +1059,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     );
                     if self.fn_self_span_reported.insert(fn_span) {
                         err.span_note(
-                            // Check whether the source is accessible
-                            if self.infcx.tcx.sess.source_map().is_span_accessible(self_arg.span) {
-                                self_arg.span
-                            } else {
-                                fn_call_span
-                            },
+                            self_arg.span,
                             "calling this operator moves the left-hand side",
                         );
                     }
                 }
-                CallKind::Normal { self_arg, desugaring, is_option_or_result } => {
+                CallKind::Normal { self_arg, desugaring, method_did } => {
                     let self_arg = self_arg.unwrap();
                     if let Some((CallDesugaringKind::ForLoopIntoIter, _)) = desugaring {
                         let ty = moved_place.ty(self.body, self.infcx.tcx).ty;
@@ -1139,14 +1134,27 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             ),
                         );
                     }
+                    let tcx = self.infcx.tcx;
                     // Avoid pointing to the same function in multiple different
                     // error messages.
                     if span != DUMMY_SP && self.fn_self_span_reported.insert(self_arg.span) {
+                        let func = tcx.def_path_str(method_did);
                         err.span_note(
                             self_arg.span,
-                            &format!("this function takes ownership of the receiver `self`, which moves {}", place_name)
+                            &format!("`{func}` takes ownership of the receiver `self`, which moves {place_name}")
                         );
                     }
+                    let parent_did = tcx.parent(method_did);
+                    let parent_self_ty = (tcx.def_kind(parent_did)
+                        == rustc_hir::def::DefKind::Impl)
+                        .then_some(parent_did)
+                        .and_then(|did| match tcx.type_of(did).kind() {
+                            ty::Adt(def, ..) => Some(def.did()),
+                            _ => None,
+                        });
+                    let is_option_or_result = parent_self_ty.map_or(false, |def_id| {
+                        matches!(tcx.get_diagnostic_name(def_id), Some(sym::Option | sym::Result))
+                    });
                     if is_option_or_result && maybe_reinitialized_locations_is_empty {
                         err.span_label(
                             var_span,

diff --git a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs b/compiler/rustc_borrowck/src/diagnostics/move_errors.rs
@@ -4,7 +4,7 @@ use rustc_middle::ty;
 use rustc_mir_dataflow::move_paths::{
     IllegalMoveOrigin, IllegalMoveOriginKind, LookupResult, MoveError, MovePathIndex,
 };
-use rustc_span::Span;
+use rustc_span::{BytePos, Span};
 
 use crate::diagnostics::{DescribePlaceOpt, UseSpans};
 use crate::prefixes::PrefixSet;
@@ -148,7 +148,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
         match_span: Span,
         statement_span: Span,
     ) {
-        debug!("append_binding_error(match_place={:?}, match_span={:?})", match_place, match_span);
+        debug!(?match_place, ?match_span, "append_binding_error");
 
         let from_simple_let = match_place.is_none();
         let match_place = match_place.unwrap_or(move_from);
@@ -160,7 +160,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     if let GroupedMoveError::MovesFromPlace { span, binds_to, .. } = ge
                         && match_span == *span
                     {
-                        debug!("appending local({:?}) to list", bind_to);
+                        debug!("appending local({bind_to:?}) to list");
                         if !binds_to.is_empty() {
                             binds_to.push(bind_to);
                         }
@@ -198,7 +198,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     } = ge
                     {
                         if match_span == *span && mpi == *other_mpi {
-                            debug!("appending local({:?}) to list", bind_to);
+                            debug!("appending local({bind_to:?}) to list");
                             binds_to.push(bind_to);
                             return;
                         }
@@ -410,15 +410,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
     fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diagnostic, span: Span) {
         match error {
             GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
-                if let Ok(snippet) = self.infcx.tcx.sess.source_map().span_to_snippet(span) {
-                    err.span_suggestion(
-                        span,
-                        "consider borrowing here",
-                        format!("&{snippet}"),
-                        Applicability::Unspecified,
-                    );
-                }
-
+                self.add_borrow_suggestions(err, span);
                 if binds_to.is_empty() {
                     let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
                     let place_desc = match self.describe_place(move_from.as_ref()) {
@@ -461,39 +453,75 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
         }
     }
 
+    fn add_borrow_suggestions(&self, err: &mut Diagnostic, span: Span) {
+        match self.infcx.tcx.sess.source_map().span_to_snippet(span) {
+            Ok(snippet) if snippet.starts_with('*') => {
+                err.span_suggestion_verbose(
+                    span.with_hi(span.lo() + BytePos(1)),
+                    "consider removing the dereference here",
+                    String::new(),
+                    Applicability::MaybeIncorrect,
+                );
+            }
+            _ => {
+                err.span_suggestion_verbose(
+                    span.shrink_to_lo(),
+                    "consider borrowing here",
+                    "&".to_string(),
+                    Applicability::MaybeIncorrect,
+                );
+            }
+        }
+    }
+
     fn add_move_error_suggestions(&self, err: &mut Diagnostic, binds_to: &[Local]) {
-        let mut suggestions: Vec<(Span, &str, String)> = Vec::new();
+        let mut suggestions: Vec<(Span, String, String)> = Vec::new();
         for local in binds_to {
             let bind_to = &self.body.local_decls[*local];
             if let Some(box LocalInfo::User(ClearCrossCrate::Set(BindingForm::Var(
                 VarBindingForm { pat_span, .. },
             )))) = bind_to.local_info
             {
-                if let Ok(pat_snippet) = self.infcx.tcx.sess.source_map().span_to_snippet(pat_span)
+                let Ok(pat_snippet) =
+                    self.infcx.tcx.sess.source_map().span_to_snippet(pat_span) else { continue; };
+                let Some(stripped) = pat_snippet.strip_prefix('&') else {
+                    suggestions.push((
+                        bind_to.source_info.span.shrink_to_lo(),
+                        "consider borrowing the pattern binding".to_string(),
+                        "ref ".to_string(),
+                    ));
+                    continue;
+                };
+                let inner_pat_snippet = stripped.trim_start();
+                let (pat_span, suggestion, to_remove) = if inner_pat_snippet.starts_with("mut")
+                    && inner_pat_snippet["mut".len()..].starts_with(rustc_lexer::is_whitespace)
                 {
-                    if let Some(stripped) = pat_snippet.strip_prefix('&') {
-                        let pat_snippet = stripped.trim_start();
-                        let (suggestion, to_remove) = if pat_snippet.starts_with("mut")
-                            && pat_snippet["mut".len()..].starts_with(rustc_lexer::is_whitespace)
-                        {
-                            (pat_snippet["mut".len()..].trim_start(), "&mut")
-                        } else {
-                            (pat_snippet, "&")
-                        };
-                        suggestions.push((pat_span, to_remove, suggestion.to_owned()));
-                    }
-                }
+                    let inner_pat_snippet = inner_pat_snippet["mut".len()..].trim_start();
+                    let pat_span = pat_span.with_hi(
+                        pat_span.lo()
+                            + BytePos((pat_snippet.len() - inner_pat_snippet.len()) as u32),
+                    );
+                    (pat_span, String::new(), "mutable borrow")
+                } else {
+                    let pat_span = pat_span.with_hi(
+                        pat_span.lo()
+                            + BytePos(
+                                (pat_snippet.len() - inner_pat_snippet.trim_start().len()) as u32,
+                            ),
+                    );
+                    (pat_span, String::new(), "borrow")
+                };
+                suggestions.push((
+                    pat_span,
+                    format!("consider removing the {to_remove}"),
+                    suggestion.to_string(),
+                ));
             }
         }
         suggestions.sort_unstable_by_key(|&(span, _, _)| span);
         suggestions.dedup_by_key(|&mut (span, _, _)| span);
-        for (span, to_remove, suggestion) in suggestions {
-            err.span_suggestion(
-                span,
-                &format!("consider removing the `{to_remove}`"),
-                suggestion,
-                Applicability::MachineApplicable,
-            );
+        for (span, msg, suggestion) in suggestions {
+            err.span_suggestion_verbose(span, &msg, suggestion, Applicability::MachineApplicable);
         }
     }
 
@@ -521,8 +549,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
 
         if binds_to.len() > 1 {
             err.note(
-                "move occurs because these variables have types that \
-                      don't implement the `Copy` trait",
+                "move occurs because these variables have types that don't implement the `Copy` \
+                 trait",
             );
         }
     }