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

More systematic error reporting in path resolution #38154

Merged
merged 2 commits into from
Dec 26, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 2 additions & 4 deletions src/librustc/middle/cstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,7 @@ pub trait CrateStore<'tcx> {

// trait/impl-item info
fn trait_of_item(&self, def_id: DefId) -> Option<DefId>;
fn associated_item<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId)
-> Option<ty::AssociatedItem>;
fn associated_item(&self, def: DefId) -> Option<ty::AssociatedItem>;

// flags
fn is_const_fn(&self, did: DefId) -> bool;
Expand Down Expand Up @@ -456,8 +455,7 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {

// trait/impl-item info
fn trait_of_item(&self, def_id: DefId) -> Option<DefId> { bug!("trait_of_item") }
fn associated_item<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId)
-> Option<ty::AssociatedItem> { bug!("associated_item") }
fn associated_item(&self, def: DefId) -> Option<ty::AssociatedItem> { bug!("associated_item") }

// flags
fn is_const_fn(&self, did: DefId) -> bool { bug!("is_const_fn") }
Expand Down
5 changes: 2 additions & 3 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2071,7 +2071,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub fn associated_item(self, def_id: DefId) -> AssociatedItem {
self.associated_items.memoize(def_id, || {
if !def_id.is_local() {
return self.sess.cstore.associated_item(self.global_tcx(), def_id)
return self.sess.cstore.associated_item(def_id)
.expect("missing AssociatedItem in metadata");
}

Expand Down Expand Up @@ -2526,8 +2526,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
/// ID of the impl that the method belongs to. Otherwise, return `None`.
pub fn impl_of_method(self, def_id: DefId) -> Option<DefId> {
if def_id.krate != LOCAL_CRATE {
return self.sess.cstore.associated_item(self.global_tcx(), def_id)
.and_then(|item| {
return self.sess.cstore.associated_item(def_id).and_then(|item| {
match item.container {
TraitContainer(_) => None,
ImplContainer(def_id) => Some(def_id),
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,7 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore {
self.get_crate_data(def_id.krate).get_trait_of_item(def_id.index)
}

fn associated_item<'a>(&self, _tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId)
-> Option<ty::AssociatedItem>
fn associated_item(&self, def: DefId) -> Option<ty::AssociatedItem>
{
self.dep_graph.read(DepNode::MetaData(def));
self.get_crate_data(def.krate).get_associated_item(def.index)
Expand Down
75 changes: 22 additions & 53 deletions src/librustc_resolve/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,58 +417,38 @@ impl<'a> Resolver<'a> {
let ident = Ident::with_empty_ctxt(child.name);
let def = child.def;
let def_id = def.def_id();
let vis = match def {
Def::Macro(..) => ty::Visibility::Public,
_ if parent.is_trait() => ty::Visibility::Public,
_ => self.session.cstore.visibility(def_id),
};
let vis = self.session.cstore.visibility(def_id);

match def {
Def::Mod(..) | Def::Enum(..) => {
let module = self.new_module(parent, ModuleKind::Def(def, ident.name), def_id);
self.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, Mark::root()));
}
Def::Variant(..) => {
Def::Variant(..) | Def::TyAlias(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::VariantCtor(..) => {
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Fn(..) |
Def::Static(..) |
Def::Const(..) |
Def::AssociatedConst(..) |
Def::Method(..) => {
Def::Fn(..) | Def::Static(..) | Def::Const(..) |
Def::VariantCtor(..) | Def::StructCtor(..) => {
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Trait(..) => {
let module_kind = ModuleKind::Def(def, ident.name);
let module = self.new_module(parent, module_kind, parent.normal_ancestor_id);
self.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, Mark::root()));

// If this is a trait, add all the trait item names to the trait info.
let trait_item_def_ids = self.session.cstore.associated_item_def_ids(def_id);
for trait_item_def_id in trait_item_def_ids {
let trait_item_name = self.session.cstore.def_key(trait_item_def_id)
.disambiguated_data.data.get_opt_name()
.expect("opt_item_name returned None for trait");
self.trait_item_map.insert((trait_item_name, def_id), false);
}
}
Def::TyAlias(..) | Def::AssociatedTy(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Struct(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
for child in self.session.cstore.item_children(def_id) {
let ns = if let Def::AssociatedTy(..) = child.def { TypeNS } else { ValueNS };
let ident = Ident::with_empty_ctxt(child.name);
self.define(module, ident, ns, (child.def, ty::Visibility::Public,
DUMMY_SP, Mark::root()));

// Record field names for error reporting.
let field_names = self.session.cstore.struct_field_names(def_id);
self.insert_field_names(def_id, field_names);
}
Def::StructCtor(..) => {
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
let has_self = self.session.cstore.associated_item(child.def.def_id())
.map_or(false, |item| item.method_has_self_argument);
self.trait_item_map.insert((def_id, child.name, ns), (child.def, has_self));
}
module.populated.set(true);
}
Def::Union(..) => {
Def::Struct(..) | Def::Union(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));

// Record field names for error reporting.
Expand All @@ -478,15 +458,7 @@ impl<'a> Resolver<'a> {
Def::Macro(..) => {
self.define(parent, ident, MacroNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Local(..) |
Def::PrimTy(..) |
Def::TyParam(..) |
Def::Upvar(..) |
Def::Label(..) |
Def::SelfTy(..) |
Def::Err => {
bug!("unexpected definition: {:?}", def);
}
_ => bug!("unexpected definition: {:?}", def)
}
}

Expand Down Expand Up @@ -751,18 +723,15 @@ impl<'a, 'b> Visitor<'a> for BuildReducedGraphVisitor<'a, 'b> {

// Add the item to the trait info.
let item_def_id = self.resolver.definitions.local_def_id(item.id);
let mut is_static_method = false;
let (def, ns) = match item.node {
TraitItemKind::Const(..) => (Def::AssociatedConst(item_def_id), ValueNS),
TraitItemKind::Method(ref sig, _) => {
is_static_method = !sig.decl.has_self();
(Def::Method(item_def_id), ValueNS)
}
TraitItemKind::Type(..) => (Def::AssociatedTy(item_def_id), TypeNS),
let (def, ns, has_self) = match item.node {
TraitItemKind::Const(..) => (Def::AssociatedConst(item_def_id), ValueNS, false),
TraitItemKind::Method(ref sig, _) =>
(Def::Method(item_def_id), ValueNS, sig.decl.has_self()),
TraitItemKind::Type(..) => (Def::AssociatedTy(item_def_id), TypeNS, false),
TraitItemKind::Macro(_) => bug!(), // handled above
};

self.resolver.trait_item_map.insert((item.ident.name, def_id), is_static_method);
self.resolver.trait_item_map.insert((def_id, item.ident.name, ns), (def, has_self));

let vis = ty::Visibility::Public;
self.resolver.define(parent, item.ident, ns, (def, vis, item.span, self.expansion));
Expand Down
27 changes: 26 additions & 1 deletion src/librustc_resolve/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,26 @@ match (A, B, C) {
```
"##,

E0422: r##"
You are trying to use an identifier that is either undefined or not a struct.
Copy link
Contributor

@jseyfried jseyfried Dec 5, 2016

Choose a reason for hiding this comment

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

I find it unnatural to group these two cases into one error code -- I would think either have an error code just for latter case or not have a separate error code here at all.
That being said, I'm not too familiar with best practices for error codes, so I'll defer to @jonathandturner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly kept status quo regarding error codes, it may make sense to have a separate code for each (path_source, has_resolution) combination, 3 new error codes will need to be allocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new error codes.

Erroneous code example:
``` compile_fail,E0422
fn main () {
let x = Foo { x: 1, y: 2 };
}
```
In this case, `Foo` is undefined, so it inherently isn't anything, and
definitely not a struct.
```compile_fail
fn main () {
let foo = 1;
let x = foo { x: 1, y: 2 };
}
```
In this case, `foo` is defined, but is not a struct, so Rust can't use it as
one.
"##,

E0423: r##"
A `struct` variant name was used like a function name.

Expand Down Expand Up @@ -1519,7 +1539,12 @@ register_diagnostics! {
// E0419, merged into 531
// E0420, merged into 532
// E0421, merged into 531
// E0422, merged into 531/532
E0531, // unresolved pattern path kind `name`
// E0427, merged into 530
E0573,
E0574,
E0575,
E0576,
E0577,
E0578,
}
Loading