Address review comments and cleanup code

Author: petrochenkov

src/librustc_resolve/lib.rs

@@ -5090,62 +5090,59 @@ impl<'a> Resolver<'a> {
         }
 
         // See https://github.com/rust-lang/rust/issues/32354
-        if old_binding.is_import() || new_binding.is_import() {
-            let binding = if new_binding.is_import() && !new_binding.span.is_dummy() {
-                new_binding
+        let directive = match (&new_binding.kind, &old_binding.kind) {
+            (NameBindingKind::Import { directive, .. }, _) if !new_binding.span.is_dummy() =>
+                Some((directive, new_binding.span)),
+            (_, NameBindingKind::Import { directive, .. }) if !old_binding.span.is_dummy() =>
+                Some((directive, old_binding.span)),
+            _ => None,
+        };
+        if let Some((directive, binding_span)) = directive {
+            let suggested_name = if name.as_str().chars().next().unwrap().is_uppercase() {
+                format!("Other{}", name)
             } else {
-                old_binding
+                format!("other_{}", name)
             };
 
-            let cm = self.session.source_map();
-            let rename_msg = "you can use `as` to change the binding name of the import";
-
-            if let (
-                Ok(snippet),
-                NameBindingKind::Import { directive, ..},
-                false,
-                false,
-            ) = (
-                cm.span_to_snippet(binding.span),
-                binding.kind.clone(),
-                binding.span.is_dummy(),
-                binding.span.ctxt().outer().expn_info().is_some(),
-            ) {
-                let suggested_name = if name.as_str().chars().next().unwrap().is_uppercase() {
-                    format!("Other{}", name)
-                } else {
-                    format!("other_{}", name)
-                };
+            let mut suggestion = None;
+            match directive.subclass {
+                ImportDirectiveSubclass::SingleImport { type_ns_only: true, .. } =>
+                    suggestion = Some(format!("self as {}", suggested_name)),
+                ImportDirectiveSubclass::SingleImport { source, .. } => {
+                    if let Some(pos) = source.span.hi().0.checked_sub(binding_span.lo().0)
+                                                         .map(|pos| pos as usize) {
+                        if let Ok(snippet) = self.session.source_map()
+                                                         .span_to_snippet(binding_span) {
+                            if pos <= snippet.len() {
+                                suggestion = Some(format!(
+                                    "{} as {}{}",
+                                    &snippet[..pos],
+                                    suggested_name,
+                                    if snippet.ends_with(";") { ";" } else { "" }
+                                ))
+                            }
+                        }
+                    }
+                }
+                ImportDirectiveSubclass::ExternCrate { source, target, .. } =>
+                    suggestion = Some(format!(
+                        "extern crate {} as {};",
+                        source.unwrap_or(target.name),
+                        suggested_name,
+                    )),
+                _ => unreachable!(),
+            }
 
+            let rename_msg = "you can use `as` to change the binding name of the import";
+            if let Some(suggestion) = suggestion {
                 err.span_suggestion_with_applicability(
-                    binding.span,
-                    &rename_msg,
-                    match directive.subclass {
-                        ImportDirectiveSubclass::SingleImport { type_ns_only: true, .. } =>
-                            format!("self as {}", suggested_name),
-                        ImportDirectiveSubclass::SingleImport { source, .. } =>
-                            format!(
-                                "{} as {}{}",
-                                &snippet[..((source.span.hi().0 - binding.span.lo().0) as usize)],
-                                suggested_name,
-                                if snippet.ends_with(";") {
-                                    ";"
-                                } else {
-                                    ""
-                                }
-                            ),
-                        ImportDirectiveSubclass::ExternCrate { source, target, .. } =>
-                            format!(
-                                "extern crate {} as {};",
-                                source.unwrap_or(target.name),
-                                suggested_name,
-                            ),
-                        _ => unreachable!(),
-                    },
+                    binding_span,
+                    rename_msg,
+                    suggestion,
                     Applicability::MaybeIncorrect,
                 );
             } else {
-                err.span_label(binding.span, rename_msg);
+                err.span_label(binding_span, rename_msg);
             }
         }
 

src/test/ui/issues/issue-56411.rs

@@ -3,14 +3,14 @@ macro_rules! import {
         $(
             mod $name;
             pub use self::$name;
-            //~^ ERROR the name `issue_56411` is defined multiple times
-            //~| ERROR `issue_56411` is private, and cannot be re-exported
+            //~^ ERROR the name `issue_56411_aux` is defined multiple times
+            //~| ERROR `issue_56411_aux` is private, and cannot be re-exported
 
         )*
     }
 }
 
-import!(issue_56411);
+import!(issue_56411_aux);
 
 fn main() {
     println!("Hello, world!");

src/test/ui/issues/issue-56411.stderr

@@ -1,29 +1,29 @@
-error[E0255]: the name `issue_56411` is defined multiple times
+error[E0255]: the name `issue_56411_aux` is defined multiple times
   --> $DIR/issue-56411.rs:5:21
    |
 LL |             mod $name;
-   |             ---------- previous definition of the module `issue_56411` here
+   |             ---------- previous definition of the module `issue_56411_aux` here
 LL |             pub use self::$name;
    |                     ^^^^^^^^^^^
    |                     |
-   |                     `issue_56411` reimported here
+   |                     `issue_56411_aux` reimported here
    |                     you can use `as` to change the binding name of the import
 ...
-LL | import!(issue_56411);
-   | --------------------- in this macro invocation
+LL | import!(issue_56411_aux);
+   | ------------------------- in this macro invocation
    |
-   = note: `issue_56411` must be defined only once in the type namespace of this module
+   = note: `issue_56411_aux` must be defined only once in the type namespace of this module
 
-error[E0365]: `issue_56411` is private, and cannot be re-exported
+error[E0365]: `issue_56411_aux` is private, and cannot be re-exported
   --> $DIR/issue-56411.rs:5:21
    |
 LL |             pub use self::$name;
-   |                     ^^^^^^^^^^^ re-export of private `issue_56411`
+   |                     ^^^^^^^^^^^ re-export of private `issue_56411_aux`
 ...
-LL | import!(issue_56411);
-   | --------------------- in this macro invocation
+LL | import!(issue_56411_aux);
+   | ------------------------- in this macro invocation
    |
-   = note: consider declaring type or module `issue_56411` with `pub`
+   = note: consider declaring type or module `issue_56411_aux` with `pub`
 
 error: aborting due to 2 previous errors