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

Handle mismatched generic param kinds in trait impls betterly #96717

Merged
merged 5 commits into from
May 10, 2022
Merged
Show file tree
Hide file tree
Changes from 3 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
165 changes: 115 additions & 50 deletions compiler/rustc_typeck/src/check/compare_method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ use rustc_hir::intravisit;
use rustc_hir::{GenericParamKind, ImplItemKind, TraitItemKind};
use rustc_infer::infer::{self, InferOk, TyCtxtInferExt};
use rustc_infer::traits::util;
use rustc_middle::ty;
use rustc_middle::ty::error::{ExpectedFound, TypeError};
use rustc_middle::ty::subst::{InternalSubsts, Subst};
use rustc_middle::ty::util::ExplicitSelf;
use rustc_middle::ty::{self, DefIdTree};
use rustc_middle::ty::{GenericParamDefKind, ToPredicate, TyCtxt};
use rustc_span::Span;
use rustc_trait_selection::traits::error_reporting::InferCtxtExt;
Expand Down Expand Up @@ -48,6 +48,10 @@ crate fn compare_impl_method<'tcx>(
return;
}

if let Err(_) = compare_generic_param_kinds(tcx, impl_m, trait_m) {
return;
}

if let Err(_) =
compare_number_of_method_arguments(tcx, impl_m, impl_m_span, trait_m, trait_item_span)
{
Expand All @@ -62,10 +66,6 @@ crate fn compare_impl_method<'tcx>(
{
return;
}

if let Err(_) = compare_const_param_types(tcx, impl_m, trait_m, trait_item_span) {
return;
}
}

fn compare_predicate_entailment<'tcx>(
Expand Down Expand Up @@ -579,6 +579,27 @@ fn compare_self_type<'tcx>(
Ok(())
}

/// Checks that the number of generics on a given assoc item in a trait impl is the same
/// as the number of generics on the respective assoc item in the trait definition.
///
/// For example this code emits the errors in the following code:
/// ```
/// trait Trait {
/// fn foo();
/// type Assoc<T>;
/// }
///
/// impl Trait for () {
/// fn foo<T>() {}
/// //~^ error
/// type Assoc = u32;
/// //~^ error
/// }
/// ```
///
/// Notably this does not error on `foo<T>` implemented as `foo<const N: u8>` or
/// `foo<const N: u8>` implemented as `foo<const N: u32>`. This is handled in
/// [`compare_generic_param_kinds`]. This function also does not handle lifetime parameters
fn compare_number_of_generics<'tcx>(
tcx: TyCtxt<'tcx>,
impl_: &ty::AssocItem,
Expand All @@ -589,6 +610,15 @@ fn compare_number_of_generics<'tcx>(
let trait_own_counts = tcx.generics_of(trait_.def_id).own_counts();
let impl_own_counts = tcx.generics_of(impl_.def_id).own_counts();

// This avoids us erroring on `foo<T>` implemented as `foo<const N: u8>` as this is implemented
// in `compare_generic_param_kinds` which will give a nicer error message than something like:
// "expected 1 type parameter, found 0 type parameters"
if (trait_own_counts.types + trait_own_counts.consts)
== (impl_own_counts.types + impl_own_counts.consts)
{
return Ok(());
}

let matchings = [
("type", trait_own_counts.types, impl_own_counts.types),
("const", trait_own_counts.consts, impl_own_counts.consts),
Expand Down Expand Up @@ -914,60 +944,93 @@ fn compare_synthetic_generics<'tcx>(
if let Some(reported) = error_found { Err(reported) } else { Ok(()) }
}

fn compare_const_param_types<'tcx>(
/// Checks that all parameters in the generics of a given assoc item in a trait impl have
/// the same kind as the respective generic parameter in the trait def.
///
/// For example all 4 errors in the following code are emitted here:
/// ```
/// trait Foo {
/// fn foo<const N: u8>();
/// type bar<const N: u8>;
/// fn baz<const N: u32>();
/// type blah<T>;
/// }
///
/// impl Foo for () {
/// fn foo<const N: u64>() {}
/// //~^ error
/// type bar<const N: u64> {}
/// //~^ error
/// fn baz<T>() {}
/// //~^ error
/// type blah<const N: i64> = u32;
/// //~^ error
/// }
/// ```
///
/// This function does not handle lifetime parameters
fn compare_generic_param_kinds<'tcx>(
tcx: TyCtxt<'tcx>,
impl_m: &ty::AssocItem,
trait_m: &ty::AssocItem,
trait_item_span: Option<Span>,
impl_item: &ty::AssocItem,
trait_item: &ty::AssocItem,
) -> Result<(), ErrorGuaranteed> {
let const_params_of = |def_id| {
tcx.generics_of(def_id).params.iter().filter_map(|param| match param.kind {
GenericParamDefKind::Const { .. } => Some(param.def_id),
_ => None,
assert_eq!(impl_item.kind, trait_item.kind);

let ty_const_params_of = |def_id| {
tcx.generics_of(def_id).params.iter().filter(|param| {
matches!(
param.kind,
GenericParamDefKind::Const { .. } | GenericParamDefKind::Type { .. }
)
})
};
let const_params_impl = const_params_of(impl_m.def_id);
let const_params_trait = const_params_of(trait_m.def_id);

for (const_param_impl, const_param_trait) in iter::zip(const_params_impl, const_params_trait) {
let impl_ty = tcx.type_of(const_param_impl);
let trait_ty = tcx.type_of(const_param_trait);
if impl_ty != trait_ty {
let (impl_span, impl_ident) = match tcx.hir().get_if_local(const_param_impl) {
Some(hir::Node::GenericParam(hir::GenericParam { span, name, .. })) => (
span,
match name {
hir::ParamName::Plain(ident) => Some(ident),
_ => None,
},
),
other => bug!(
"expected GenericParam, found {:?}",
other.map_or_else(|| "nothing".to_string(), |n| format!("{:?}", n))
),
};
let trait_span = match tcx.hir().get_if_local(const_param_trait) {
Some(hir::Node::GenericParam(hir::GenericParam { span, .. })) => Some(span),
_ => None,

for (param_impl, param_trait) in
iter::zip(ty_const_params_of(impl_item.def_id), ty_const_params_of(trait_item.def_id))
{
use GenericParamDefKind::*;
if match (&param_impl.kind, &param_trait.kind) {
(Const { .. }, Const { .. })
if tcx.type_of(param_impl.def_id) != tcx.type_of(param_trait.def_id) =>
{
true
}
(Const { .. }, Type { .. }) | (Type { .. }, Const { .. }) => true,
// this is exhaustive so that anyone adding new generic param kinds knows
// to make sure this error is reported for them.
(Const { .. }, Const { .. }) | (Type { .. }, Type { .. }) => false,
(Lifetime { .. }, _) | (_, Lifetime { .. }) => unreachable!(),
} {
let make_param_message = |prefix: &str, param: &ty::GenericParamDef| match param.kind {
Const { .. } => {
format!("{} const parameter with type `{}`", prefix, tcx.type_of(param.def_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
format!("{} const parameter with type `{}`", prefix, tcx.type_of(param.def_id))
format!("{} const parameter of type `{}`", prefix, tcx.type_of(param.def_id))

For me "of type" seems more natural here, idk 😅

Considering that this closure doesn't capture anything, can you move it out of the loop to ty_const_params_of?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the error wording.

I don't think it'd be good to have the closure be by ty_const_params_of since then its really far away from the code actually calling the closure. I pushed code moving it furthur down so its closer to the call site, imo makes it easier to understand the code.

}
Type { .. } => format!("{} type parameter", prefix),
Lifetime { .. } => unreachable!(),
};

let param_impl_span = tcx.def_span(param_impl.def_id);
let param_trait_span = tcx.def_span(param_trait.def_id);

let mut err = struct_span_err!(
tcx.sess,
*impl_span,
param_impl_span,
E0053,
"method `{}` has an incompatible const parameter type for trait",
trait_m.name
);
err.span_note(
trait_span.map_or_else(|| trait_item_span.unwrap_or(*impl_span), |span| *span),
&format!(
"the const parameter{} has type `{}`, but the declaration \
in trait `{}` has type `{}`",
&impl_ident.map_or_else(|| "".to_string(), |ident| format!(" `{ident}`")),
impl_ty,
tcx.def_path_str(trait_m.def_id),
trait_ty
),
"{} `{}` has an incompatible generic parameter for trait: `{}`",
BoxyUwU marked this conversation as resolved.
Show resolved Hide resolved
assoc_item_kind_str(&impl_item),
trait_item.name,
&tcx.def_path_str(tcx.parent(trait_item.def_id))
);

let trait_header_span = tcx.def_ident_span(tcx.parent(trait_item.def_id)).unwrap();
err.span_label(trait_header_span, "");
err.span_label(param_trait_span, make_param_message("expected", param_trait));

let impl_header_span =
tcx.sess.source_map().guess_head_span(tcx.def_span(tcx.parent(impl_item.def_id)));
err.span_label(impl_header_span, "");
err.span_label(param_impl_span, make_param_message("found", param_impl));

let reported = err.emit();
return Err(reported);
}
Expand Down Expand Up @@ -1095,6 +1158,8 @@ crate fn compare_ty_impl<'tcx>(
let _: Result<(), ErrorGuaranteed> = (|| {
compare_number_of_generics(tcx, impl_ty, impl_ty_span, trait_ty, trait_item_span)?;

compare_generic_param_kinds(tcx, impl_ty, trait_ty)?;

let sp = tcx.def_span(impl_ty.def_id);
compare_type_predicate_entailment(tcx, impl_ty, sp, trait_ty, impl_trait_ref)?;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
trait Trait {
fn foo<U>() {}
}
impl Trait for () {
fn foo<const M: u64>() {}
//~^ error: method `foo` has an incompatible generic parameter for trait
}

trait Other {
fn bar<const M: u8>() {}
}
impl Other for () {
fn bar<T>() {}
//~^ error: method `bar` has an incompatible generic parameter for trait
}

trait Uwu {
fn baz<const N: u32>() {}
}
impl Uwu for () {
fn baz<const N: i32>() {}
//~^ error: method `baz` has an incompatible generic parameter for trait
}

trait Aaaaaa {
fn bbbb<const N: u32, T>() {}
}
impl Aaaaaa for () {
fn bbbb<T, const N: u32>() {}
//~^ error: method `bbbb` has an incompatible generic parameter for trait
}

trait Names {
fn abcd<T, const N: u32>() {}
}
impl Names for () {
fn abcd<const N: u32, T>() {}
//~^ error: method `abcd` has an incompatible generic parameter for trait
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
error[E0053]: method `foo` has an incompatible generic parameter for trait: `Trait`
--> $DIR/mismatched_ty_const_in_trait_impl.rs:5:12
|
LL | trait Trait {
| -----
LL | fn foo<U>() {}
| - expected type parameter
LL | }
LL | impl Trait for () {
| -----------------
LL | fn foo<const M: u64>() {}
| ^^^^^^^^^^^^ found const parameter with type `u64`

error[E0053]: method `bar` has an incompatible generic parameter for trait: `Other`
--> $DIR/mismatched_ty_const_in_trait_impl.rs:13:12
|
LL | trait Other {
| -----
LL | fn bar<const M: u8>() {}
| ----------- expected const parameter with type `u8`
LL | }
LL | impl Other for () {
| -----------------
LL | fn bar<T>() {}
| ^ found type parameter

error[E0053]: method `baz` has an incompatible generic parameter for trait: `Uwu`
--> $DIR/mismatched_ty_const_in_trait_impl.rs:21:12
|
LL | trait Uwu {
| ---
LL | fn baz<const N: u32>() {}
| ------------ expected const parameter with type `u32`
LL | }
LL | impl Uwu for () {
| ---------------
LL | fn baz<const N: i32>() {}
| ^^^^^^^^^^^^ found const parameter with type `i32`

error[E0053]: method `bbbb` has an incompatible generic parameter for trait: `Aaaaaa`
--> $DIR/mismatched_ty_const_in_trait_impl.rs:29:13
|
LL | trait Aaaaaa {
| ------
LL | fn bbbb<const N: u32, T>() {}
| ------------ expected const parameter with type `u32`
LL | }
LL | impl Aaaaaa for () {
| ------------------
LL | fn bbbb<T, const N: u32>() {}
| ^ found type parameter

error[E0053]: method `abcd` has an incompatible generic parameter for trait: `Names`
--> $DIR/mismatched_ty_const_in_trait_impl.rs:37:13
|
LL | trait Names {
| -----
LL | fn abcd<T, const N: u32>() {}
| - expected type parameter
LL | }
LL | impl Names for () {
| -----------------
LL | fn abcd<const N: u32, T>() {}
| ^^^^^^^^^^^^ found const parameter with type `u32`

error: aborting due to 5 previous errors

For more information about this error, try `rustc --explain E0053`.
9 changes: 4 additions & 5 deletions src/test/ui/const-generics/issues/issue-86820.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Regression test for the ICE described in #86820.

#![allow(unused,dead_code)]
#![allow(unused, dead_code)]
use std::ops::BitAnd;

const C: fn() = || is_set();
Expand All @@ -9,13 +9,12 @@ fn is_set() {
}

trait Bits {
fn bit<const I : u8>(self) -> bool;
//~^ NOTE: the const parameter `I` has type `usize`, but the declaration in trait `Bits::bit` has type `u8`
fn bit<const I: u8>(self) -> bool;
}

impl Bits for u8 {
fn bit<const I : usize>(self) -> bool {
//~^ ERROR: method `bit` has an incompatible const parameter type for trait [E0053]
fn bit<const I: usize>(self) -> bool {
//~^ ERROR: method `bit` has an incompatible generic parameter for trait: `Bits` [E0053]
let i = 1 << I;
let mask = u8::from(i);
mask & self == mask
Expand Down
21 changes: 11 additions & 10 deletions src/test/ui/const-generics/issues/issue-86820.stderr
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
error[E0053]: method `bit` has an incompatible const parameter type for trait
--> $DIR/issue-86820.rs:17:12
error[E0053]: method `bit` has an incompatible generic parameter for trait: `Bits`
--> $DIR/issue-86820.rs:16:12
|
LL | fn bit<const I : usize>(self) -> bool {
| ^^^^^^^^^^^^^^^
|
note: the const parameter `I` has type `usize`, but the declaration in trait `Bits::bit` has type `u8`
--> $DIR/issue-86820.rs:12:12
|
LL | fn bit<const I : u8>(self) -> bool;
| ^^^^^^^^^^^^
LL | trait Bits {
| ----
LL | fn bit<const I: u8>(self) -> bool;
| ----------- expected const parameter with type `u8`
...
LL | impl Bits for u8 {
| ----------------
LL | fn bit<const I: usize>(self) -> bool {
| ^^^^^^^^^^^^^^ found const parameter with type `usize`

error: aborting due to previous error

Expand Down
Loading