From 3e9f27dadfea2921649837c1a282ad159ceb8684 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 22 Jan 2021 08:06:01 +0100 Subject: [PATCH 1/7] Remove unnecessary closure --- compiler/rustc_typeck/src/check/check.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index 8e339eb26b26c..279c4422fe129 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -932,11 +932,11 @@ pub(super) fn check_impl_items_against_trait<'tcx>( // Locate trait definition and items let trait_def = tcx.trait_def(impl_trait_ref.def_id); - let impl_items = || impl_item_refs.iter().map(|iiref| tcx.hir().impl_item(iiref.id)); + let impl_items = impl_item_refs.iter().map(|iiref| tcx.hir().impl_item(iiref.id)); // Check existing impl methods to see if they are both present in trait // and compatible with trait signature - for impl_item in impl_items() { + for impl_item in impl_items { let namespace = impl_item.kind.namespace(); let ty_impl_item = tcx.associated_item(tcx.hir().local_def_id(impl_item.hir_id)); let ty_trait_item = tcx From 58a90de9adfac7cc26fe178b3f736153b756a515 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 22 Jan 2021 08:14:25 +0100 Subject: [PATCH 2/7] No peeking --- compiler/rustc_typeck/src/check/check.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index 279c4422fe129..e89a063598ab7 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -854,13 +854,7 @@ pub(super) fn check_specialization_validity<'tcx>( } else { Some((parent, parent.item(tcx, trait_item.ident, kind, trait_def.def_id))) } - }) - .peekable(); - - if ancestor_impls.peek().is_none() { - // No parent, nothing to specialize. - return; - } + }); let opt_result = ancestor_impls.find_map(|(parent_impl, parent_item)| { match parent_item { From 9988821a04073360c27bd95b9535a8dceca67d9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 22 Jan 2021 08:22:15 +0100 Subject: [PATCH 3/7] Move missing_item check inside condition --- compiler/rustc_typeck/src/check/check.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index e89a063598ab7..ace2ccb2fceac 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -1043,9 +1043,9 @@ pub(super) fn check_impl_items_against_trait<'tcx>( } } - // Check for missing items from trait - let mut missing_items = Vec::new(); if let Ok(ancestors) = trait_def.ancestors(tcx, impl_id.to_def_id()) { + // Check for missing items from trait + let mut missing_items = Vec::new(); for trait_item in tcx.associated_items(impl_trait_ref.def_id).in_definition_order() { let is_implemented = ancestors .leaf_def(tcx, trait_item.ident, trait_item.kind) @@ -1058,10 +1058,10 @@ pub(super) fn check_impl_items_against_trait<'tcx>( } } } - } - if !missing_items.is_empty() { - missing_items_err(tcx, impl_span, &missing_items, full_impl_span); + if !missing_items.is_empty() { + missing_items_err(tcx, impl_span, &missing_items, full_impl_span); + } } } From d63b278c2ff01b73653397a5e2c469689ef0adf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 22 Jan 2021 18:15:55 +0100 Subject: [PATCH 4/7] Only scan through assoc items once in check_impl_items_against_trait --- compiler/rustc_typeck/src/check/check.rs | 215 ++++++++++++----------- 1 file changed, 117 insertions(+), 98 deletions(-) diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index ace2ccb2fceac..eda5a27e97e3a 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -846,15 +846,13 @@ pub(super) fn check_specialization_validity<'tcx>( Ok(ancestors) => ancestors, Err(_) => return, }; - let mut ancestor_impls = ancestors - .skip(1) - .filter_map(|parent| { - if parent.is_from_trait() { - None - } else { - Some((parent, parent.item(tcx, trait_item.ident, kind, trait_def.def_id))) - } - }); + let mut ancestor_impls = ancestors.skip(1).filter_map(|parent| { + if parent.is_from_trait() { + None + } else { + Some((parent, parent.item(tcx, trait_item.ident, kind, trait_def.def_id))) + } + }); let opt_result = ancestor_impls.find_map(|(parent_impl, parent_item)| { match parent_item { @@ -931,105 +929,72 @@ pub(super) fn check_impl_items_against_trait<'tcx>( // Check existing impl methods to see if they are both present in trait // and compatible with trait signature for impl_item in impl_items { - let namespace = impl_item.kind.namespace(); let ty_impl_item = tcx.associated_item(tcx.hir().local_def_id(impl_item.hir_id)); - let ty_trait_item = tcx - .associated_items(impl_trait_ref.def_id) - .find_by_name_and_namespace(tcx, ty_impl_item.ident, namespace, impl_trait_ref.def_id) - .or_else(|| { - // Not compatible, but needed for the error message - tcx.associated_items(impl_trait_ref.def_id) - .filter_by_name(tcx, ty_impl_item.ident, impl_trait_ref.def_id) - .next() - }); - - // Check that impl definition matches trait definition - if let Some(ty_trait_item) = ty_trait_item { + let associated_items = tcx.associated_items(impl_trait_ref.def_id); + + let mut items = associated_items.filter_by_name(tcx, ty_impl_item.ident, impl_trait_ref.def_id); + + let (compatible_kind, ty_trait_item) = if let Some(ty_trait_item) = items.next() { + + let is_compatible = |ty: &&ty::AssocItem| { + match (ty.kind, &impl_item.kind) { + (ty::AssocKind::Const, hir::ImplItemKind::Const(..)) => true, + (ty::AssocKind::Fn, hir::ImplItemKind::Fn(..)) => true, + (ty::AssocKind::Type, hir::ImplItemKind::TyAlias(..)) => true, + _ => false + } + }; + + // If we don't have a compatible item, we'll use the first one whose name matches + // to report an error. + let mut compatible_kind = is_compatible(&ty_trait_item); + let mut trait_item = ty_trait_item; + + if !compatible_kind { + if let Some(ty_trait_item) = items.find(is_compatible) { + compatible_kind = true; + trait_item = ty_trait_item; + } + } + + (compatible_kind, trait_item) + } else { + continue; + }; + + if compatible_kind { match impl_item.kind { hir::ImplItemKind::Const(..) => { // Find associated const definition. - if ty_trait_item.kind == ty::AssocKind::Const { - compare_const_impl( - tcx, - &ty_impl_item, - impl_item.span, - &ty_trait_item, - impl_trait_ref, - ); - } else { - let mut err = struct_span_err!( - tcx.sess, - impl_item.span, - E0323, - "item `{}` is an associated const, \ - which doesn't match its trait `{}`", - ty_impl_item.ident, - impl_trait_ref.print_only_trait_path() - ); - err.span_label(impl_item.span, "does not match trait"); - // We can only get the spans from local trait definition - // Same for E0324 and E0325 - if let Some(trait_span) = tcx.hir().span_if_local(ty_trait_item.def_id) { - err.span_label(trait_span, "item in trait"); - } - err.emit() - } + compare_const_impl( + tcx, + &ty_impl_item, + impl_item.span, + &ty_trait_item, + impl_trait_ref, + ); } hir::ImplItemKind::Fn(..) => { let opt_trait_span = tcx.hir().span_if_local(ty_trait_item.def_id); - if ty_trait_item.kind == ty::AssocKind::Fn { - compare_impl_method( - tcx, - &ty_impl_item, - impl_item.span, - &ty_trait_item, - impl_trait_ref, - opt_trait_span, - ); - } else { - let mut err = struct_span_err!( - tcx.sess, - impl_item.span, - E0324, - "item `{}` is an associated method, \ - which doesn't match its trait `{}`", - ty_impl_item.ident, - impl_trait_ref.print_only_trait_path() - ); - err.span_label(impl_item.span, "does not match trait"); - if let Some(trait_span) = opt_trait_span { - err.span_label(trait_span, "item in trait"); - } - err.emit() - } + compare_impl_method( + tcx, + &ty_impl_item, + impl_item.span, + &ty_trait_item, + impl_trait_ref, + opt_trait_span, + ); } hir::ImplItemKind::TyAlias(_) => { let opt_trait_span = tcx.hir().span_if_local(ty_trait_item.def_id); - if ty_trait_item.kind == ty::AssocKind::Type { - compare_ty_impl( - tcx, - &ty_impl_item, - impl_item.span, - &ty_trait_item, - impl_trait_ref, - opt_trait_span, - ); - } else { - let mut err = struct_span_err!( - tcx.sess, - impl_item.span, - E0325, - "item `{}` is an associated type, \ - which doesn't match its trait `{}`", - ty_impl_item.ident, - impl_trait_ref.print_only_trait_path() - ); - err.span_label(impl_item.span, "does not match trait"); - if let Some(trait_span) = opt_trait_span { - err.span_label(trait_span, "item in trait"); - } - err.emit() - } + compare_ty_impl( + tcx, + &ty_impl_item, + impl_item.span, + &ty_trait_item, + impl_trait_ref, + opt_trait_span, + ); } } @@ -1040,6 +1005,8 @@ pub(super) fn check_impl_items_against_trait<'tcx>( impl_id.to_def_id(), impl_item, ); + } else { + report_mismatch_error(tcx, ty_trait_item.def_id, impl_trait_ref, impl_item, &ty_impl_item); } } @@ -1065,6 +1032,58 @@ pub(super) fn check_impl_items_against_trait<'tcx>( } } +#[inline(never)] +#[cold] +fn report_mismatch_error<'tcx>( + tcx: TyCtxt<'tcx>, + trait_item_def_id: DefId, + impl_trait_ref: ty::TraitRef<'tcx>, + impl_item: &hir::ImplItem<'_>, + ty_impl_item: &ty::AssocItem, +) { + let mut err = match impl_item.kind { + hir::ImplItemKind::Const(..) => { + // Find associated const definition. + struct_span_err!( + tcx.sess, + impl_item.span, + E0323, + "item `{}` is an associated const, which doesn't match its trait `{}`", + ty_impl_item.ident, + impl_trait_ref.print_only_trait_path() + ) + } + + hir::ImplItemKind::Fn(..) => { + struct_span_err!( + tcx.sess, + impl_item.span, + E0324, + "item `{}` is an associated method, which doesn't match its trait `{}`", + ty_impl_item.ident, + impl_trait_ref.print_only_trait_path() + ) + } + + hir::ImplItemKind::TyAlias(_) => { + struct_span_err!( + tcx.sess, + impl_item.span, + E0325, + "item `{}` is an associated type, which doesn't match its trait `{}`", + ty_impl_item.ident, + impl_trait_ref.print_only_trait_path() + ) + } + }; + + err.span_label(impl_item.span, "does not match trait"); + if let Some(trait_span) = tcx.hir().span_if_local(trait_item_def_id) { + err.span_label(trait_span, "item in trait"); + } + err.emit(); +} + /// Checks whether a type can be represented in memory. In particular, it /// identifies types that contain themselves without indirection through a /// pointer, which would mean their size is unbounded. From ee639de007e952c006cee53278f9bc1fd773e7d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 22 Jan 2021 18:17:53 +0100 Subject: [PATCH 5/7] Only guess span if absolutely necessary --- compiler/rustc_typeck/src/check/check.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index eda5a27e97e3a..f937970aa552f 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -894,8 +894,6 @@ pub(super) fn check_impl_items_against_trait<'tcx>( impl_trait_ref: ty::TraitRef<'tcx>, impl_item_refs: &[hir::ImplItemRef<'_>], ) { - let impl_span = tcx.sess.source_map().guess_head_span(full_impl_span); - // If the trait reference itself is erroneous (so the compilation is going // to fail), skip checking the items here -- the `impl_item` table in `tcx` // isn't populated for such impls. @@ -1011,6 +1009,8 @@ pub(super) fn check_impl_items_against_trait<'tcx>( } if let Ok(ancestors) = trait_def.ancestors(tcx, impl_id.to_def_id()) { + let impl_span = tcx.sess.source_map().guess_head_span(full_impl_span); + // Check for missing items from trait let mut missing_items = Vec::new(); for trait_item in tcx.associated_items(impl_trait_ref.def_id).in_definition_order() { From aa4f5833e1e3dc468923be7be988eb9e25a9e4c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 22 Jan 2021 18:19:27 +0100 Subject: [PATCH 6/7] Only query associated_items once --- compiler/rustc_typeck/src/check/check.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index f937970aa552f..fa630bb2b52e3 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -921,14 +921,13 @@ pub(super) fn check_impl_items_against_trait<'tcx>( // Locate trait definition and items let trait_def = tcx.trait_def(impl_trait_ref.def_id); - let impl_items = impl_item_refs.iter().map(|iiref| tcx.hir().impl_item(iiref.id)); + let associated_items = tcx.associated_items(impl_trait_ref.def_id); // Check existing impl methods to see if they are both present in trait // and compatible with trait signature for impl_item in impl_items { let ty_impl_item = tcx.associated_item(tcx.hir().local_def_id(impl_item.hir_id)); - let associated_items = tcx.associated_items(impl_trait_ref.def_id); let mut items = associated_items.filter_by_name(tcx, ty_impl_item.ident, impl_trait_ref.def_id); @@ -1010,7 +1009,7 @@ pub(super) fn check_impl_items_against_trait<'tcx>( if let Ok(ancestors) = trait_def.ancestors(tcx, impl_id.to_def_id()) { let impl_span = tcx.sess.source_map().guess_head_span(full_impl_span); - + // Check for missing items from trait let mut missing_items = Vec::new(); for trait_item in tcx.associated_items(impl_trait_ref.def_id).in_definition_order() { From f29b32983d1e885b0e141b6aac2cae281ff39a22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 22 Jan 2021 18:55:37 +0100 Subject: [PATCH 7/7] Fix formatting --- compiler/rustc_typeck/src/check/check.rs | 26 ++++++++++++++---------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index fa630bb2b52e3..518090a710d3b 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -929,17 +929,15 @@ pub(super) fn check_impl_items_against_trait<'tcx>( for impl_item in impl_items { let ty_impl_item = tcx.associated_item(tcx.hir().local_def_id(impl_item.hir_id)); - let mut items = associated_items.filter_by_name(tcx, ty_impl_item.ident, impl_trait_ref.def_id); + let mut items = + associated_items.filter_by_name(tcx, ty_impl_item.ident, impl_trait_ref.def_id); let (compatible_kind, ty_trait_item) = if let Some(ty_trait_item) = items.next() { - - let is_compatible = |ty: &&ty::AssocItem| { - match (ty.kind, &impl_item.kind) { - (ty::AssocKind::Const, hir::ImplItemKind::Const(..)) => true, - (ty::AssocKind::Fn, hir::ImplItemKind::Fn(..)) => true, - (ty::AssocKind::Type, hir::ImplItemKind::TyAlias(..)) => true, - _ => false - } + let is_compatible = |ty: &&ty::AssocItem| match (ty.kind, &impl_item.kind) { + (ty::AssocKind::Const, hir::ImplItemKind::Const(..)) => true, + (ty::AssocKind::Fn, hir::ImplItemKind::Fn(..)) => true, + (ty::AssocKind::Type, hir::ImplItemKind::TyAlias(..)) => true, + _ => false, }; // If we don't have a compatible item, we'll use the first one whose name matches @@ -947,7 +945,7 @@ pub(super) fn check_impl_items_against_trait<'tcx>( let mut compatible_kind = is_compatible(&ty_trait_item); let mut trait_item = ty_trait_item; - if !compatible_kind { + if !compatible_kind { if let Some(ty_trait_item) = items.find(is_compatible) { compatible_kind = true; trait_item = ty_trait_item; @@ -1003,7 +1001,13 @@ pub(super) fn check_impl_items_against_trait<'tcx>( impl_item, ); } else { - report_mismatch_error(tcx, ty_trait_item.def_id, impl_trait_ref, impl_item, &ty_impl_item); + report_mismatch_error( + tcx, + ty_trait_item.def_id, + impl_trait_ref, + impl_item, + &ty_impl_item, + ); } }