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.

Sign up for GitHub

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

Fix ICE: Restrict param constraint suggestion #117246

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Fix ICE: Restrict param constraint suggestion #117246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bbc70a
Restrict param constraint suggestion
estebank Oct 26, 2023
7449478
Account for type param from other item in `note_and_explain`
estebank Oct 26, 2023
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
113 changes: 67 additions & 46 deletions compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,17 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
}
(ty::Param(expected), ty::Param(found)) => {
let generics = tcx.generics_of(body_owner_def_id);
let e_span = tcx.def_span(generics.type_param(expected, tcx).def_id);
if !sp.contains(e_span) {
diag.span_label(e_span, "expected type parameter");
if let Some(param) = generics.opt_type_param(expected, tcx) {
let e_span = tcx.def_span(param.def_id);
if !sp.contains(e_span) {
diag.span_label(e_span, "expected type parameter");
}
}
let f_span = tcx.def_span(generics.type_param(found, tcx).def_id);
if !sp.contains(f_span) {
diag.span_label(f_span, "found type parameter");
if let Some(param) = generics.opt_type_param(found, tcx) {
let f_span = tcx.def_span(param.def_id);
if !sp.contains(f_span) {
diag.span_label(f_span, "found type parameter");
}
}
diag.note(
"a type parameter was expected, but a different one was found; \
Expand All @@ -83,23 +87,29 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
| (ty::Alias(ty::Projection, proj), ty::Param(p))
if !tcx.is_impl_trait_in_trait(proj.def_id) =>
{
let p_def_id = tcx.generics_of(body_owner_def_id).type_param(p, tcx).def_id;
let p_span = tcx.def_span(p_def_id);
let expected = match (values.expected.kind(), values.found.kind()) {
(ty::Param(_), _) => "expected ",
(_, ty::Param(_)) => "found ",
_ => "",
};
if !sp.contains(p_span) {
diag.span_label(p_span, format!("{expected}this type parameter"));
}
let hir = tcx.hir();
let parent = tcx.generics_of(body_owner_def_id)
.opt_type_param(p, tcx)
.and_then(|param| {
let p_def_id = param.def_id;
let p_span = tcx.def_span(p_def_id);
let expected = match (values.expected.kind(), values.found.kind()) {
(ty::Param(_), _) => "expected ",
(_, ty::Param(_)) => "found ",
_ => "",
};
if !sp.contains(p_span) {
diag.span_label(
p_span,
format!("{expected}this type parameter"),
);
}
p_def_id.as_local().and_then(|id| {
let local_id = tcx.hir().local_def_id_to_hir_id(id);
let generics = tcx.hir().find_parent(local_id)?.generics()?;
Some((id, generics))
})
});
let mut note = true;
let parent = p_def_id.as_local().and_then(|id| {
let local_id = hir.local_def_id_to_hir_id(id);
let generics = tcx.hir().find_parent(local_id)?.generics()?;
Some((id, generics))
});
if let Some((local_id, generics)) = parent {
// Synthesize the associated type restriction `Add<Output = Expected>`.
// FIXME: extract this logic for use in other diagnostics.
Expand Down Expand Up @@ -172,14 +182,16 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
(ty::Param(p), ty::Dynamic(..) | ty::Alias(ty::Opaque, ..))
| (ty::Dynamic(..) | ty::Alias(ty::Opaque, ..), ty::Param(p)) => {
let generics = tcx.generics_of(body_owner_def_id);
let p_span = tcx.def_span(generics.type_param(p, tcx).def_id);
let expected = match (values.expected.kind(), values.found.kind()) {
(ty::Param(_), _) => "expected ",
(_, ty::Param(_)) => "found ",
_ => "",
};
if !sp.contains(p_span) {
diag.span_label(p_span, format!("{expected}this type parameter"));
if let Some(param) = generics.opt_type_param(p, tcx) {
let p_span = tcx.def_span(param.def_id);
let expected = match (values.expected.kind(), values.found.kind()) {
(ty::Param(_), _) => "expected ",
(_, ty::Param(_)) => "found ",
_ => "",
};
if !sp.contains(p_span) {
diag.span_label(p_span, format!("{expected}this type parameter"));
}
}
diag.help("type parameters must be constrained to match other types");
if tcx.sess.teach(&diag.get_code().unwrap()) {
Expand Down Expand Up @@ -217,9 +229,11 @@ impl<T> Trait<T> for X {
}
(ty::Param(p), ty::Closure(..) | ty::Coroutine(..)) => {
let generics = tcx.generics_of(body_owner_def_id);
let p_span = tcx.def_span(generics.type_param(p, tcx).def_id);
if !sp.contains(p_span) {
diag.span_label(p_span, "expected this type parameter");
if let Some(param) = generics.opt_type_param(p, tcx) {
let p_span = tcx.def_span(param.def_id);
if !sp.contains(p_span) {
diag.span_label(p_span, "expected this type parameter");
}
}
diag.help(format!(
"every closure has a distinct type and so could not always match the \
Expand All @@ -228,14 +242,16 @@ impl<T> Trait<T> for X {
}
(ty::Param(p), _) | (_, ty::Param(p)) => {
let generics = tcx.generics_of(body_owner_def_id);
let p_span = tcx.def_span(generics.type_param(p, tcx).def_id);
let expected = match (values.expected.kind(), values.found.kind()) {
(ty::Param(_), _) => "expected ",
(_, ty::Param(_)) => "found ",
_ => "",
};
if !sp.contains(p_span) {
diag.span_label(p_span, format!("{expected}this type parameter"));
if let Some(param) = generics.opt_type_param(p, tcx) {
let p_span = tcx.def_span(param.def_id);
let expected = match (values.expected.kind(), values.found.kind()) {
(ty::Param(_), _) => "expected ",
(_, ty::Param(_)) => "found ",
_ => "",
};
if !sp.contains(p_span) {
diag.span_label(p_span, format!("{expected}this type parameter"));
}
}
}
(ty::Alias(ty::Projection | ty::Inherent, proj_ty), _)
Expand Down Expand Up @@ -364,13 +380,14 @@ impl<T> Trait<T> for X {
};
// Get the `DefId` for the type parameter corresponding to `A` in `<A as T>::Foo`.
// This will also work for `impl Trait`.
let def_id = if let ty::Param(param_ty) = proj_ty.self_ty().kind() {
let generics = tcx.generics_of(body_owner_def_id);
generics.type_param(param_ty, tcx).def_id
} else {
let ty::Param(param_ty) = proj_ty.self_ty().kind() else {
return false;
};
let Some(def_id) = def_id.as_local() else {
let generics = tcx.generics_of(body_owner_def_id);
let Some(param) = generics.opt_type_param(param_ty, tcx) else {
return false;
};
let Some(def_id) = param.def_id.as_local() else {
return false;
};

Expand All @@ -390,6 +407,10 @@ impl<T> Trait<T> for X {
return true;
}
}
if (param_ty.index as usize) >= generics.parent_count {
// The param comes from the current item, do not look at the parent. (#117209)
return false;
}
// If associated item, look to constrain the params of the trait/impl.
let hir_id = match item {
hir::Node::ImplItem(item) => item.hir_id(),
Expand Down
28 changes: 28 additions & 0 deletions compiler/rustc_middle/src/ty/generics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,20 @@ impl<'tcx> Generics {
}
}

/// Returns the `GenericParamDef` with the given index if available.
pub fn opt_param_at(
&'tcx self,
param_index: usize,
tcx: TyCtxt<'tcx>,
) -> Option<&'tcx GenericParamDef> {
if let Some(index) = param_index.checked_sub(self.parent_count) {
self.params.get(index)
} else {
tcx.generics_of(self.parent.expect("parent_count > 0 but no parent?"))
.opt_param_at(param_index, tcx)
}
}

pub fn params_to(&'tcx self, param_index: usize, tcx: TyCtxt<'tcx>) -> &'tcx [GenericParamDef] {
if let Some(index) = param_index.checked_sub(self.parent_count) {
&self.params[..index]
Expand Down Expand Up @@ -268,6 +282,20 @@ impl<'tcx> Generics {
}
}

/// Returns the `GenericParamDef` associated with this `ParamTy` if it belongs to this
/// `Generics`.
pub fn opt_type_param(
&'tcx self,
param: &ParamTy,
tcx: TyCtxt<'tcx>,
) -> Option<&'tcx GenericParamDef> {
let param = self.opt_param_at(param.index as usize, tcx)?;
match param.kind {
GenericParamDefKind::Type { .. } => Some(param),
_ => None,
}
}

/// Returns the `GenericParamDef` associated with this `ParamConst`.
pub fn const_param(&'tcx self, param: &ParamConst, tcx: TyCtxt<'tcx>) -> &GenericParamDef {
let param = self.param_at(param.index as usize, tcx);
Expand Down
28 changes: 28 additions & 0 deletions ...e-bounds/do-not-look-at-parent-item-in-suggestion-for-type-param-of-current-assoc-item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
use std::collections::HashMap;
use std::hash::Hash;

trait LowT: Identify {}

trait Identify {
type Id: Clone + Hash + PartialEq + Eq;
fn identify(&self) -> Self::Id;
}

struct MapStore<L, I>
where
L: LowT + Identify<Id = I>,
{
lows: HashMap<I, L>,
}

impl<L, I> MapStore<L, I>
where
L: LowT + Identify<Id = I>,
I: Clone + Hash + PartialEq + Eq,
{
fn remove_low(&mut self, low: &impl LowT) {
let _low = self.lows.remove(low.identify()).unwrap(); //~ ERROR mismatched types
}
}

fn main() {}
18 changes: 18 additions & 0 deletions ...unds/do-not-look-at-parent-item-in-suggestion-for-type-param-of-current-assoc-item.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
error[E0308]: mismatched types
--> $DIR/do-not-look-at-parent-item-in-suggestion-for-type-param-of-current-assoc-item.rs:24:37
|
LL | let _low = self.lows.remove(low.identify()).unwrap();
| ------ ^^^^^^^^^^^^^^ expected `&I`, found associated type
| |
| arguments to this method are incorrect
|
= note: expected reference `&I`
found associated type `<impl LowT as Identify>::Id`
= help: consider constraining the associated type `<impl LowT as Identify>::Id` to `&I`
= note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
note: method defined here
--> $SRC_DIR/std/src/collections/hash/map.rs:LL:COL

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
7 changes: 7 additions & 0 deletions tests/ui/malformed/do-not-ice-on-note_and_explain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
struct A<B>(B);
impl<B>A<B>{fn d(){fn d(){Self(1)}}}
Copy link
Member

@compiler-errors compiler-errors Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this is not the right way of fixing this bug?

Why do we allow Self constructors from outer items that reference parameters that aren't in scope anyways? Like, we're referencing a type parameter that doesn't exist here, right?

I feel like this also may cause strange behavior (or unsoundness? not exactly sure, depends on when we compare params by name or by index) when we actually do have a substitutable param here:

use std::marker::PhantomData;

struct S0<T>(PhantomData<T>);

impl<T> S0<T> {
    fn foo() {
        fn bar<T2>() -> S0<T2> {
            Self(PhantomData) // <~ this constructs S0<T> not S0<T2> :(
        }
    }
}

cc @cjgillot who tried this and had to unland it in #111557

Copy link
Contributor

Choose a reason for hiding this comment

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

#111020 was reverted due to a regression, but it was probably a right thing to do in general.

Maybe we should do the usual bugfix process with a crater run, deprecation lint, and a tracking issue here.
In the meantime this PR could be merged to avoid the ICE.

//~^ ERROR the size for values of type `B` cannot be known at compilation time
//~| ERROR the size for values of type `B` cannot be known at compilation time
//~| ERROR mismatched types
//~| ERROR mismatched types
//~| ERROR `main` function not found in crate
79 changes: 79 additions & 0 deletions tests/ui/malformed/do-not-ice-on-note_and_explain.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
error[E0601]: `main` function not found in crate `do_not_ice_on_note_and_explain`
--> $DIR/do-not-ice-on-note_and_explain.rs:2:37
|
LL | impl<B>A<B>{fn d(){fn d(){Self(1)}}}
| ^ consider adding a `main` function to `$DIR/do-not-ice-on-note_and_explain.rs`

error[E0277]: the size for values of type `B` cannot be known at compilation time
--> $DIR/do-not-ice-on-note_and_explain.rs:2:32
|
LL | impl<B>A<B>{fn d(){fn d(){Self(1)}}}
| - ---- ^ doesn't have a size known at compile-time
| | |
| | required by a bound introduced by this call
| this type parameter needs to be `Sized`
|
note: required by a bound in `A`
--> $DIR/do-not-ice-on-note_and_explain.rs:1:10
|
LL | struct A<B>(B);
| ^ required by this bound in `A`

error[E0308]: mismatched types
--> $DIR/do-not-ice-on-note_and_explain.rs:2:32
|
LL | impl<B>A<B>{fn d(){fn d(){Self(1)}}}
| ---- ^ expected type parameter `B`, found integer
| |
| arguments to this function are incorrect
|
= note: expected type parameter `B`
found type `{integer}`
note: tuple struct defined here
--> $DIR/do-not-ice-on-note_and_explain.rs:1:8
|
LL | struct A<B>(B);
| ^

error[E0308]: mismatched types
--> $DIR/do-not-ice-on-note_and_explain.rs:2:27
|
LL | impl<B>A<B>{fn d(){fn d(){Self(1)}}}
| ^^^^^^^ expected `()`, found `A<B>`
|
= note: expected unit type `()`
found struct `A<B>`
help: consider using a semicolon here
|
LL | impl<B>A<B>{fn d(){fn d(){Self(1);}}}
| +
help: try adding a return type
|
LL | impl<B>A<B>{fn d(){fn d() -> A<B>{Self(1)}}}
| +++++++

error[E0277]: the size for values of type `B` cannot be known at compilation time
--> $DIR/do-not-ice-on-note_and_explain.rs:2:27
|
LL | impl<B>A<B>{fn d(){fn d(){Self(1)}}}
| - ^^^^^^^ doesn't have a size known at compile-time
| |
| this type parameter needs to be `Sized`
|
note: required by a bound in `A`
--> $DIR/do-not-ice-on-note_and_explain.rs:1:10
|
LL | struct A<B>(B);
| ^ required by this bound in `A`
help: you could relax the implicit `Sized` bound on `B` if it were used through indirection like `&B` or `Box<B>`
--> $DIR/do-not-ice-on-note_and_explain.rs:1:10
|
LL | struct A<B>(B);
| ^ - ...if indirection were used here: `Box<B>`
| |
| this could be changed to `B: ?Sized`...

error: aborting due to 5 previous errors

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