Auto merge of #32284 - jseyfried:name_conflict_diagnostics, r=eddyb

Resolve: improve diagnostics for duplicate definitions and imports

This PR improves and regularizes the diagnostics for duplicate definitions and imports.

After this PR, the second of two duplicate definitions/imports will have the following form:
> a(n) [value|type|module|trait|extern crate] named \`*name*\` has already been [defined|imported] in this [module|block|trait|enum]

with a note referencing this first of the two duplicate definitions/imports:
> previous [definition|import] of \`*name*\` here

The error indices remain unchanged.

r? @eddyb

Author: bors

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<T: ToNameBinding<'b>>(&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, &note);
-            }
-            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);
         }
     }
 

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;

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,

src/test/compile-fail/blind-item-block-item-shadow.rs

@@ -14,7 +14,7 @@ fn main() {
     {
         struct Bar;
         use foo::Bar;
-        //~^ ERROR import `Bar` conflicts with type in this module
-        //~^^ ERROR import `Bar` conflicts with value in this module
+        //~^ ERROR a type named `Bar` has already been defined in this block
+        //~^^ ERROR a value named `Bar` has already been defined in this block
     }
 }

src/test/compile-fail/blind-item-item-shadow.rs

@@ -8,8 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-mod foo { pub mod foo {  } }
+mod foo { pub mod foo {  } } //~ NOTE previous definition of `foo` here
 
-use foo::foo; //~ ERROR import `foo` conflicts with existing submodule
+use foo::foo; //~ ERROR a module named `foo` has already been defined in this module
 
 fn main() {}

src/test/compile-fail/enum-and-module-in-same-scope.rs

@@ -8,11 +8,11 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-enum Foo {
+enum Foo { //~ NOTE previous definition
     X
 }
 
-mod Foo { //~ ERROR duplicate definition of type or module `Foo`
+mod Foo { //~ ERROR a type named `Foo` has already been defined
     pub static X: isize = 42;
     fn f() { f() } // Check that this does not result in a resolution error
 }

src/test/compile-fail/issue-19498.rs

@@ -8,14 +8,14 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use self::A; //~ ERROR import `A` conflicts with existing submodule
-use self::B; //~ ERROR import `B` conflicts with existing submodule
-mod A {}
-pub mod B {}
+use self::A; //~ NOTE previous import of `A` here
+use self::B; //~ NOTE previous import of `B` here
+mod A {} //~ ERROR a module named `A` has already been imported in this module
+pub mod B {} //~ ERROR a module named `B` has already been imported in this module
 
 mod C {
-    use C::D; //~ ERROR import `D` conflicts with existing submodule
-    mod D {}
+    use C::D; //~ NOTE previous import of `D` here
+    mod D {} //~ ERROR a module named `D` has already been imported in this module
 }
 
 fn main() {}

src/test/compile-fail/issue-21546.rs

@@ -12,54 +12,54 @@
 
 #[allow(non_snake_case)]
 mod Foo { }
-//~^ NOTE first definition of type or module `Foo`
+//~^ NOTE previous definition of `Foo` here
 
 #[allow(dead_code)]
 struct Foo;
-//~^ ERROR duplicate definition of type or module `Foo`
-
+//~^ ERROR a module named `Foo` has already been defined in this module
 
 #[allow(non_snake_case)]
 mod Bar { }
-//~^ NOTE first definition of type or module `Bar`
+//~^ NOTE previous definition of `Bar` here
 
 #[allow(dead_code)]
 struct Bar(i32);
-//~^ ERROR duplicate definition of type or module `Bar`
+//~^ ERROR a module named `Bar` has already been defined
 
 
 #[allow(dead_code)]
 struct Baz(i32);
-//~^ NOTE first definition of type or module
+//~^ NOTE previous definition
 
 #[allow(non_snake_case)]
 mod Baz { }
-//~^ ERROR duplicate definition of type or module `Baz`
+//~^ ERROR a type named `Baz` has already been defined
 
 
 #[allow(dead_code)]
 struct Qux { x: bool }
-//~^ NOTE first definition of type or module
+//~^ NOTE previous definition
 
 #[allow(non_snake_case)]
 mod Qux { }
-//~^ ERROR duplicate definition of type or module `Qux`
+//~^ ERROR a type named `Qux` has already been defined
 
 
 #[allow(dead_code)]
 struct Quux;
-//~^ NOTE first definition of type or module
+//~^ NOTE previous definition
 
 #[allow(non_snake_case)]
 mod Quux { }
-//~^ ERROR duplicate definition of type or module `Quux`
+//~^ ERROR a type named `Quux` has already been defined
 
 
 #[allow(dead_code)]
 enum Corge { A, B }
+//~^ NOTE previous definition
 
 #[allow(non_snake_case)]
 mod Corge { }
-//~^ ERROR duplicate definition of type or module `Corge`
+//~^ ERROR a type named `Corge` has already been defined
 
 fn main() { }

src/test/compile-fail/issue-24081.rs

@@ -8,16 +8,16 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use std::ops::Add; //~ ERROR import `Add` conflicts with type in this module
-use std::ops::Sub; //~ ERROR import `Sub` conflicts with type in this module
-use std::ops::Mul; //~ ERROR import `Mul` conflicts with type in this module
-use std::ops::Div; //~ ERROR import `Div` conflicts with existing submodule
-use std::ops::Rem; //~ ERROR import `Rem` conflicts with trait in this module
+use std::ops::Add; //~ NOTE previous import
+use std::ops::Sub; //~ NOTE previous import
+use std::ops::Mul; //~ NOTE previous import
+use std::ops::Div; //~ NOTE previous import
+use std::ops::Rem; //~ NOTE previous import
 
-type Add = bool;
-struct Sub { x: f32 }
-enum Mul { A, B }
-mod Div { }
-trait Rem { }
+type Add = bool; //~ ERROR a trait named `Add` has already been imported in this module
+struct Sub { x: f32 } //~ ERROR a trait named `Sub` has already been imported in this module
+enum Mul { A, B } //~ ERROR a trait named `Mul` has already been imported in this module
+mod Div { } //~ ERROR a trait named `Div` has already been imported in this module
+trait Rem {  } //~ ERROR a trait named `Rem` has already been imported in this module
 
 fn main() {}

src/test/compile-fail/issue-28472.rs

@@ -13,10 +13,10 @@
 extern {
   fn foo();
 
-  pub //~ ERROR duplicate definition
+  pub //~ ERROR a value named `foo` has already been defined
   fn foo();
 
-  pub //~ ERROR duplicate definition
+  pub //~ ERROR a value named `foo` has already been defined
   static mut foo: u32;
 }