Skip to content

Commit

Permalink
Rollup merge of rust-lang#57501 - petrochenkov:highvar, r=alexreg
Browse files Browse the repository at this point in the history
High priority resolutions for associated variants

In rust-lang#56225 variants were assigned lowest priority during name resolution to avoid crater run and potential breakage.

This PR changes the rules to give variants highest priority instead.
Some motivation:
- If variants (and their constructors) are treated as associated items, then they are obviously *inherent* associated items since they don't come from traits.
- Inherent associated items have higher priority during resolution than associated items from traits.
- The reason is that there is a way to disambiguate in favor of trait items (`<Type as Trait>::Ambiguous`), but there's no way to disambiguate in favor of inherent items, so they became unusable in case of ambiguities if they have low priority.
- It's technically problematic to fallback from associated types to anything until lazy normalization (?) is implemented.

Crater found some regressions from this change, but they are all in type positions, e.g.
```rust
fn f() -> Self::Ambiguos { ... } // Variant `Ambiguous` or associated type `Ambiguous`?
```
, so variants are not usable there right now, but they may become usable in the future if rust-lang/rfcs#2593 is accepted.
This PR keeps code like this successfully resolving, but introduces a future-compatibility lint `ambiguous_associated_items` that recommends rewriting it as `<Self as Trait>::Ambiguous`.
  • Loading branch information
Centril authored Jan 19, 2019
2 parents c0ad942 + 01d0ae9 commit b941f29
Show file tree
Hide file tree
Showing 11 changed files with 225 additions and 103 deletions.
7 changes: 7 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,12 @@ declare_lint! {
report_in_external_macro: true
}

declare_lint! {
pub AMBIGUOUS_ASSOCIATED_ITEMS,
Warn,
"ambiguous associated items"
}

/// Does nothing as a lint pass, but registers some `Lint`s
/// that are used by other parts of the compiler.
#[derive(Copy, Clone)]
Expand Down Expand Up @@ -433,6 +439,7 @@ impl LintPass for HardwiredLints {
parser::QUESTION_MARK_MACRO_SEP,
parser::ILL_FORMED_ATTRIBUTE_INPUT,
DEPRECATED_IN_FUTURE,
AMBIGUOUS_ASSOCIATED_ITEMS,
)
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
reference: "issue #57571 <https://github.com/rust-lang/rust/issues/57571>",
edition: None,
},
FutureIncompatibleInfo {
id: LintId::of(AMBIGUOUS_ASSOCIATED_ITEMS),
reference: "issue #57644 <https://github.com/rust-lang/rust/issues/57644>",
edition: None,
},
]);

// Register renamed and removed lints.
Expand Down
162 changes: 96 additions & 66 deletions src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use hir::HirVec;
use lint;
use middle::resolve_lifetime as rl;
use namespace::Namespace;
use rustc::lint::builtin::AMBIGUOUS_ASSOCIATED_ITEMS;
use rustc::traits;
use rustc::ty::{self, Ty, TyCtxt, ToPredicate, TypeFoldable};
use rustc::ty::{GenericParamDef, GenericParamDefKind};
Expand Down Expand Up @@ -1278,29 +1279,50 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
}

// Create a type from a path to an associated type.
// For a path `A::B::C::D`, `ty` and `ty_path_def` are the type and def for `A::B::C`
// For a path `A::B::C::D`, `qself_ty` and `qself_def` are the type and def for `A::B::C`
// and item_segment is the path segment for `D`. We return a type and a def for
// the whole path.
// Will fail except for `T::A` and `Self::A`; i.e., if `ty`/`ty_path_def` are not a type
// Will fail except for `T::A` and `Self::A`; i.e., if `qself_ty`/`qself_def` are not a type
// parameter or `Self`.
pub fn associated_path_def_to_ty(&self,
ref_id: ast::NodeId,
span: Span,
ty: Ty<'tcx>,
ty_path_def: Def,
item_segment: &hir::PathSegment)
-> (Ty<'tcx>, Def)
{
pub fn associated_path_to_ty(
&self,
ref_id: ast::NodeId,
span: Span,
qself_ty: Ty<'tcx>,
qself_def: Def,
assoc_segment: &hir::PathSegment,
permit_variants: bool,
) -> (Ty<'tcx>, Def) {
let tcx = self.tcx();
let assoc_name = item_segment.ident;
let assoc_ident = assoc_segment.ident;

debug!("associated_path_def_to_ty: {:?}::{}", ty, assoc_name);
debug!("associated_path_to_ty: {:?}::{}", qself_ty, assoc_ident);

self.prohibit_generics(slice::from_ref(item_segment));
self.prohibit_generics(slice::from_ref(assoc_segment));

// Check if we have an enum variant.
let mut variant_resolution = None;
if let ty::Adt(adt_def, _) = qself_ty.sty {
if adt_def.is_enum() {
let variant_def = adt_def.variants.iter().find(|vd| {
tcx.hygienic_eq(assoc_ident, vd.ident, adt_def.did)
});
if let Some(variant_def) = variant_def {
let def = Def::Variant(variant_def.did);
if permit_variants {
check_type_alias_enum_variants_enabled(tcx, span);
tcx.check_stability(variant_def.did, Some(ref_id), span);
return (qself_ty, def);
} else {
variant_resolution = Some(def);
}
}
}
}

// Find the type of the associated item, and the trait where the associated
// item is declared.
let bound = match (&ty.sty, ty_path_def) {
let bound = match (&qself_ty.sty, qself_def) {
(_, Def::SelfTy(Some(_), Some(impl_def_id))) => {
// `Self` in an impl of a trait -- we have a concrete self type and a
// trait reference.
Expand All @@ -1313,77 +1335,61 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
};

let candidates = traits::supertraits(tcx, ty::Binder::bind(trait_ref))
.filter(|r| self.trait_defines_associated_type_named(r.def_id(), assoc_name));
.filter(|r| self.trait_defines_associated_type_named(r.def_id(), assoc_ident));

match self.one_bound_for_assoc_type(candidates, "Self", assoc_name, span) {
match self.one_bound_for_assoc_type(candidates, "Self", assoc_ident, span) {
Ok(bound) => bound,
Err(ErrorReported) => return (tcx.types.err, Def::Err),
}
}
(&ty::Param(_), Def::SelfTy(Some(param_did), None)) |
(&ty::Param(_), Def::TyParam(param_did)) => {
match self.find_bound_for_assoc_item(param_did, assoc_name, span) {
match self.find_bound_for_assoc_item(param_did, assoc_ident, span) {
Ok(bound) => bound,
Err(ErrorReported) => return (tcx.types.err, Def::Err),
}
}
(&ty::Adt(adt_def, _substs), Def::Enum(_did)) => {
let ty_str = ty.to_string();
// Incorrect enum variant.
let mut err = tcx.sess.struct_span_err(
span,
&format!("no variant `{}` on enum `{}`", &assoc_name.as_str(), ty_str),
);
// Check if it was a typo.
let input = adt_def.variants.iter().map(|variant| &variant.ident.name);
if let Some(suggested_name) = find_best_match_for_name(
input,
&assoc_name.as_str(),
None,
) {
err.span_suggestion_with_applicability(
_ => {
if variant_resolution.is_some() {
// Variant in type position
let msg = format!("expected type, found variant `{}`", assoc_ident);
tcx.sess.span_err(span, &msg);
} else if qself_ty.is_enum() {
// Report as incorrect enum variant rather than ambiguous type.
let mut err = tcx.sess.struct_span_err(
span,
"did you mean",
format!("{}::{}", ty_str, suggested_name.to_string()),
Applicability::MaybeIncorrect,
&format!("no variant `{}` on enum `{}`", &assoc_ident.as_str(), qself_ty),
);
} else {
err.span_label(span, "unknown variant");
}
err.emit();
return (tcx.types.err, Def::Err);
}
_ => {
// Check if we have an enum variant.
match ty.sty {
ty::Adt(adt_def, _) if adt_def.is_enum() => {
let variant_def = adt_def.variants.iter().find(|vd| {
tcx.hygienic_eq(assoc_name, vd.ident, adt_def.did)
});
if let Some(variant_def) = variant_def {
check_type_alias_enum_variants_enabled(tcx, span);

let def = Def::Variant(variant_def.did);
tcx.check_stability(def.def_id(), Some(ref_id), span);
return (ty, def);
}
},
_ => (),
}

// Don't print `TyErr` to the user.
if !ty.references_error() {
// Check if it was a typo.
let adt_def = qself_ty.ty_adt_def().expect("enum is not an ADT");
if let Some(suggested_name) = find_best_match_for_name(
adt_def.variants.iter().map(|variant| &variant.ident.name),
&assoc_ident.as_str(),
None,
) {
err.span_suggestion_with_applicability(
span,
"did you mean",
format!("{}::{}", qself_ty, suggested_name),
Applicability::MaybeIncorrect,
);
} else {
err.span_label(span, "unknown variant");
}
err.emit();
} else if !qself_ty.references_error() {
// Don't print `TyErr` to the user.
self.report_ambiguous_associated_type(span,
&ty.to_string(),
&qself_ty.to_string(),
"Trait",
&assoc_name.as_str());
&assoc_ident.as_str());
}
return (tcx.types.err, Def::Err);
}
};

let trait_did = bound.def_id();
let (assoc_ident, def_scope) = tcx.adjust_ident(assoc_name, trait_did, ref_id);
let (assoc_ident, def_scope) = tcx.adjust_ident(assoc_ident, trait_did, ref_id);
let item = tcx.associated_items(trait_did).find(|i| {
Namespace::from(i.kind) == Namespace::Type &&
i.ident.modern() == assoc_ident
Expand All @@ -1394,11 +1400,35 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {

let def = Def::AssociatedTy(item.def_id);
if !item.vis.is_accessible_from(def_scope, tcx) {
let msg = format!("{} `{}` is private", def.kind_name(), assoc_name);
let msg = format!("{} `{}` is private", def.kind_name(), assoc_ident);
tcx.sess.span_err(span, &msg);
}
tcx.check_stability(item.def_id, Some(ref_id), span);

if let Some(variant_def) = variant_resolution {
let mut err = tcx.struct_span_lint_node(
AMBIGUOUS_ASSOCIATED_ITEMS,
ref_id,
span,
"ambiguous associated item",
);

let mut could_refer_to = |def: Def, also| {
let note_msg = format!("`{}` could{} refer to {} defined here",
assoc_ident, also, def.kind_name());
err.span_note(tcx.def_span(def.def_id()), &note_msg);
};
could_refer_to(variant_def, "");
could_refer_to(def, " also");

err.span_suggestion_with_applicability(
span,
"use fully-qualified syntax",
format!("<{} as {}>::{}", qself_ty, "Trait", assoc_ident),
Applicability::HasPlaceholders,
).emit();
}

(ty, def)
}

Expand Down Expand Up @@ -1773,7 +1803,7 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
} else {
Def::Err
};
self.associated_path_def_to_ty(ast_ty.id, ast_ty.span, ty, def, segment).0
self.associated_path_to_ty(ast_ty.id, ast_ty.span, ty, def, segment, false).0
}
hir::TyKind::Array(ref ty, ref length) => {
let length_def_id = tcx.hir().local_def_id(length.id);
Expand Down
61 changes: 26 additions & 35 deletions src/librustc_typeck/check/method/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,45 +408,36 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {

let tcx = self.tcx;

let mode = probe::Mode::Path;
match self.probe_for_name(span, mode, method_name, IsSuggestion(false),
self_ty, expr_id, ProbeScope::TraitsInScope) {
Ok(pick) => {
debug!("resolve_ufcs: pick={:?}", pick);
if let Some(import_id) = pick.import_id {
let import_def_id = tcx.hir().local_def_id(import_id);
debug!("resolve_ufcs: used_trait_import: {:?}", import_def_id);
Lrc::get_mut(&mut self.tables.borrow_mut().used_trait_imports)
.unwrap().insert(import_def_id);
// Check if we have an enum variant.
if let ty::Adt(adt_def, _) = self_ty.sty {
if adt_def.is_enum() {
let variant_def = adt_def.variants.iter().find(|vd| {
tcx.hygienic_eq(method_name, vd.ident, adt_def.did)
});
if let Some(variant_def) = variant_def {
check_type_alias_enum_variants_enabled(tcx, span);

let def = Def::VariantCtor(variant_def.did, variant_def.ctor_kind);
tcx.check_stability(def.def_id(), Some(expr_id), span);
return Ok(def);
}

let def = pick.item.def();
debug!("resolve_ufcs: def={:?}", def);
tcx.check_stability(def.def_id(), Some(expr_id), span);

Ok(def)
}
Err(err) => {
// Check if we have an enum variant.
match self_ty.sty {
ty::Adt(adt_def, _) if adt_def.is_enum() => {
let variant_def = adt_def.variants.iter().find(|vd| {
tcx.hygienic_eq(method_name, vd.ident, adt_def.did)
});
if let Some(variant_def) = variant_def {
check_type_alias_enum_variants_enabled(tcx, span);

let def = Def::VariantCtor(variant_def.did, variant_def.ctor_kind);
tcx.check_stability(def.def_id(), Some(expr_id), span);
return Ok(def);
}
},
_ => (),
}
}

Err(err)
}
let pick = self.probe_for_name(span, probe::Mode::Path, method_name, IsSuggestion(false),
self_ty, expr_id, ProbeScope::TraitsInScope)?;
debug!("resolve_ufcs: pick={:?}", pick);
if let Some(import_id) = pick.import_id {
let import_def_id = tcx.hir().local_def_id(import_id);
debug!("resolve_ufcs: used_trait_import: {:?}", import_def_id);
Lrc::get_mut(&mut self.tables.borrow_mut().used_trait_imports)
.unwrap().insert(import_def_id);
}

let def = pick.item.def();
debug!("resolve_ufcs: def={:?}", def);
tcx.check_stability(def.def_id(), Some(expr_id), span);
Ok(def)
}

/// Find item with name `item_name` defined in impl/trait `def_id`
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4724,8 +4724,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
} else {
Def::Err
};
let (ty, def) = AstConv::associated_path_def_to_ty(self, node_id, path_span,
ty, def, segment);
let (ty, def) = AstConv::associated_path_to_ty(self, node_id, path_span,
ty, def, segment, true);

// Write back the new resolution.
let hir_id = self.tcx.hir().node_to_hir_id(node_id);
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/type-alias-enum-variants-priority-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![feature(type_alias_enum_variants)]

enum E {
V(u8)
}

impl E {
fn V() {}
}

fn main() {
<E>::V(); //~ ERROR this function takes 1 parameter but 0 parameters were supplied
}
12 changes: 12 additions & 0 deletions src/test/ui/type-alias-enum-variants-priority-2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0061]: this function takes 1 parameter but 0 parameters were supplied
--> $DIR/type-alias-enum-variants-priority-2.rs:12:5
|
LL | V(u8)
| ----- defined here
...
LL | <E>::V(); //~ ERROR this function takes 1 parameter but 0 parameters were supplied
| ^^^^^^^^ expected 1 parameter

error: aborting due to previous error

For more information about this error, try `rustc --explain E0061`.
10 changes: 10 additions & 0 deletions src/test/ui/type-alias-enum-variants-priority-3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#![feature(type_alias_enum_variants)]

enum E {
V
}

fn check() -> <E>::V {}
//~^ ERROR expected type, found variant `V`

fn main() {}
8 changes: 8 additions & 0 deletions src/test/ui/type-alias-enum-variants-priority-3.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: expected type, found variant `V`
--> $DIR/type-alias-enum-variants-priority-3.rs:7:15
|
LL | fn check() -> <E>::V {}
| ^^^^^^

error: aborting due to previous error

Loading

0 comments on commit b941f29

Please sign in to comment.