From 595e2910d8132170dab30fd31fc09800609188a8 Mon Sep 17 00:00:00 2001 From: Kevin Butler Date: Thu, 8 May 2014 22:35:09 +0100 Subject: [PATCH] rustc: Improve error messages for resolve failures. --- src/librustc/middle/resolve.rs | 362 +++++++++++++++++++--------- src/test/compile-fail/issue-2356.rs | 79 +++++- 2 files changed, 325 insertions(+), 116 deletions(-) diff --git a/src/librustc/middle/resolve.rs b/src/librustc/middle/resolve.rs index bce82002ecb20..471e290d9145e 100644 --- a/src/librustc/middle/resolve.rs +++ b/src/librustc/middle/resolve.rs @@ -216,6 +216,15 @@ impl ResolveResult { } } +enum FallbackSuggestion { + NoSuggestion, + Field, + Method, + TraitMethod, + StaticMethod(StrBuf), + StaticTraitMethod(StrBuf), +} + enum TypeParameters<'a> { NoTypeParameters, //< No type parameters. HasTypeParameters(&'a Generics, //< Type parameters. @@ -822,7 +831,7 @@ fn Resolver<'a>(session: &'a Session, graph_root: graph_root, method_map: RefCell::new(FnvHashMap::new()), - structs: HashSet::new(), + structs: FnvHashMap::new(), unresolved_imports: 0, @@ -831,7 +840,8 @@ fn Resolver<'a>(session: &'a Session, type_ribs: RefCell::new(Vec::new()), label_ribs: RefCell::new(Vec::new()), - current_trait_refs: None, + current_trait_ref: None, + current_self_type: None, self_ident: special_idents::self_, type_self_ident: special_idents::type_self, @@ -861,7 +871,7 @@ struct Resolver<'a> { graph_root: NameBindings, method_map: RefCell>, - structs: HashSet, + structs: FnvHashMap>, // The number of imports that are currently unresolved. unresolved_imports: uint, @@ -880,7 +890,10 @@ struct Resolver<'a> { label_ribs: RefCell>, // The trait that the current context can refer to. - current_trait_refs: Option >, + current_trait_ref: Option<(DefId, TraitRef)>, + + // The current self type if inside an impl (used for better errors). + current_self_type: Option, // The ident for the keyword "self". self_ident: Ident, @@ -1229,8 +1242,14 @@ impl<'a> Resolver<'a> { None }); - // Record the def ID of this struct. - self.structs.insert(local_def(item.id)); + // Record the def ID and fields of this struct. + let named_fields = struct_def.fields.iter().filter_map(|f| { + match f.node.kind { + NamedField(ident, _) => Some(ident.name), + UnnamedField(_) => None + } + }).collect(); + self.structs.insert(local_def(item.id), named_fields); parent } @@ -1405,7 +1424,9 @@ impl<'a> Resolver<'a> { child.define_type(DefVariant(item_id, local_def(variant.node.id), true), variant.span, is_public); - self.structs.insert(local_def(variant.node.id)); + + // Not adding fields for variants as they are not accessed with a self receiver + self.structs.insert(local_def(variant.node.id), Vec::new()); } } } @@ -1631,7 +1652,8 @@ impl<'a> Resolver<'a> { self.external_exports.contains(&enum_did); if is_struct { child_name_bindings.define_type(def, DUMMY_SP, is_exported); - self.structs.insert(variant_id); + // Not adding fields for variants as they are not accessed with a self receiver + self.structs.insert(variant_id, Vec::new()); } else { child_name_bindings.define_value(def, DUMMY_SP, is_exported); } @@ -1689,10 +1711,16 @@ impl<'a> Resolver<'a> { crate) building type and value for {}", final_ident); child_name_bindings.define_type(def, DUMMY_SP, is_public); - if csearch::get_struct_fields(&self.session.cstore, def_id).len() == 0 { + let fields = csearch::get_struct_fields(&self.session.cstore, def_id).iter().map(|f| { + f.name + }).collect::>(); + + if fields.len() == 0 { child_name_bindings.define_value(def, DUMMY_SP, is_public); } - self.structs.insert(def_id); + + // Record the def ID and fields of this struct. + self.structs.insert(def_id, fields); } DefMethod(..) => { debug!("(building reduced graph for external crate) \ @@ -2046,7 +2074,7 @@ impl<'a> Resolver<'a> { } } - fn idents_to_str(&mut self, idents: &[Ident]) -> StrBuf { + fn idents_to_str(&self, idents: &[Ident]) -> StrBuf { let mut first = true; let mut result = StrBuf::new(); for ident in idents.iter() { @@ -2060,7 +2088,7 @@ impl<'a> Resolver<'a> { result } - fn path_idents_to_str(&mut self, path: &Path) -> StrBuf { + fn path_idents_to_str(&self, path: &Path) -> StrBuf { let identifiers: Vec = path.segments .iter() .map(|seg| seg.identifier) @@ -2611,7 +2639,7 @@ impl<'a> Resolver<'a> { while index < module_path_len { let name = module_path[index]; match self.resolve_name_in_module(search_module.clone(), - name, + name.name, TypeNS, name_search_type, false) { @@ -2924,7 +2952,7 @@ impl<'a> Resolver<'a> { // Resolve the name in the parent module. match self.resolve_name_in_module(search_module.clone(), - name, + name.name, namespace, PathSearch, true) { @@ -3090,19 +3118,19 @@ impl<'a> Resolver<'a> { /// passed through a public re-export proxy. fn resolve_name_in_module(&mut self, module_: Rc, - name: Ident, + name: Name, namespace: Namespace, name_search_type: NameSearchType, allow_private_imports: bool) -> ResolveResult<(Target, bool)> { debug!("(resolving name in module) resolving `{}` in `{}`", - token::get_ident(name), + token::get_name(name).get(), self.module_to_str(&*module_)); // First, check the direct children of the module. self.populate_module_if_necessary(&module_); - match module_.children.borrow().find(&name.name) { + match module_.children.borrow().find(&name) { Some(name_bindings) if name_bindings.defined_in_namespace(namespace) => { debug!("(resolving name in module) found node as child"); @@ -3123,7 +3151,7 @@ impl<'a> Resolver<'a> { } // Check the list of resolved imports. - match module_.import_resolutions.borrow().find(&name.name) { + match module_.import_resolutions.borrow().find(&name) { Some(import_resolution) if allow_private_imports || import_resolution.is_public => { @@ -3152,7 +3180,7 @@ impl<'a> Resolver<'a> { // Finally, search through external children. if namespace == TypeNS { - match module_.external_module_children.borrow().find_copy(&name.name) { + match module_.external_module_children.borrow().find_copy(&name) { None => {} Some(module) => { let name_bindings = @@ -3164,7 +3192,7 @@ impl<'a> Resolver<'a> { // We're out of luck. debug!("(resolving name in module) failed to resolve `{}`", - token::get_ident(name)); + token::get_name(name).get()); return Failed; } @@ -3881,7 +3909,7 @@ impl<'a> Resolver<'a> { Some(t) => match t.node { TyPath(ref path, None, path_id) => { match this.resolve_path(id, path, TypeNS, true) { - Some((DefTy(def_id), lp)) if this.structs.contains(&def_id) => { + Some((DefTy(def_id), lp)) if this.structs.contains_key(&def_id) => { let def = DefStruct(def_id); debug!("(resolving struct) resolved `{}` to type {:?}", token::get_ident(path.segments @@ -3932,6 +3960,37 @@ impl<'a> Resolver<'a> { self.resolve_function(rib_kind, Some(method.decl), type_parameters, method.body); } + fn with_current_self_type(&mut self, self_type: &Ty, f: |&mut Resolver| -> T) -> T { + // Handle nested impls (inside fn bodies) + let previous_value = replace(&mut self.current_self_type, Some(self_type.clone())); + let result = f(self); + self.current_self_type = previous_value; + result + } + + fn with_optional_trait_ref(&mut self, id: NodeId, + opt_trait_ref: &Option, + f: |&mut Resolver| -> T) -> T { + let new_val = match *opt_trait_ref { + Some(ref trait_ref) => { + self.resolve_trait_reference(id, trait_ref, TraitImplementation); + + match self.def_map.borrow().find(&trait_ref.ref_id) { + Some(def) => { + let did = def_id_of_def(*def); + Some((did, trait_ref.clone())) + } + None => None + } + } + None => None + }; + let original_trait_ref = replace(&mut self.current_trait_ref, new_val); + let result = f(self); + self.current_trait_ref = original_trait_ref; + result + } + fn resolve_implementation(&mut self, id: NodeId, generics: &Generics, @@ -3949,58 +4008,19 @@ impl<'a> Resolver<'a> { this.resolve_type_parameters(&generics.ty_params); // Resolve the trait reference, if necessary. - let original_trait_refs; - match opt_trait_reference { - &Some(ref trait_reference) => { - this.resolve_trait_reference(id, trait_reference, - TraitImplementation); - - // Record the current set of trait references. - let mut new_trait_refs = Vec::new(); - for &def in this.def_map.borrow() - .find(&trait_reference.ref_id).iter() { - new_trait_refs.push(def_id_of_def(*def)); + this.with_optional_trait_ref(id, opt_trait_reference, |this| { + // Resolve the self type. + this.resolve_type(self_type); + + this.with_current_self_type(self_type, |this| { + for method in methods.iter() { + // We also need a new scope for the method-specific type parameters. + this.resolve_method(MethodRibKind(id, Provided(method.id)), + *method, + outer_type_parameter_count); } - original_trait_refs = Some(replace( - &mut this.current_trait_refs, - Some(new_trait_refs))); - } - &None => { - original_trait_refs = None; - } - } - - // Resolve the self type. - this.resolve_type(self_type); - - for method in methods.iter() { - // We also need a new scope for the method-specific - // type parameters. - this.resolve_method(MethodRibKind( - id, - Provided(method.id)), - *method, - outer_type_parameter_count); -/* - let borrowed_type_parameters = &method.tps; - self.resolve_function(MethodRibKind( - id, - Provided(method.id)), - Some(method.decl), - HasTypeParameters - (borrowed_type_parameters, - method.id, - outer_type_parameter_count, - NormalRibKind), - method.body); -*/ - } - - // Restore the original trait references. - match original_trait_refs { - Some(r) => { this.current_trait_refs = r; } - None => () - } + }); + }); }); } @@ -4459,16 +4479,16 @@ impl<'a> Resolver<'a> { PatStruct(ref path, _, _) => { match self.resolve_path(pat_id, path, TypeNS, false) { Some((DefTy(class_id), lp)) - if self.structs.contains(&class_id) => { + if self.structs.contains_key(&class_id) => { let class_def = DefStruct(class_id); self.record_def(pattern.id, (class_def, lp)); } Some(definition @ (DefStruct(class_id), _)) => { - assert!(self.structs.contains(&class_id)); + assert!(self.structs.contains_key(&class_id)); self.record_def(pattern.id, definition); } Some(definition @ (DefVariant(_, variant_id, _), _)) - if self.structs.contains(&variant_id) => { + if self.structs.contains_key(&variant_id) => { self.record_def(pattern.id, definition); } result => { @@ -4607,13 +4627,13 @@ impl<'a> Resolver<'a> { // FIXME #4952: Merge me with resolve_name_in_module? fn resolve_definition_of_name_in_module(&mut self, containing_module: Rc, - name: Ident, + name: Name, namespace: Namespace) -> NameDefinition { // First, search children. self.populate_module_if_necessary(&containing_module); - match containing_module.children.borrow().find(&name.name) { + match containing_module.children.borrow().find(&name) { Some(child_name_bindings) => { match child_name_bindings.def_for_namespace(namespace) { Some(def) => { @@ -4632,7 +4652,7 @@ impl<'a> Resolver<'a> { } // Next, search import resolutions. - match containing_module.import_resolutions.borrow().find(&name.name) { + match containing_module.import_resolutions.borrow().find(&name) { Some(import_resolution) if import_resolution.is_public => { match (*import_resolution).target_for_namespace(namespace) { Some(target) => { @@ -4658,7 +4678,7 @@ impl<'a> Resolver<'a> { // Finally, search through external children. if namespace == TypeNS { match containing_module.external_module_children.borrow() - .find_copy(&name.name) { + .find_copy(&name) { None => {} Some(module) => { match module.def_id.get() { @@ -4713,7 +4733,7 @@ impl<'a> Resolver<'a> { let ident = path.segments.last().unwrap().identifier; let def = match self.resolve_definition_of_name_in_module(containing_module.clone(), - ident, + ident.name, namespace) { NoNameDefinition => { // We failed to resolve the name. Report an error. @@ -4782,7 +4802,7 @@ impl<'a> Resolver<'a> { } } - let name = path.segments.last().unwrap().identifier; + let name = path.segments.last().unwrap().identifier.name; match self.resolve_definition_of_name_in_module(containing_module, name, namespace) { @@ -4883,6 +4903,99 @@ impl<'a> Resolver<'a> { } } + fn find_fallback_in_self_type(&mut self, name: Name) -> FallbackSuggestion { + fn get_module(this: &mut Resolver, span: Span, ident_path: &[ast::Ident]) + -> Option> { + let root = this.current_module.clone(); + let last_name = ident_path.last().unwrap().name; + + if ident_path.len() == 1 { + match this.primitive_type_table.primitive_types.find(&last_name) { + Some(_) => None, + None => { + match this.current_module.children.borrow().find(&last_name) { + Some(child) => child.get_module_if_available(), + None => None + } + } + } + } else { + match this.resolve_module_path(root, + ident_path.as_slice(), + UseLexicalScope, + span, + PathSearch) { + Success((module, _)) => Some(module), + _ => None + } + } + } + + let (path, node_id) = match self.current_self_type { + Some(ref ty) => match ty.node { + TyPath(ref path, _, node_id) => (path.clone(), node_id), + _ => unreachable!(), + }, + None => return NoSuggestion, + }; + + // Look for a field with the same name in the current self_type. + match self.def_map.borrow().find(&node_id) { + Some(&DefTy(did)) + | Some(&DefStruct(did)) + | Some(&DefVariant(_, did, _)) => match self.structs.find(&did) { + None => {} + Some(fields) => { + if fields.iter().any(|&field_name| name == field_name) { + return Field; + } + } + }, + _ => {} // Self type didn't resolve properly + } + + let ident_path = path.segments.iter().map(|seg| seg.identifier).collect::>(); + + // Look for a method in the current self type's impl module. + match get_module(self, path.span, ident_path.as_slice()) { + Some(module) => match module.children.borrow().find(&name) { + Some(binding) => { + let p_str = self.path_idents_to_str(&path); + match binding.def_for_namespace(ValueNS) { + Some(DefStaticMethod(_, provenance, _)) => { + match provenance { + FromImpl(_) => return StaticMethod(p_str), + FromTrait(_) => unreachable!() + } + } + Some(DefMethod(_, None)) => return Method, + Some(DefMethod(_, _)) => return TraitMethod, + _ => () + } + } + None => {} + }, + None => {} + } + + // Look for a method in the current trait. + let method_map = self.method_map.borrow(); + match self.current_trait_ref { + Some((did, ref trait_ref)) => { + let path_str = self.path_idents_to_str(&trait_ref.path); + + match method_map.find(&(name, did)) { + Some(&SelfStatic) => return StaticTraitMethod(path_str), + Some(_) => return TraitMethod, + None => {} + } + } + None => {} + } + + NoSuggestion + } + fn find_best_match_for_name(&mut self, name: &str, max_distance: uint) -> Option { let this = &mut *self; @@ -4970,7 +5083,7 @@ impl<'a> Resolver<'a> { match self.with_no_errors(|this| this.resolve_path(expr.id, path, TypeNS, false)) { Some((DefTy(struct_id), _)) - if self.structs.contains(&struct_id) => { + if self.structs.contains_key(&struct_id) => { self.resolve_error(expr.span, format!("`{}` is a structure name, but \ this expression \ @@ -4983,25 +5096,53 @@ impl<'a> Resolver<'a> { wrong_name)); } - _ => - // limit search to 5 to reduce the number - // of stupid suggestions - match self.find_best_match_for_name( - wrong_name.as_slice(), - 5) { - Some(m) => { - self.resolve_error(expr.span, - format!("unresolved name `{}`. \ - Did you mean `{}`?", - wrong_name, - m)); - } - None => { - self.resolve_error(expr.span, - format!("unresolved name `{}`.", - wrong_name.as_slice())); - } - } + _ => { + let mut method_scope = false; + self.value_ribs.borrow().iter().rev().advance(|rib| { + let res = match *rib { + Rib { bindings: _, kind: MethodRibKind(_, _) } => true, + Rib { bindings: _, kind: OpaqueFunctionRibKind } => false, + _ => return true, // Keep advancing + }; + + method_scope = res; + false // Stop advancing + }); + + if method_scope && token::get_name(self.self_ident.name).get() + == wrong_name.as_slice() { + self.resolve_error(expr.span, + format!("`self` is not available in a \ + static method. Maybe a `self` \ + argument is missing?")); + } else { + let name = path_to_ident(path).name; + let mut msg = match self.find_fallback_in_self_type(name) { + NoSuggestion => { + // limit search to 5 to reduce the number + // of stupid suggestions + self.find_best_match_for_name(wrong_name.as_slice(), 5) + .map_or("".into_owned(), + |x| format!("`{}`", x)) + } + Field => + format!("`self.{}`", wrong_name), + Method + | TraitMethod => + format!("to call `self.{}`", wrong_name), + StaticTraitMethod(path_str) + | StaticMethod(path_str) => + format!("to call `{}::{}`", path_str, wrong_name) + }; + + if msg.len() > 0 { + msg = format!(" Did you mean {}?", msg) + } + + self.resolve_error(expr.span, format!("unresolved name `{}`.{}", + wrong_name, msg)); + } + } } } } @@ -5020,12 +5161,12 @@ impl<'a> Resolver<'a> { // Resolve the path to the structure it goes to. match self.resolve_path(expr.id, path, TypeNS, false) { Some((DefTy(class_id), lp)) | Some((DefStruct(class_id), lp)) - if self.structs.contains(&class_id) => { + if self.structs.contains_key(&class_id) => { let class_def = DefStruct(class_id); self.record_def(expr.id, (class_def, lp)); } Some(definition @ (DefVariant(_, class_id, _), _)) - if self.structs.contains(&class_id) => { + if self.structs.contains_key(&class_id) => { self.record_def(expr.id, definition); } result => { @@ -5125,18 +5266,15 @@ impl<'a> Resolver<'a> { let mut search_module = self.current_module.clone(); loop { // Look for the current trait. - match self.current_trait_refs { - Some(ref trait_def_ids) => { + match self.current_trait_ref { + Some((trait_def_id, _)) => { let method_map = self.method_map.borrow(); - for &trait_def_id in trait_def_ids.iter() { - if method_map.contains_key(&(name, trait_def_id)) { - add_trait_info(&mut found_traits, trait_def_id, name); - } + + if method_map.contains_key(&(name, trait_def_id)) { + add_trait_info(&mut found_traits, trait_def_id, name); } } - None => { - // Nothing to do. - } + None => {} // Nothing to do. } // Look for trait children. diff --git a/src/test/compile-fail/issue-2356.rs b/src/test/compile-fail/issue-2356.rs index 22676d0b8c5f0..4d9e56a21f804 100644 --- a/src/test/compile-fail/issue-2356.rs +++ b/src/test/compile-fail/issue-2356.rs @@ -8,12 +8,83 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-test Resolve code for classes knew how to do this, impls don't +trait Groom { + fn shave(); +} + +pub struct cat { + whiskers: int, +} + +pub enum MaybeDog { + Dog, + NoDog +} + +impl MaybeDog { + fn bark() { + // If this provides a suggestion, it's a bug as MaybeDog doesn't impl Groom + shave(); + //~^ ERROR: unresolved name `shave`. + } +} + +impl Groom for cat { + fn shave(&self, other: uint) { + whiskers -= other; + //~^ ERROR: unresolved name `whiskers`. Did you mean `self.whiskers`? + shave(4); + //~^ ERROR: unresolved name `shave`. Did you mean to call `Groom::shave`? + purr(); + //~^ ERROR: unresolved name `purr`. Did you mean to call `self.purr`? + } +} + +impl cat { + fn static_method() {} -struct cat { - tail: int, + fn purr_louder() { + static_method(); + //~^ ERROR: unresolved name `static_method`. Did you mean to call `cat::static_method` + purr(); + //~^ ERROR: unresolved name `purr`. Did you mean to call `self.purr`? + purr(); + //~^ ERROR: unresolved name `purr`. Did you mean to call `self.purr`? + purr(); + //~^ ERROR: unresolved name `purr`. Did you mean to call `self.purr`? + } } impl cat { - pub fn meow() { tail += 1; } //~ ERROR: Did you mean: `self.tail` + fn meow() { + if self.whiskers > 3 { + //~^ ERROR: `self` is not available in a static method. Maybe a `self` argument is missing? + println!("MEOW"); + } + } + + fn purr(&self) { + grow_older(); + //~^ ERROR: unresolved name `grow_older`. Did you mean to call `cat::grow_older` + shave(); + //~^ ERROR: unresolved name `shave`. + } + + fn burn_whiskers(&mut self) { + whiskers = 0; + //~^ ERROR: unresolved name `whiskers`. Did you mean `self.whiskers`? + } + + pub fn grow_older(other:uint) { + whiskers = 4; + //~^ ERROR: unresolved name `whiskers`. Did you mean `self.whiskers`? + purr_louder(); + //~^ ERROR: unresolved name `purr_louder`. Did you mean to call `cat::purr_louder` + } +} + +fn main() { + self += 1; + //~^ ERROR: unresolved name `self`. + // it's a bug if this suggests a missing `self` as we're not in a method }