Skip to content
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

Report more detailed reason why Index impl is not satisfied #110432

Merged
merged 3 commits into from
Apr 19, 2023
Merged
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
90 changes: 90 additions & 0 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ use rustc_infer::infer;
use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use rustc_infer::infer::DefineOpaqueTypes;
use rustc_infer::infer::InferOk;
use rustc_infer::traits::query::NoSolution;
use rustc_infer::traits::ObligationCause;
use rustc_middle::middle::stability;
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AllowTwoPhase};
Expand All @@ -53,6 +54,8 @@ use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_target::abi::FieldIdx;
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};

impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Expand Down Expand Up @@ -2800,6 +2803,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
element_ty
}
None => {
// Attempt to *shallowly* search for an impl which matches,
// but has nested obligations which are unsatisfied.
for (base_t, _) in self.autoderef(base.span, base_t).silence_errors() {
if let Some((_, index_ty, element_ty)) =
self.find_and_report_unsatisfied_index_impl(expr.hir_id, base, base_t)
{
self.demand_coerce(idx, idx_t, index_ty, None, AllowTwoPhase::No);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we registering a coercion? To avoid ambiguities later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly just copying the confirmation logic from the happy path. Leaving it unconstrained probably isn't great, but I haven't thought much about creating additional spurious errors from this.

return element_ty;
}
}

let mut err = type_error_struct!(
self.tcx.sess,
expr.span,
Expand Down Expand Up @@ -2843,6 +2857,82 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

/// Try to match an implementation of `Index` against a self type, and report
/// the unsatisfied predicates that result from confirming this impl.
///
/// Given an index expression, sometimes the `Self` type shallowly but does not
/// deeply satisfy an impl predicate. Instead of simply saying that the type
/// does not support being indexed, we want to point out exactly what nested
/// predicates cause this to be, so that the user can add them to fix their code.
fn find_and_report_unsatisfied_index_impl(
&self,
index_expr_hir_id: HirId,
base_expr: &hir::Expr<'_>,
base_ty: Ty<'tcx>,
) -> Option<(ErrorGuaranteed, Ty<'tcx>, Ty<'tcx>)> {
let index_trait_def_id = self.tcx.lang_items().index_trait()?;
let index_trait_output_def_id = self.tcx.get_diagnostic_item(sym::IndexOutput)?;

let mut relevant_impls = vec![];
self.tcx.for_each_relevant_impl(index_trait_def_id, base_ty, |impl_def_id| {
relevant_impls.push(impl_def_id);
});
let [impl_def_id] = relevant_impls[..] else {
// Only report unsatisfied impl predicates if there's one impl
return None;
};

self.commit_if_ok(|_| {
let ocx = ObligationCtxt::new_in_snapshot(self);
let impl_substs = self.fresh_substs_for_item(base_expr.span, impl_def_id);
let impl_trait_ref =
self.tcx.impl_trait_ref(impl_def_id).unwrap().subst(self.tcx, impl_substs);
let cause = self.misc(base_expr.span);

// Match the impl self type against the base ty. If this fails,
// we just skip this impl, since it's not particularly useful.
let impl_trait_ref = ocx.normalize(&cause, self.param_env, impl_trait_ref);
ocx.eq(&cause, self.param_env, impl_trait_ref.self_ty(), base_ty)?;

// Register the impl's predicates. One of these predicates
// must be unsatisfied, or else we wouldn't have gotten here
// in the first place.
ocx.register_obligations(traits::predicates_for_generics(
|idx, span| {
traits::ObligationCause::new(
base_expr.span,
self.body_id,
if span.is_dummy() {
traits::ExprItemObligation(impl_def_id, index_expr_hir_id, idx)
} else {
traits::ExprBindingObligation(impl_def_id, span, index_expr_hir_id, idx)
},
)
},
self.param_env,
self.tcx.predicates_of(impl_def_id).instantiate(self.tcx, impl_substs),
));

// Normalize the output type, which we can use later on as the
// return type of the index expression...
let element_ty = ocx.normalize(
&cause,
self.param_env,
self.tcx.mk_projection(index_trait_output_def_id, impl_trait_ref.substs),
);

let errors = ocx.select_where_possible();
// There should be at least one error reported. If not, we
// will still delay a span bug in `report_fulfillment_errors`.
Ok::<_, NoSolution>((
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are emitting an error, why are we committing inference constraints?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because otherwise we wouldn't be able to return a useful return type here for later typechecking to use. I guess we could return ty::Error here or something, but I prefer recoveries to be as accurate as possible for downstream typechecking.

self.err_ctxt().report_fulfillment_errors(&errors),
impl_trait_ref.substs.type_at(1),
element_ty,
))
})
.ok()
}

fn point_at_index_if_possible(
&self,
errors: &mut Vec<traits::FulfillmentError<'tcx>>,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ symbols! {
HashSet,
Hasher,
Implied,
IndexOutput,
Input,
Into,
IntoDiagnostic,
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ops/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
pub trait Index<Idx: ?Sized> {
/// The returned type after indexing.
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_diagnostic_item = "IndexOutput"]
type Output: ?Sized;

/// Performs the indexing (`container[index]`) operation.
Expand Down
27 changes: 27 additions & 0 deletions tests/ui/typeck/bad-index-due-to-nested.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
use std::hash::Hash;
Copy link
Member Author

Choose a reason for hiding this comment

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

I would've marked this as // run-rustfix but I cannot due to overlapping suggestion spans. I'll fix that in a different PR.

use std::marker::PhantomData;
use std::ops::Index;

struct HashMap<K, V>(PhantomData<(K, V)>);

impl<K, V> Index<&K> for HashMap<K, V>
where
K: Hash,
V: Copy,
{
type Output = V;

fn index(&self, k: &K) -> &V {
todo!()
}
}

fn index<'a, K, V>(map: &'a HashMap<K, V>, k: K) -> &'a V {
map[k]
//~^ ERROR the trait bound `K: Hash` is not satisfied
//~| ERROR the trait bound `V: Copy` is not satisfied
//~| ERROR mismatched types
//~| ERROR mismatched types
}

fn main() {}
64 changes: 64 additions & 0 deletions tests/ui/typeck/bad-index-due-to-nested.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
error[E0277]: the trait bound `K: Hash` is not satisfied
--> $DIR/bad-index-due-to-nested.rs:20:5
|
LL | map[k]
| ^^^ the trait `Hash` is not implemented for `K`
|
note: required by a bound in `<HashMap<K, V> as Index<&K>>`
--> $DIR/bad-index-due-to-nested.rs:9:8
|
LL | K: Hash,
| ^^^^ required by this bound in `<HashMap<K, V> as Index<&K>>`
help: consider restricting type parameter `K`
|
LL | fn index<'a, K: std::hash::Hash, V>(map: &'a HashMap<K, V>, k: K) -> &'a V {
| +++++++++++++++++

error[E0277]: the trait bound `V: Copy` is not satisfied
--> $DIR/bad-index-due-to-nested.rs:20:5
|
LL | map[k]
| ^^^ the trait `Copy` is not implemented for `V`
|
note: required by a bound in `<HashMap<K, V> as Index<&K>>`
--> $DIR/bad-index-due-to-nested.rs:10:8
|
LL | V: Copy,
| ^^^^ required by this bound in `<HashMap<K, V> as Index<&K>>`
help: consider restricting type parameter `V`
|
LL | fn index<'a, K, V: std::marker::Copy>(map: &'a HashMap<K, V>, k: K) -> &'a V {
| +++++++++++++++++++

error[E0308]: mismatched types
--> $DIR/bad-index-due-to-nested.rs:20:9
|
LL | fn index<'a, K, V>(map: &'a HashMap<K, V>, k: K) -> &'a V {
| - this type parameter
LL | map[k]
| ^
| |
| expected `&K`, found type parameter `K`
| help: consider borrowing here: `&k`
|
= note: expected reference `&K`
found type parameter `K`

error[E0308]: mismatched types
--> $DIR/bad-index-due-to-nested.rs:20:5
|
LL | fn index<'a, K, V>(map: &'a HashMap<K, V>, k: K) -> &'a V {
| - this type parameter ----- expected `&'a V` because of return type
LL | map[k]
| ^^^^^^
| |
| expected `&V`, found type parameter `V`
| help: consider borrowing here: `&map[k]`
|
= note: expected reference `&'a V`
found type parameter `V`

error: aborting due to 4 previous errors

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.