From 65ec4dfe61a70de869c2b62948af7d172c8ee612 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 16 Mar 2016 05:20:58 +0000 Subject: [PATCH] Improve diagnostics for duplicate names --- src/librustc_resolve/build_reduced_graph.rs | 32 +--------- src/librustc_resolve/lib.rs | 66 ++++++++++++++++---- src/librustc_resolve/resolve_imports.rs | 69 ++------------------- 3 files changed, 63 insertions(+), 104 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index dba7d178b4c..08b5e517290 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -105,36 +105,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { /// otherwise, reports an error. fn define>(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) { let binding = def.to_name_binding(); - let old_binding = match parent.try_define_child(name, ns, binding.clone()) { - Ok(()) => return, - Err(old_binding) => old_binding, - }; - - let span = binding.span.unwrap_or(DUMMY_SP); - if !old_binding.is_extern_crate() && !binding.is_extern_crate() { - // Record an error here by looking up the namespace that had the duplicate - let ns_str = match ns { TypeNS => "type or module", ValueNS => "value" }; - let resolution_error = ResolutionError::DuplicateDefinition(ns_str, name); - let mut err = resolve_struct_error(self, span, resolution_error); - - if let Some(sp) = old_binding.span { - let note = format!("first definition of {} `{}` here", ns_str, name); - err.span_note(sp, ¬e); - } - err.emit(); - } else if old_binding.is_extern_crate() && binding.is_extern_crate() { - span_err!(self.session, - span, - E0259, - "an external crate named `{}` has already been imported into this module", - name); - } else { - span_err!(self.session, - span, - E0260, - "the name `{}` conflicts with an external crate \ - that has been imported into this module", - name); + if let Err(old_binding) = parent.try_define_child(name, ns, binding.clone()) { + self.report_conflict(parent, name, ns, old_binding, &binding); } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 9a4173bad6e..6313d7b7036 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -183,8 +183,6 @@ pub enum ResolutionError<'a> { UndeclaredLabel(&'a str), /// error E0427: cannot use `ref` binding mode with ... CannotUseRefBindingModeWith(&'a str), - /// error E0428: duplicate definition - DuplicateDefinition(&'a str, Name), /// error E0429: `self` imports are only allowed within a { } list SelfImportsOnlyAllowedWithin, /// error E0430: `self` import can only appear once in the list @@ -490,14 +488,6 @@ fn resolve_struct_error<'b, 'a: 'b, 'tcx: 'a>(resolver: &'b Resolver<'a, 'tcx>, "cannot use `ref` binding mode with {}", descr) } - ResolutionError::DuplicateDefinition(namespace, name) => { - struct_span_err!(resolver.session, - span, - E0428, - "duplicate definition of {} `{}`", - namespace, - name) - } ResolutionError::SelfImportsOnlyAllowedWithin => { struct_span_err!(resolver.session, span, @@ -3530,8 +3520,62 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } } } -} + fn report_conflict(&self, + parent: Module, + name: Name, + ns: Namespace, + binding: &NameBinding, + old_binding: &NameBinding) { + // Error on the second of two conflicting names + if old_binding.span.unwrap().lo > binding.span.unwrap().lo { + return self.report_conflict(parent, name, ns, old_binding, binding); + } + + let container = match parent.def { + Some(Def::Mod(_)) => "module", + Some(Def::Trait(_)) => "trait", + None => "block", + _ => "enum", + }; + + let (participle, noun) = match old_binding.is_import() || old_binding.is_extern_crate() { + true => ("imported", "import"), + false => ("defined", "definition"), + }; + + let span = binding.span.unwrap(); + let msg = { + let kind = match (ns, old_binding.module()) { + (ValueNS, _) => "a value", + (TypeNS, Some(module)) if module.extern_crate_id.is_some() => "an extern crate", + (TypeNS, Some(module)) if module.is_normal() => "a module", + (TypeNS, Some(module)) if module.is_trait() => "a trait", + (TypeNS, _) => "a type", + }; + format!("{} named `{}` has already been {} in this {}", + kind, name, participle, container) + }; + + let mut err = match (old_binding.is_extern_crate(), binding.is_extern_crate()) { + (true, true) => struct_span_err!(self.session, span, E0259, "{}", msg), + (true, _) | (_, true) if binding.is_import() || old_binding.is_import() => + struct_span_err!(self.session, span, E0254, "{}", msg), + (true, _) | (_, true) => struct_span_err!(self.session, span, E0260, "{}", msg), + _ => match (old_binding.is_import(), binding.is_import()) { + (false, false) => struct_span_err!(self.session, span, E0428, "{}", msg), + (true, true) => struct_span_err!(self.session, span, E0252, "{}", msg), + _ => struct_span_err!(self.session, span, E0255, "{}", msg), + }, + }; + + let span = old_binding.span.unwrap(); + if span != codemap::DUMMY_SP { + err.span_note(span, &format!("previous {} of `{}` here", noun, name)); + } + err.emit(); + } +} fn names_to_string(names: &[Name]) -> String { let mut first = true; diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index c2b665b3b4c..bca79df7a91 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -513,7 +513,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let imported_binding = directive.import(binding, privacy_error); let conflict = module_.try_define_child(target, ns, imported_binding); if let Err(old_binding) = conflict { - self.report_conflict(target, ns, &directive.import(binding, None), old_binding); + let binding = &directive.import(binding, None); + self.resolver.report_conflict(module_, target, ns, binding, old_binding); } } @@ -650,67 +651,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { return Success(()); } - fn report_conflict(&mut self, - name: Name, - ns: Namespace, - binding: &NameBinding, - old_binding: &NameBinding) { - // Error on the second of two conflicting imports - if old_binding.is_import() && binding.is_import() && - old_binding.span.unwrap().lo > binding.span.unwrap().lo { - self.report_conflict(name, ns, old_binding, binding); - return; - } - - if old_binding.is_extern_crate() { - let msg = format!("import `{0}` conflicts with imported crate \ - in this module (maybe you meant `use {0}::*`?)", - name); - span_err!(self.resolver.session, binding.span.unwrap(), E0254, "{}", &msg); - } else if old_binding.is_import() { - let ns_word = match (ns, old_binding.module()) { - (ValueNS, _) => "value", - (TypeNS, Some(module)) if module.is_normal() => "module", - (TypeNS, Some(module)) if module.is_trait() => "trait", - (TypeNS, _) => "type", - }; - let mut err = struct_span_err!(self.resolver.session, - binding.span.unwrap(), - E0252, - "a {} named `{}` has already been imported \ - in this module", - ns_word, - name); - err.span_note(old_binding.span.unwrap(), - &format!("previous import of `{}` here", name)); - err.emit(); - } else if ns == ValueNS { // Check for item conflicts in the value namespace - let mut err = struct_span_err!(self.resolver.session, - binding.span.unwrap(), - E0255, - "import `{}` conflicts with value in this module", - name); - err.span_note(old_binding.span.unwrap(), "conflicting value here"); - err.emit(); - } else { // Check for item conflicts in the type namespace - let (what, note) = match old_binding.module() { - Some(ref module) if module.is_normal() => - ("existing submodule", "note conflicting module here"), - Some(ref module) if module.is_trait() => - ("trait in this module", "note conflicting trait here"), - _ => ("type in this module", "note conflicting type here"), - }; - let mut err = struct_span_err!(self.resolver.session, - binding.span.unwrap(), - E0256, - "import `{}` conflicts with {}", - name, - what); - err.span_note(old_binding.span.unwrap(), note); - err.emit(); - } - } - // Miscellaneous post-processing, including recording reexports, recording shadowed traits, // reporting conflicts, reporting the PRIVATE_IN_PUBLIC lint, and reporting unresolved imports. fn finalize_resolutions(&mut self, module: Module<'b>, report_unresolved_imports: bool) { @@ -720,7 +660,10 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let mut reexports = Vec::new(); for (&(name, ns), resolution) in module.resolutions.borrow().iter() { - resolution.report_conflicts(|b1, b2| self.report_conflict(name, ns, b1, b2)); + resolution.report_conflicts(|b1, b2| { + self.resolver.report_conflict(module, name, ns, b1, b2) + }); + let binding = match resolution.binding { Some(binding) => binding, None => continue,