Further cleanup in resolve

`try_define` is not used in build_reduced_graph anymore
Collection of field names for error reporting is optimized
Some comments added

Author: petrochenkov

src/librustc_metadata/decoder.rs

@@ -699,6 +699,8 @@ impl<'a, 'tcx> CrateMetadata {
                             }
                         }
                         Def::Variant(def_id) => {
+                            // Braced variants, unlike structs, generate unusable names in
+                            // value namespace, they are reserved for possible future use.
                             let vkind = self.get_variant_kind(child_index).unwrap();
                             let ctor_def = Def::VariantCtor(def_id, vkind.ctor_kind());
                             callback(def::Export { def: ctor_def, name: name });

src/librustc_resolve/build_reduced_graph.rs

@@ -31,7 +31,6 @@ use std::rc::Rc;
 
 use syntax::ast::Name;
 use syntax::attr;
-use syntax::parse::token;
 
 use syntax::ast::{self, Block, ForeignItem, ForeignItemKind, Item, ItemKind};
 use syntax::ast::{Mutability, StmtKind, TraitItem, TraitItemKind};
@@ -77,6 +76,12 @@ impl<'b> Resolver<'b> {
         })
     }
 
+    fn insert_field_names(&mut self, def_id: DefId, field_names: Vec<Name>) {
+        if !field_names.is_empty() {
+            self.field_names.insert(def_id, field_names);
+        }
+    }
+
     /// Constructs the reduced graph for one item.
     fn build_reduced_graph_for_item(&mut self, item: &Item, expansion: Mark) {
         let parent = self.current_module;
@@ -301,28 +306,26 @@ impl<'b> Resolver<'b> {
                     self.define(parent, name, ValueNS, (ctor_def, sp, vis));
                 }
 
-                // Record the def ID and fields of this struct.
-                let field_names = struct_def.fields().iter().enumerate().map(|(index, field)| {
+                // Record field names for error reporting.
+                let field_names = struct_def.fields().iter().filter_map(|field| {
                     self.resolve_visibility(&field.vis);
                     field.ident.map(|ident| ident.name)
-                               .unwrap_or_else(|| token::intern(&index.to_string()))
                 }).collect();
                 let item_def_id = self.definitions.local_def_id(item.id);
-                self.structs.insert(item_def_id, field_names);
+                self.insert_field_names(item_def_id, field_names);
             }
 
             ItemKind::Union(ref vdata, _) => {
                 let def = Def::Union(self.definitions.local_def_id(item.id));
                 self.define(parent, name, TypeNS, (def, sp, vis));
 
-                // Record the def ID and fields of this union.
-                let field_names = vdata.fields().iter().enumerate().map(|(index, field)| {
+                // Record field names for error reporting.
+                let field_names = vdata.fields().iter().filter_map(|field| {
                     self.resolve_visibility(&field.vis);
                     field.ident.map(|ident| ident.name)
-                               .unwrap_or_else(|| token::intern(&index.to_string()))
                 }).collect();
                 let item_def_id = self.definitions.local_def_id(item.id);
-                self.structs.insert(item_def_id, field_names);
+                self.insert_field_names(item_def_id, field_names);
             }
 
             ItemKind::DefaultImpl(..) | ItemKind::Impl(..) => {}
@@ -347,18 +350,17 @@ impl<'b> Resolver<'b> {
                                        parent: Module<'b>,
                                        vis: ty::Visibility) {
         let name = variant.node.name.name;
-        let ctor_kind = CtorKind::from_vdata(&variant.node.data);
-        if variant.node.data.is_struct() {
-            // Not adding fields for variants as they are not accessed with a self receiver
-            let variant_def_id = self.definitions.local_def_id(variant.node.data.id());
-            self.structs.insert(variant_def_id, Vec::new());
-        }
+        let def_id = self.definitions.local_def_id(variant.node.data.id());
 
-        // All variants are defined in both type and value namespaces as future-proofing.
-        let def = Def::Variant(self.definitions.local_def_id(variant.node.data.id()));
-        let ctor_def = Def::VariantCtor(self.definitions.local_def_id(variant.node.data.id()),
-                                        ctor_kind);
+        // Define a name in the type namespace.
+        let def = Def::Variant(def_id);
         self.define(parent, name, TypeNS, (def, variant.span, vis));
+
+        // Define a constructor name in the value namespace.
+        // Braced variants, unlike structs, generate unusable names in
+        // value namespace, they are reserved for possible future use.
+        let ctor_kind = CtorKind::from_vdata(&variant.node.data);
+        let ctor_def = Def::VariantCtor(def_id, ctor_kind);
         self.define(parent, name, ValueNS, (ctor_def, variant.span, vis));
     }
 
@@ -407,79 +409,55 @@ impl<'b> Resolver<'b> {
         };
 
         match def {
-            Def::Mod(_) | Def::Enum(..) => {
-                debug!("(building reduced graph for external crate) building module {} {:?}",
-                       name, vis);
+            Def::Mod(..) | Def::Enum(..) => {
                 let module = self.new_module(parent, ModuleKind::Def(def, name), false);
-                let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis));
+                self.define(parent, name, TypeNS, (module, DUMMY_SP, vis));
             }
             Def::Variant(..) => {
-                debug!("(building reduced graph for external crate) building variant {}", name);
-                // All variants are defined in both type and value namespaces as future-proofing.
-                let vkind = self.session.cstore.variant_kind(def_id).unwrap();
-                let _ = self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis));
-                if vkind == ty::VariantKind::Struct {
-                    // Not adding fields for variants as they are not accessed with a self receiver
-                    self.structs.insert(def_id, Vec::new());
-                }
+                self.define(parent, name, TypeNS, (def, DUMMY_SP, vis));
             }
             Def::VariantCtor(..) => {
-                let _ = self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis));
+                self.define(parent, name, ValueNS, (def, DUMMY_SP, vis));
             }
             Def::Fn(..) |
             Def::Static(..) |
             Def::Const(..) |
             Def::AssociatedConst(..) |
             Def::Method(..) => {
-                debug!("(building reduced graph for external crate) building value (fn/static) {}",
-                       name);
-                let _ = self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis));
+                self.define(parent, name, ValueNS, (def, DUMMY_SP, vis));
             }
-            Def::Trait(_) => {
-                debug!("(building reduced graph for external crate) building type {}", name);
-
-                // If this is a trait, add all the trait item names to the trait
-                // info.
+            Def::Trait(..) => {
+                let module = self.new_module(parent, ModuleKind::Def(def, name), false);
+                self.define(parent, name, TypeNS, (module, DUMMY_SP, vis));
 
+                // If this is a trait, add all the trait item names to the trait info.
                 let trait_item_def_ids = self.session.cstore.impl_or_trait_items(def_id);
-                for &trait_item_def in &trait_item_def_ids {
-                    let trait_item_name =
-                        self.session.cstore.def_key(trait_item_def)
-                            .disambiguated_data.data.get_opt_name()
-                            .expect("opt_item_name returned None for trait");
-
-                    debug!("(building reduced graph for external crate) ... adding trait item \
-                            '{}'",
-                           trait_item_name);
-
+                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);
                 }
-
-                let module = self.new_module(parent, ModuleKind::Def(def, name), false);
-                let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis));
             }
             Def::TyAlias(..) | Def::AssociatedTy(..) => {
-                debug!("(building reduced graph for external crate) building type {}", name);
-                let _ = self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis));
+                self.define(parent, name, TypeNS, (def, DUMMY_SP, vis));
             }
             Def::Struct(..) => {
-                debug!("(building reduced graph for external crate) building type and value for {}",
-                       name);
-                let _ = self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis));
+                self.define(parent, name, TypeNS, (def, DUMMY_SP, vis));
 
-                // Record the def ID and fields of this struct.
-                let fields = self.session.cstore.struct_field_names(def_id);
-                self.structs.insert(def_id, fields);
+                // 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(..) => {
-                let _ = self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis));
+                self.define(parent, name, ValueNS, (def, DUMMY_SP, vis));
             }
-            Def::Union(_) => {
-                let _ = self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis));
+            Def::Union(..) => {
+                self.define(parent, name, TypeNS, (def, DUMMY_SP, vis));
 
-                // Record the def ID and fields of this union.
-                let fields = self.session.cstore.struct_field_names(def_id);
-                self.structs.insert(def_id, fields);
+                // 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::Local(..) |
             Def::PrimTy(..) |
@@ -488,7 +466,7 @@ impl<'b> Resolver<'b> {
             Def::Label(..) |
             Def::SelfTy(..) |
             Def::Err => {
-                bug!("didn't expect `{:?}`", def);
+                bug!("unexpected definition: {:?}", def);
             }
         }
     }

src/librustc_resolve/lib.rs

@@ -1006,7 +1006,9 @@ pub struct Resolver<'a> {
 
     trait_item_map: FnvHashMap<(Name, DefId), bool /* is static method? */>,
 
-    structs: FnvHashMap<DefId, Vec<Name>>,
+    // Names of fields of an item `DefId` accessible with dot syntax.
+    // Used for hints during error reporting.
+    field_names: FnvHashMap<DefId, Vec<Name>>,
 
     // All imports known to succeed or fail.
     determined_imports: Vec<&'a ImportDirective<'a>>,
@@ -1218,7 +1220,7 @@ impl<'a> Resolver<'a> {
             prelude: None,
 
             trait_item_map: FnvHashMap(),
-            structs: FnvHashMap(),
+            field_names: FnvHashMap(),
 
             determined_imports: Vec::new(),
             indeterminate_imports: Vec::new(),
@@ -2779,8 +2781,8 @@ impl<'a> Resolver<'a> {
                 match resolution.base_def {
                     Def::Enum(did) | Def::TyAlias(did) | Def::Union(did) |
                     Def::Struct(did) | Def::Variant(did) if resolution.depth == 0 => {
-                        if let Some(fields) = self.structs.get(&did) {
-                            if fields.iter().any(|&field_name| name == field_name) {
+                        if let Some(field_names) = self.field_names.get(&did) {
+                            if field_names.iter().any(|&field_name| name == field_name) {
                                 return Field;
                             }
                         }
@@ -2852,7 +2854,6 @@ impl<'a> Resolver<'a> {
                         _ => false,
                     };
                     if is_struct_variant {
-                        let _ = self.structs.contains_key(&path_res.base_def.def_id());
                         let path_name = path_names_to_string(path, 0);
 
                         let mut err = resolve_struct_error(self,
@@ -2885,9 +2886,6 @@ impl<'a> Resolver<'a> {
                     }
                 } else {
                     // Be helpful if the name refers to a struct
-                    // (The pattern matching def_tys where the id is in self.structs
-                    // matches on regular structs while excluding tuple- and enum-like
-                    // structs, which wouldn't result in this error.)
                     let path_name = path_names_to_string(path, 0);
                     let type_res = self.with_no_errors(|this| {
                         this.resolve_path(expr.id, path, 0, TypeNS)