Skip to content

Commit 80adfd8

Browse files
estebankestebank
committed
Account for multiple trait bounds with missing associated types
1 parent 83c5b4e commit 80adfd8

File tree

6 files changed

+367
-146
lines changed

6 files changed

+367
-146
lines changed

‎src/librustc_typeck/astconv.rs

Lines changed: 191 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -1444,59 +1444,68 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
14441444
}
14451445

14461446
// Use a `BTreeSet` to keep output in a more consistent order.
1447-
let mut associated_types = BTreeSet::default();
1447+
let mut associated_types: FxHashMap<Span, BTreeSet<DefId>> = FxHashMap::default();
14481448

1449-
let regular_traits_refs = bounds
1449+
let regular_traits_refs_spans = bounds
14501450
.trait_bounds
14511451
.into_iter()
1452-
.filter(|(trait_ref, _)| !tcx.trait_is_auto(trait_ref.def_id()))
1453-
.map(|(trait_ref, _)| trait_ref);
1454-
for trait_ref in traits::elaborate_trait_refs(tcx, regular_traits_refs) {
1455-
debug!("conv_object_ty_poly_trait_ref: observing object predicate `{:?}`", trait_ref);
1456-
match trait_ref {
1457-
ty::Predicate::Trait(pred) => {
1458-
associated_types.extend(
1459-
tcx.associated_items(pred.def_id())
1460-
.filter(|item| item.kind == ty::AssocKind::Type)
1461-
.map(|item| item.def_id),
1462-
);
1463-
}
1464-
ty::Predicate::Projection(pred) => {
1465-
// A `Self` within the original bound will be substituted with a
1466-
// `trait_object_dummy_self`, so check for that.
1467-
let references_self = pred.skip_binder().ty.walk().any(|t| t == dummy_self);
1468-
1469-
// If the projection output contains `Self`, force the user to
1470-
// elaborate it explicitly to avoid a lot of complexity.
1471-
//
1472-
// The "classicaly useful" case is the following:
1473-
// ```
1474-
// trait MyTrait: FnMut() -> <Self as MyTrait>::MyOutput {
1475-
// type MyOutput;
1476-
// }
1477-
// ```
1478-
//
1479-
// Here, the user could theoretically write `dyn MyTrait<Output = X>`,
1480-
// but actually supporting that would "expand" to an infinitely-long type
1481-
// `fix $ τ → dyn MyTrait<MyOutput = X, Output = <τ as MyTrait>::MyOutput`.
1482-
//
1483-
// Instead, we force the user to write `dyn MyTrait<MyOutput = X, Output = X>`,
1484-
// which is uglier but works. See the discussion in #56288 for alternatives.
1485-
if !references_self {
1486-
// Include projections defined on supertraits.
1487-
bounds.projection_bounds.push((pred, DUMMY_SP))
1452+
.filter(|(trait_ref, _)| !tcx.trait_is_auto(trait_ref.def_id()));
1453+
1454+
for (base_trait_ref, span) in regular_traits_refs_spans {
1455+
debug!("conv_object_ty_poly_trait_ref regular_trait_ref `{:?}`", base_trait_ref);
1456+
let mut new_bounds = vec![];
1457+
for trait_ref in traits::elaborate_trait_ref(tcx, base_trait_ref) {
1458+
debug!(
1459+
"conv_object_ty_poly_trait_ref: observing object predicate `{:?}`",
1460+
trait_ref
1461+
);
1462+
match trait_ref {
1463+
ty::Predicate::Trait(pred) => {
1464+
associated_types.entry(span).or_default().extend(
1465+
tcx.associated_items(pred.def_id())
1466+
.filter(|item| item.kind == ty::AssocKind::Type)
1467+
.map(|item| item.def_id),
1468+
);
14881469
}
1470+
ty::Predicate::Projection(pred) => {
1471+
// A `Self` within the original bound will be substituted with a
1472+
// `trait_object_dummy_self`, so check for that.
1473+
let references_self = pred.skip_binder().ty.walk().any(|t| t == dummy_self);
1474+
1475+
// If the projection output contains `Self`, force the user to
1476+
// elaborate it explicitly to avoid a lot of complexity.
1477+
//
1478+
// The "classicaly useful" case is the following:
1479+
// ```
1480+
// trait MyTrait: FnMut() -> <Self as MyTrait>::MyOutput {
1481+
// type MyOutput;
1482+
// }
1483+
// ```
1484+
//
1485+
// Here, the user could theoretically write `dyn MyTrait<Output = X>`,
1486+
// but actually supporting that would "expand" to an infinitely-long type
1487+
// `fix $ τ → dyn MyTrait<MyOutput = X, Output = <τ as MyTrait>::MyOutput`.
1488+
//
1489+
// Instead, we force the user to write `dyn MyTrait<MyOutput = X, Output = X>`,
1490+
// which is uglier but works. See the discussion in #56288 for alternatives.
1491+
if !references_self {
1492+
// Include projections defined on supertraits.
1493+
new_bounds.push((pred, span));
1494+
}
1495+
}
1496+
_ => (),
14891497
}
1490-
_ => (),
14911498
}
1499+
bounds.projection_bounds.extend(new_bounds);
14921500
}
14931501

14941502
for (projection_bound, _) in &bounds.projection_bounds {
1495-
associated_types.remove(&projection_bound.projection_def_id());
1503+
for (_, def_ids) in &mut associated_types {
1504+
def_ids.remove(&projection_bound.projection_def_id());
1505+
}
14961506
}
14971507

14981508
self.complain_about_missing_associated_types(
1499-
span,
15001509
associated_types,
15011510
potential_assoc_types,
15021511
trait_bounds,
@@ -1591,134 +1600,183 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
15911600

15921601
fn complain_about_missing_associated_types(
15931602
&self,
1594-
span: Span,
1595-
associated_types: BTreeSet<DefId>,
1603+
mut associated_types: FxHashMap<Span, BTreeSet<DefId>>,
15961604
potential_assoc_types: Vec<Span>,
15971605
trait_bounds: &[hir::PolyTraitRef],
15981606
) {
1599-
if associated_types.is_empty() {
1607+
if !associated_types.values().any(|v| v.len() > 0) {
16001608
return;
16011609
}
1602-
// Account for things like `dyn Foo + 'a` by pointing at the `TraitRef.path`
1603-
// `Span` instead of the `PolyTraitRef` `Span`. That way the suggestion will
1604-
// be valid, otherwise we would suggest `dyn Foo + 'a<A = Type>`. See tests
1605-
// `issue-22434.rs` and `issue-22560.rs` for examples.
1606-
let sugg_span = match (&potential_assoc_types[..], &trait_bounds) {
1610+
let tcx = self.tcx();
1611+
let mut names = vec![];
1612+
1613+
// Account for things like `dyn Foo + 'a`, like in tests `issue-22434.rs` and
1614+
// `issue-22560.rs`.
1615+
let mut trait_bound_spans: Vec<Span> = vec![];
1616+
for (span, item_def_ids) in &associated_types {
1617+
if !item_def_ids.is_empty() {
1618+
trait_bound_spans.push(*span);
1619+
}
1620+
for item_def_id in item_def_ids {
1621+
let assoc_item = tcx.associated_item(*item_def_id);
1622+
let trait_def_id = assoc_item.container.id();
1623+
names.push(format!(
1624+
"`{}` (from trait `{}`)",
1625+
assoc_item.ident,
1626+
tcx.def_path_str(trait_def_id),
1627+
));
1628+
}
1629+
}
1630+
1631+
match (&potential_assoc_types[..], &trait_bounds) {
16071632
([], [bound]) => match &bound.trait_ref.path.segments[..] {
16081633
// FIXME: `trait_ref.path.span` can point to a full path with multiple
16091634
// segments, even though `trait_ref.path.segments` is of length `1`. Work
16101635
// around that bug here, even though it should be fixed elsewhere.
16111636
// This would otherwise cause an invalid suggestion. For an example, look at
1612-
// `src/test/ui/issues/issue-28344.rs`.
1613-
[segment] if segment.args.is_none() => segment.ident.span,
1614-
_ => bound.trait_ref.path.span,
1637+
// `src/test/ui/issues/issue-28344.rs` where instead of the following:
1638+
//
1639+
// error[E0191]: the value of the associated type `Output`
1640+
// (from trait `std::ops::BitXor`) must be specified
1641+
// --> $DIR/issue-28344.rs:4:17
1642+
// |
1643+
// LL | let x: u8 = BitXor::bitor(0 as u8, 0 as u8);
1644+
// | ^^^^^^ help: specify the associated type:
1645+
// | `BitXor<Output = Type>`
1646+
//
1647+
// we would output:
1648+
//
1649+
// error[E0191]: the value of the associated type `Output`
1650+
// (from trait `std::ops::BitXor`) must be specified
1651+
// --> $DIR/issue-28344.rs:4:17
1652+
// |
1653+
// LL | let x: u8 = BitXor::bitor(0 as u8, 0 as u8);
1654+
// | ^^^^^^^^^^^^^ help: specify the associated type:
1655+
// | `BitXor::bitor<Output = Type>`
1656+
[segment] if segment.args.is_none() => {
1657+
trait_bound_spans = vec![segment.ident.span];
1658+
associated_types = associated_types
1659+
.into_iter()
1660+
.map(|(_, defs)| (segment.ident.span, defs))
1661+
.collect();
1662+
}
1663+
_ => {}
16151664
},
1616-
_ => span,
1617-
};
1618-
let tcx = self.tcx();
1619-
let names = associated_types
1620-
.iter()
1621-
.map(|item_def_id| {
1622-
let assoc_item = tcx.associated_item(*item_def_id);
1623-
let trait_def_id = assoc_item.container.id();
1624-
format!("`{}` (from trait `{}`)", assoc_item.ident, tcx.def_path_str(trait_def_id))
1625-
})
1626-
.collect::<Vec<_>>()
1627-
.join(", ");
1665+
_ => {}
1666+
}
16281667
let mut err = struct_span_err!(
16291668
tcx.sess,
1630-
sugg_span,
1669+
trait_bound_spans,
16311670
E0191,
16321671
"the value of the associated type{} {} must be specified",
1633-
pluralize!(associated_types.len()),
1634-
names,
1672+
pluralize!(names.len()),
1673+
names.join(", "),
16351674
);
1636-
let mut suggestions = Vec::new();
1637-
let mut applicability = Applicability::MaybeIncorrect;
1638-
for (i, item_def_id) in associated_types.iter().enumerate() {
1639-
let assoc_item = tcx.associated_item(*item_def_id);
1640-
if let Some(sp) = tcx.hir().span_if_local(*item_def_id) {
1641-
err.span_label(sp, format!("`{}` defined here", assoc_item.ident));
1642-
}
1643-
if potential_assoc_types.len() == associated_types.len() {
1675+
let mut suggestions = vec![];
1676+
let mut types_count = 0;
1677+
let mut where_constraints = vec![];
1678+
for (span, def_ids) in &associated_types {
1679+
let assoc_items: Vec<_> =
1680+
def_ids.iter().map(|def_id| tcx.associated_item(*def_id)).collect();
1681+
let mut names: FxHashMap<_, usize> = FxHashMap::default();
1682+
for item in &assoc_items {
1683+
types_count += 1;
1684+
*names.entry(item.ident.name).or_insert(0) += 1;
1685+
}
1686+
let mut dupes = false;
1687+
for item in &assoc_items {
1688+
let prefix = if names[&item.ident.name] > 1 {
1689+
let trait_def_id = item.container.id();
1690+
dupes = true;
1691+
format!("{}::", tcx.def_path_str(trait_def_id))
1692+
} else {
1693+
String::new()
1694+
};
1695+
if let Some(sp) = tcx.hir().span_if_local(item.def_id) {
1696+
err.span_label(sp, format!("`{}{}` defined here", prefix, item.ident));
1697+
}
1698+
}
1699+
if potential_assoc_types.len() == assoc_items.len() {
16441700
// Only suggest when the amount of missing associated types equals the number of
16451701
// extra type arguments present, as that gives us a relatively high confidence
16461702
// that the user forgot to give the associtated type's name. The canonical
16471703
// example would be trying to use `Iterator<isize>` instead of
16481704
// `Iterator<Item = isize>`.
1649-
if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(potential_assoc_types[i])
1650-
{
1651-
suggestions.push((
1652-
potential_assoc_types[i],
1653-
format!("{} = {}", assoc_item.ident, snippet),
1654-
));
1705+
for (potential, item) in potential_assoc_types.iter().zip(assoc_items.iter()) {
1706+
if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(*potential) {
1707+
suggestions.push((*potential, format!("{} = {}", item.ident, snippet)));
1708+
}
16551709
}
1656-
}
1657-
}
1658-
let mut suggestions_len = suggestions.len();
1659-
if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(sugg_span) {
1660-
let assoc_types: Vec<String> = associated_types
1661-
.iter()
1662-
.map(|item_def_id| {
1663-
let assoc_item = tcx.associated_item(*item_def_id);
1664-
format!("{} = Type", assoc_item.ident)
1665-
})
1666-
.collect();
1667-
let dedup = assoc_types.clone().drain(..).collect::<FxHashSet<_>>();
1668-
1669-
if dedup.len() != assoc_types.len() && trait_bounds.len() == 1 {
1670-
// If there are duplicates associated type names and a single trait bound do not
1671-
// use structured suggestion, it means that there are multiple super-traits with
1672-
// the same associated type name.
1673-
err.help(
1674-
"consider introducing a new type parameter, adding `where` constraints \
1675-
using the fully-qualified path to the associated type",
1676-
);
1677-
} else if dedup.len() == assoc_types.len() &&
1678-
potential_assoc_types.is_empty() &&
1679-
trait_bounds.len() == 1 &&
1680-
// Do not attempt to suggest when we don't know which path segment needs the
1681-
// type parameter set.
1682-
trait_bounds[0].trait_ref.path.segments.len() == 1
1710+
} else if let (Ok(snippet), false) =
1711+
(tcx.sess.source_map().span_to_snippet(*span), dupes)
16831712
{
1684-
applicability = Applicability::HasPlaceholders;
1685-
let sugg = assoc_types.join(", ");
1686-
if snippet.ends_with('>') {
1713+
let types: Vec<_> =
1714+
assoc_items.iter().map(|item| format!("{} = Type", item.ident)).collect();
1715+
let code = if snippet.ends_with(">") {
16871716
// The user wrote `Trait<'a>` or similar and we don't have a type we can
16881717
// suggest, but at least we can clue them to the correct syntax
16891718
// `Trait<'a, Item = Type>` while accounting for the `<'a>` in the
16901719
// suggestion.
1691-
suggestions.push((
1692-
sugg_span,
1693-
format!("{}, {}>", &snippet[..snippet.len() - 1], sugg,),
1694-
));
1720+
format!("{}, {}>", &snippet[..snippet.len() - 1], types.join(", "))
16951721
} else {
16961722
// The user wrote `Iterator`, so we don't have a type we can suggest, but at
16971723
// least we can clue them to the correct syntax `Iterator<Item = Type>`.
1698-
suggestions.push((sugg_span, format!("{}<{}>", snippet, sugg)));
1699-
}
1700-
suggestions_len = assoc_types.len();
1724+
format!("{}<{}>", snippet, types.join(", "))
1725+
};
1726+
suggestions.push((*span, code));
1727+
} else if dupes {
1728+
where_constraints.push(*span);
17011729
}
17021730
}
1731+
let where_msg = "consider introducing a new type parameter, adding `where` constraints \
1732+
using the fully-qualified path to the associated types";
1733+
if !where_constraints.is_empty() && suggestions.is_empty() {
1734+
// If there are duplicates associated type names and a single trait bound do not
1735+
// use structured suggestion, it means that there are multiple super-traits with
1736+
// the same associated type name.
1737+
err.help(where_msg);
1738+
}
17031739
if suggestions.len() != 1 {
17041740
// We don't need this label if there's an inline suggestion, show otherwise.
1705-
let names = associated_types
1706-
.iter()
1707-
.map(|t| format!("`{}`", tcx.associated_item(*t).ident))
1708-
.collect::<Vec<_>>()
1709-
.join(", ");
1710-
err.span_label(
1711-
sugg_span,
1712-
format!(
1713-
"associated type{} {} must be specified",
1714-
pluralize!(associated_types.len()),
1715-
names,
1716-
),
1717-
);
1741+
for (span, def_ids) in &associated_types {
1742+
let assoc_items: Vec<_> =
1743+
def_ids.iter().map(|def_id| tcx.associated_item(*def_id)).collect();
1744+
let mut names: FxHashMap<_, usize> = FxHashMap::default();
1745+
for item in &assoc_items {
1746+
types_count += 1;
1747+
*names.entry(item.ident.name).or_insert(0) += 1;
1748+
}
1749+
let mut label = vec![];
1750+
for item in &assoc_items {
1751+
let postfix = if names[&item.ident.name] > 1 {
1752+
let trait_def_id = item.container.id();
1753+
format!(" (from trait `{}`)", tcx.def_path_str(trait_def_id))
1754+
} else {
1755+
String::new()
1756+
};
1757+
label.push(format!("`{}`{}", item.ident, postfix));
1758+
}
1759+
if !label.is_empty() {
1760+
err.span_label(
1761+
*span,
1762+
format!(
1763+
"associated type{} {} must be specified",
1764+
pluralize!(label.len()),
1765+
label.join(", "),
1766+
),
1767+
);
1768+
}
1769+
}
17181770
}
17191771
if !suggestions.is_empty() {
1720-
let msg = format!("specify the associated type{}", pluralize!(suggestions_len));
1721-
err.multipart_suggestion(&msg, suggestions, applicability);
1772+
err.multipart_suggestion(
1773+
&format!("specify the associated type{}", pluralize!(types_count)),
1774+
suggestions,
1775+
Applicability::HasPlaceholders,
1776+
);
1777+
if !where_constraints.is_empty() {
1778+
err.span_help(where_constraints, where_msg);
1779+
}
17221780
}
17231781
err.emit();
17241782
}

0 commit comments

Comments
 (0)