Skip to content

Suggest removal of borrow in index when appropriate #117913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 46 additions & 6 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(
Expand Down Expand Up @@ -2958,7 +2959,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
}
Expand Down Expand Up @@ -3131,7 +3132,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.ok()
}

fn point_at_index(&self, errors: &mut Vec<traits::FulfillmentError<'tcx>>, span: Span) {
fn point_at_index(
&self,
errors: &mut Vec<traits::FulfillmentError<'tcx>>,
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
Expand All @@ -3155,7 +3164,38 @@ 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);
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try_index_step still registers things into the fnctxt's fulfullment context :(

I think it's simpler to be creating your own Index trait obligation here -- the only thing that you'd be losing is the [u8; N] -> [u8] deref step, which I guess you could replicate here.

}
let obligations = autoderef.into_obligations();
let mut fulfillment_cx = <dyn TraitEngine<'_>>::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 =
error.obligation.cause.clone().derived_cause(pred, |cause| {
ObligationCauseCode::IndexExprDerivedObligation(Box::new((
cause,
idx_span.with_hi(idx.span.lo()),
)))
});
}
}
}
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/place_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_>,
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(|| {
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/indexing/point-at-index-for-obligation-failure.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// run-rustfix
fn main() {
let a = std::collections::HashMap::<String,String>::new();
let s = "hello";
let _b = &a[
s //~ ERROR E0277
];
}
3 changes: 2 additions & 1 deletion tests/ui/indexing/point-at-index-for-obligation-failure.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// run-rustfix
fn main() {
let a = std::collections::HashMap::<String,String>::new();
let s = "hello";
let _b = a[
let _b = &a[
&s //~ ERROR E0277
];
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
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`
|
= help: the trait `Borrow<str>` is implemented for `String`
= help: for that trait implementation, expected `str`, found `&str`
= note: required for `HashMap<String, String>` to implement `Index<&&str>`
help: remove this borrow
|
LL - &s
LL + s
|

error: aborting due to 1 previous error

Expand Down