From 65c17aebaf21f85862eddf3322eae8556c90efbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 13 Nov 2023 23:20:49 +0000 Subject: [PATCH 1/2] Suggest removal of borrow in index when appropriate When encountering ```rust let a = std::collections::HashMap::::new(); let s = "hello"; let _b = &a[&s]; ``` suggest `let _b = &a[s];`. Fix #66023. --- compiler/rustc_hir_typeck/src/expr.rs | 46 +++++++++++++++++-- compiler/rustc_middle/src/traits/mod.rs | 2 + .../src/traits/error_reporting/suggestions.rs | 19 ++++++++ ...oint-at-index-for-obligation-failure.fixed | 8 ++++ .../point-at-index-for-obligation-failure.rs | 3 +- ...int-at-index-for-obligation-failure.stderr | 7 ++- 6 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 tests/ui/indexing/point-at-index-for-obligation-failure.fixed diff --git a/compiler/rustc_hir_typeck/src/expr.rs b/compiler/rustc_hir_typeck/src/expr.rs index c2a8eb3bc8e00..850cc5058a647 100644 --- a/compiler/rustc_hir_typeck/src/expr.rs +++ b/compiler/rustc_hir_typeck/src/expr.rs @@ -2958,7 +2958,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // two-phase not needed because index_ty is never mutable self.demand_coerce(idx, idx_t, index_ty, None, AllowTwoPhase::No); self.select_obligations_where_possible(|errors| { - self.point_at_index(errors, idx.span); + self.point_at_index(errors, idx, expr, base, base_t); }); element_ty } @@ -3131,7 +3131,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { .ok() } - fn point_at_index(&self, errors: &mut Vec>, span: Span) { + fn point_at_index( + &self, + errors: &mut Vec>, + idx: &'tcx hir::Expr<'tcx>, + expr: &'tcx hir::Expr<'tcx>, + base: &'tcx hir::Expr<'tcx>, + base_t: Ty<'tcx>, + ) { + let idx_span = idx.span; let mut seen_preds = FxHashSet::default(); // We re-sort here so that the outer most root obligations comes first, as we have the // subsequent weird logic to identify *every* relevant obligation for proper deduplication @@ -3155,7 +3163,39 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { (root, pred) if seen_preds.contains(&pred) || seen_preds.contains(&root) => {} _ => continue, } - error.obligation.cause.span = span; + error.obligation.cause.span = idx_span; + + // If the index value is a double borrow, that can cause typeck errors + // that can be easily resolved by removing the borrow from the expression. + // We check for that here and provide a suggestion in a custom obligation + // cause code. + if let hir::ExprKind::AddrOf(_, _, idx) = idx.kind { + let idx_t = self.typeck_results.borrow().expr_ty(idx); + if let Some((index_ty, _element_ty)) = + self.lookup_indexing(expr, base, base_t, idx, idx_t) + { + let (_ty, err) = + self.demand_coerce_diag(idx, idx_t, index_ty, None, AllowTwoPhase::No); + if let Some(err) = err { + err.cancel(); + } else if self + .fulfillment_cx + .borrow_mut() + .select_where_possible(self) + .is_empty() + { + if let Some(pred) = error.obligation.predicate.to_opt_poly_trait_pred() { + error.obligation.cause = + error.obligation.cause.clone().derived_cause(pred, |cause| { + ObligationCauseCode::IndexExprDerivedObligation(Box::new(( + cause, + idx_span.with_hi(idx.span.lo()), + ))) + }); + } + } + } + } } } diff --git a/compiler/rustc_middle/src/traits/mod.rs b/compiler/rustc_middle/src/traits/mod.rs index 5d0187a859833..c7656b2d1b4d4 100644 --- a/compiler/rustc_middle/src/traits/mod.rs +++ b/compiler/rustc_middle/src/traits/mod.rs @@ -331,6 +331,8 @@ pub enum ObligationCauseCode<'tcx> { DerivedObligation(DerivedObligationCause<'tcx>), + IndexExprDerivedObligation(Box<(DerivedObligationCause<'tcx>, Span)>), + FunctionArgumentObligation { /// The node of the relevant argument in the function call. arg_hir_id: hir::HirId, diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index d5e1efb966326..deab95ad90175 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -3399,6 +3399,25 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> { ) }); } + ObligationCauseCode::IndexExprDerivedObligation(ref data) => { + ensure_sufficient_stack(|| { + self.note_obligation_cause_code( + body_id, + err, + predicate, + param_env, + &data.0.parent_code, + obligated_types, + seen_requirements, + ) + }); + err.span_suggestion_verbose( + data.1, + "remove this borrow", + String::new(), + Applicability::MaybeIncorrect, + ); + } ObligationCauseCode::TypeAlias(ref nested, span, def_id) => { // #74711: avoid a stack overflow ensure_sufficient_stack(|| { diff --git a/tests/ui/indexing/point-at-index-for-obligation-failure.fixed b/tests/ui/indexing/point-at-index-for-obligation-failure.fixed new file mode 100644 index 0000000000000..07b874552e739 --- /dev/null +++ b/tests/ui/indexing/point-at-index-for-obligation-failure.fixed @@ -0,0 +1,8 @@ +// run-rustfix +fn main() { + let a = std::collections::HashMap::::new(); + let s = "hello"; + let _b = &a[ + s //~ ERROR E0277 + ]; +} diff --git a/tests/ui/indexing/point-at-index-for-obligation-failure.rs b/tests/ui/indexing/point-at-index-for-obligation-failure.rs index e9c429b53ced1..625eb6f5bbf5f 100644 --- a/tests/ui/indexing/point-at-index-for-obligation-failure.rs +++ b/tests/ui/indexing/point-at-index-for-obligation-failure.rs @@ -1,7 +1,8 @@ +// run-rustfix fn main() { let a = std::collections::HashMap::::new(); let s = "hello"; - let _b = a[ + let _b = &a[ &s //~ ERROR E0277 ]; } diff --git a/tests/ui/indexing/point-at-index-for-obligation-failure.stderr b/tests/ui/indexing/point-at-index-for-obligation-failure.stderr index 4cced22789f74..6f5fb91b53580 100644 --- a/tests/ui/indexing/point-at-index-for-obligation-failure.stderr +++ b/tests/ui/indexing/point-at-index-for-obligation-failure.stderr @@ -1,5 +1,5 @@ error[E0277]: the trait bound `String: Borrow<&str>` is not satisfied - --> $DIR/point-at-index-for-obligation-failure.rs:5:9 + --> $DIR/point-at-index-for-obligation-failure.rs:6:9 | LL | &s | ^^ the trait `Borrow<&str>` is not implemented for `String` @@ -7,6 +7,11 @@ LL | &s = help: the trait `Borrow` is implemented for `String` = help: for that trait implementation, expected `str`, found `&str` = note: required for `HashMap` to implement `Index<&&str>` +help: remove this borrow + | +LL - &s +LL + s + | error: aborting due to 1 previous error From 33e8027580807621c8f272c83c259162ff4d5f7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 2 Dec 2023 01:27:13 +0000 Subject: [PATCH 2/2] review comment --- compiler/rustc_hir_typeck/src/expr.rs | 30 +++++++++++------------ compiler/rustc_hir_typeck/src/place_op.rs | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/expr.rs b/compiler/rustc_hir_typeck/src/expr.rs index 850cc5058a647..2b603f3e5cfff 100644 --- a/compiler/rustc_hir_typeck/src/expr.rs +++ b/compiler/rustc_hir_typeck/src/expr.rs @@ -40,7 +40,7 @@ use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKi use rustc_infer::infer::DefineOpaqueTypes; use rustc_infer::infer::InferOk; use rustc_infer::traits::query::NoSolution; -use rustc_infer::traits::ObligationCause; +use rustc_infer::traits::{ObligationCause, TraitEngineExt as _}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AllowTwoPhase}; use rustc_middle::ty::error::{ ExpectedFound, @@ -59,8 +59,9 @@ use rustc_target::abi::{FieldIdx, FIRST_VARIANT}; use rustc_target::spec::abi::Abi::RustIntrinsic; use rustc_trait_selection::infer::InferCtxtExt; use rustc_trait_selection::traits::error_reporting::TypeErrCtxtExt; -use rustc_trait_selection::traits::ObligationCtxt; -use rustc_trait_selection::traits::{self, ObligationCauseCode}; +use rustc_trait_selection::traits::{ + self, ObligationCauseCode, ObligationCtxt, TraitEngine, TraitEngineExt, +}; impl<'a, 'tcx> FnCtxt<'a, 'tcx> { pub fn check_expr_has_type_or_error( @@ -3171,18 +3172,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // cause code. if let hir::ExprKind::AddrOf(_, _, idx) = idx.kind { let idx_t = self.typeck_results.borrow().expr_ty(idx); - if let Some((index_ty, _element_ty)) = - self.lookup_indexing(expr, base, base_t, idx, idx_t) - { - let (_ty, err) = - self.demand_coerce_diag(idx, idx_t, index_ty, None, AllowTwoPhase::No); - if let Some(err) = err { - err.cancel(); - } else if self - .fulfillment_cx - .borrow_mut() - .select_where_possible(self) - .is_empty() + let mut autoderef = self.autoderef(base.span, base_t); + let mut result = None; + while result.is_none() && autoderef.next().is_some() { + result = self.try_index_step(expr, base, &autoderef, idx_t, idx); + } + let obligations = autoderef.into_obligations(); + let mut fulfillment_cx = >::new(&self.infcx); + fulfillment_cx.register_predicate_obligations(&self.infcx, obligations); + if let Some((index_ty, _element_ty)) = result { + if self.can_coerce(idx_t, index_ty) + && fulfillment_cx.select_where_possible(self).is_empty() { if let Some(pred) = error.obligation.predicate.to_opt_poly_trait_pred() { error.obligation.cause = diff --git a/compiler/rustc_hir_typeck/src/place_op.rs b/compiler/rustc_hir_typeck/src/place_op.rs index 79e41ef922759..12b0829583cce 100644 --- a/compiler/rustc_hir_typeck/src/place_op.rs +++ b/compiler/rustc_hir_typeck/src/place_op.rs @@ -98,7 +98,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// supports builtin indexing or overloaded indexing. /// This loop implements one step in that search; the autoderef loop /// is implemented by `lookup_indexing`. - fn try_index_step( + pub(super) fn try_index_step( &self, expr: &hir::Expr<'_>, base_expr: &hir::Expr<'_>,