Auto merge of #50665 - alexcrichton:fix-single-item-path-warnings, r=oli-obk

rustc: Fix `crate` lint for single-item paths

This commit fixes recommending the `crate` prefix when migrating to 2018 for
paths that look like `use foo;` or `use {bar, baz}`

Closes #50660

Author: bors

src/librustc/lint/builtin.rs

@@ -261,7 +261,7 @@ declare_lint! {
 }
 
 declare_lint! {
-    pub ABSOLUTE_PATH_STARTING_WITH_MODULE,
+    pub ABSOLUTE_PATH_NOT_STARTING_WITH_CRATE,
     Allow,
     "fully qualified paths that start with a module name \
      instead of `crate`, `self`, or an extern crate name"
@@ -328,7 +328,7 @@ impl LintPass for HardwiredLints {
             TYVAR_BEHIND_RAW_POINTER,
             ELIDED_LIFETIME_IN_PATH,
             BARE_TRAIT_OBJECT,
-            ABSOLUTE_PATH_STARTING_WITH_MODULE,
+            ABSOLUTE_PATH_NOT_STARTING_WITH_CRATE,
             UNSTABLE_NAME_COLLISION,
         )
     }

src/librustc_lint/lib.rs

@@ -40,7 +40,7 @@ extern crate rustc_target;
 extern crate syntax_pos;
 
 use rustc::lint;
-use rustc::lint::builtin::{BARE_TRAIT_OBJECT, ABSOLUTE_PATH_STARTING_WITH_MODULE};
+use rustc::lint::builtin::{BARE_TRAIT_OBJECT, ABSOLUTE_PATH_NOT_STARTING_WITH_CRATE};
 use rustc::session;
 use rustc::util;
 
@@ -283,7 +283,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             //       standard library, and thus should never be removed or changed to an error.
         },
         FutureIncompatibleInfo {
-            id: LintId::of(ABSOLUTE_PATH_STARTING_WITH_MODULE),
+            id: LintId::of(ABSOLUTE_PATH_NOT_STARTING_WITH_CRATE),
             reference: "issue TBD",
             edition: Some(Edition::Edition2018),
         },
@@ -322,4 +322,6 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
         "converted into hard error, see https://github.com/rust-lang/rust/issues/48950");
     store.register_removed("resolve_trait_on_defaulted_unit",
         "converted into hard error, see https://github.com/rust-lang/rust/issues/48950");
+    store.register_removed("absolute_path_starting_with_module",
+        "renamed to `absolute_path_not_starting_with_crate`");
 }

src/librustc_resolve/lib.rs

@@ -3232,6 +3232,7 @@ impl<'a> Resolver<'a> {
                     -> PathResult<'a> {
         let mut module = None;
         let mut allow_super = true;
+        let mut second_binding = None;
 
         for (i, &ident) in path.iter().enumerate() {
             debug!("resolve_path ident {} {:?}", i, ident);
@@ -3321,7 +3322,9 @@ impl<'a> Resolver<'a> {
                     .map(MacroBinding::binding)
             } else {
                 match self.resolve_ident_in_lexical_scope(ident, ns, record_used, path_span) {
+                    // we found a locally-imported or available item/module
                     Some(LexicalScopeBinding::Item(binding)) => Ok(binding),
+                    // we found a local variable or type param
                     Some(LexicalScopeBinding::Def(def))
                             if opt_ns == Some(TypeNS) || opt_ns == Some(ValueNS) => {
                         return PathResult::NonModule(PathResolution::with_unresolved_segments(
@@ -3334,13 +3337,22 @@ impl<'a> Resolver<'a> {
 
             match binding {
                 Ok(binding) => {
+                    if i == 1 {
+                        second_binding = Some(binding);
+                    }
                     let def = binding.def();
                     let maybe_assoc = opt_ns != Some(MacroNS) && PathSource::Type.is_expected(def);
                     if let Some(next_module) = binding.module() {
                         module = Some(next_module);
                     } else if def == Def::Err {
                         return PathResult::NonModule(err_path_resolution());
                     } else if opt_ns.is_some() && (is_last || maybe_assoc) {
+                        self.lint_if_path_starts_with_module(
+                            node_id,
+                            path,
+                            path_span,
+                            second_binding,
+                        );
                         return PathResult::NonModule(PathResolution::with_unresolved_segments(
                             def, path.len() - i - 1
                         ));
@@ -3349,33 +3361,6 @@ impl<'a> Resolver<'a> {
                                                   format!("Not a module `{}`", ident),
                                                   is_last);
                     }
-
-                    if let Some(id) = node_id {
-                        if i == 1 && self.session.features_untracked().crate_in_paths
-                                  && !self.session.rust_2018() {
-                            let prev_name = path[0].name;
-                            if prev_name == keywords::Extern.name() ||
-                               prev_name == keywords::CrateRoot.name() {
-                                let mut is_crate = false;
-                                if let NameBindingKind::Import { directive: d, .. } = binding.kind {
-                                    if let ImportDirectiveSubclass::ExternCrate(..) = d.subclass {
-                                        is_crate = true;
-                                    }
-                                }
-
-                                if !is_crate {
-                                    let diag = lint::builtin::BuiltinLintDiagnostics
-                                                   ::AbsPathWithModule(path_span);
-                                    self.session.buffer_lint_with_diagnostic(
-                                        lint::builtin::ABSOLUTE_PATH_STARTING_WITH_MODULE,
-                                        id, path_span,
-                                        "Absolute paths must start with `self`, `super`, \
-                                        `crate`, or an external crate name in the 2018 edition",
-                                        diag);
-                                }
-                            }
-                        }
-                    }
                 }
                 Err(Undetermined) => return PathResult::Indeterminate,
                 Err(Determined) => {
@@ -3408,9 +3393,77 @@ impl<'a> Resolver<'a> {
             }
         }
 
+        self.lint_if_path_starts_with_module(node_id, path, path_span, second_binding);
+
         PathResult::Module(module.unwrap_or(self.graph_root))
     }
 
+    fn lint_if_path_starts_with_module(&self,
+                                       id: Option<NodeId>,
+                                       path: &[Ident],
+                                       path_span: Span,
+                                       second_binding: Option<&NameBinding>) {
+        let id = match id {
+            Some(id) => id,
+            None => return,
+        };
+
+        let first_name = match path.get(0) {
+            Some(ident) => ident.name,
+            None => return,
+        };
+
+        // We're only interested in `use` paths which should start with
+        // `{{root}}` or `extern` currently.
+        if first_name != keywords::Extern.name() && first_name != keywords::CrateRoot.name() {
+            return
+        }
+
+        match path.get(1) {
+            // If this import looks like `crate::...` it's already good
+            Some(name) if name.name == keywords::Crate.name() => return,
+            // Otherwise go below to see if it's an extern crate
+            Some(_) => {}
+            // If the path has length one (and it's `CrateRoot` most likely)
+            // then we don't know whether we're gonna be importing a crate or an
+            // item in our crate. Defer this lint to elsewhere
+            None => return,
+        }
+
+        // If the first element of our path was actually resolved to an
+        // `ExternCrate` (also used for `crate::...`) then no need to issue a
+        // warning, this looks all good!
+        if let Some(binding) = second_binding {
+            if let NameBindingKind::Import { directive: d, .. } = binding.kind {
+                if let ImportDirectiveSubclass::ExternCrate(..) = d.subclass {
+                    return
+                }
+            }
+        }
+
+        self.lint_path_starts_with_module(id, path_span);
+    }
+
+    fn lint_path_starts_with_module(&self, id: NodeId, span: Span) {
+        // In the 2018 edition this lint is a hard error, so nothing to do
+        if self.session.rust_2018() {
+            return
+        }
+        // In the 2015 edition there's no use in emitting lints unless the
+        // crate's already enabled the feature that we're going to suggest
+        if !self.session.features_untracked().crate_in_paths {
+            return
+        }
+        let diag = lint::builtin::BuiltinLintDiagnostics
+            ::AbsPathWithModule(span);
+        self.session.buffer_lint_with_diagnostic(
+            lint::builtin::ABSOLUTE_PATH_NOT_STARTING_WITH_CRATE,
+            id, span,
+            "absolute paths must start with `self`, `super`, \
+            `crate`, or an external crate name in the 2018 edition",
+            diag);
+    }
+
     // Resolve a local definition, potentially adjusting for closures.
     fn adjust_local_def(&mut self,
                         ns: Namespace,

src/librustc_resolve/resolve_imports.rs

@@ -640,6 +640,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
     fn finalize_import(&mut self, directive: &'b ImportDirective<'b>) -> Option<(Span, String)> {
         self.current_module = directive.parent;
         let ImportDirective { ref module_path, span, .. } = *directive;
+        let mut warn_if_binding_comes_from_local_crate = false;
 
         // FIXME: Last path segment is treated specially in import resolution, so extern crate
         // mode for absolute paths needs some special support for single-segment imports.
@@ -653,6 +654,9 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                     return Some((directive.span,
                                  "cannot glob-import all possible crates".to_string()));
                 }
+                GlobImport { .. } if self.session.features_untracked().extern_absolute_paths => {
+                    self.lint_path_starts_with_module(directive.id, span);
+                }
                 SingleImport { source, target, .. } => {
                     let crate_root = if source.name == keywords::Crate.name() &&
                                         module_path[0].name != keywords::Extern.name() {
@@ -676,6 +680,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                         self.populate_module_if_necessary(crate_root);
                         Some(crate_root)
                     } else {
+                        warn_if_binding_comes_from_local_crate = true;
                         None
                     };
 
@@ -870,6 +875,26 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
             }
         }
 
+        if warn_if_binding_comes_from_local_crate {
+            let mut warned = false;
+            self.per_ns(|this, ns| {
+                let binding = match result[ns].get().ok() {
+                    Some(b) => b,
+                    None => return
+                };
+                if let NameBindingKind::Import { directive: d, .. } = binding.kind {
+                    if let ImportDirectiveSubclass::ExternCrate(..) = d.subclass {
+                        return
+                    }
+                }
+                if warned {
+                    return
+                }
+                warned = true;
+                this.lint_path_starts_with_module(directive.id, span);
+            });
+        }
+
         // Record what this import resolves to for later uses in documentation,
         // this may resolve to either a value or a type, but for documentation
         // purposes it's good enough to just favor one over the other.

src/test/ui/auxiliary/edition-lint-paths.rs

@@ -0,0 +1,11 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+pub fn foo() {}

src/test/ui/edition-lint-paths.fixed

@@ -0,0 +1,77 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// aux-build:edition-lint-paths.rs
+// run-rustfix
+
+#![feature(rust_2018_preview)]
+#![deny(absolute_path_not_starting_with_crate)]
+#![allow(unused)]
+
+extern crate edition_lint_paths;
+
+pub mod foo {
+    use edition_lint_paths;
+    use crate::bar::Bar;
+    //~^ ERROR absolute
+    //~| WARN this was previously accepted
+    use super::bar::Bar2;
+    use crate::bar::Bar3;
+
+    use crate::bar;
+    //~^ ERROR absolute
+    //~| WARN this was previously accepted
+    use crate::{bar as something_else};
+
+    use {crate::Bar as SomethingElse, crate::main};
+    //~^ ERROR absolute
+    //~| WARN this was previously accepted
+    //~| ERROR absolute
+    //~| WARN this was previously accepted
+
+    use crate::{Bar as SomethingElse2, main as another_main};
+
+    pub fn test() {
+    }
+}
+
+use crate::bar::Bar;
+//~^ ERROR absolute
+//~| WARN this was previously accepted
+
+pub mod bar {
+    use edition_lint_paths as foo;
+    pub struct Bar;
+    pub type Bar2 = Bar;
+    pub type Bar3 = Bar;
+}
+
+mod baz {
+    use crate::*;
+    //~^ ERROR absolute
+    //~| WARN this was previously accepted
+}
+
+fn main() {
+    let x = crate::bar::Bar;
+    //~^ ERROR absolute
+    //~| WARN this was previously accepted
+    let x = bar::Bar;
+    let x = ::crate::bar::Bar;
+    let x = self::bar::Bar;
+    foo::test();
+
+    {
+        use edition_lint_paths as bar;
+        edition_lint_paths::foo();
+        bar::foo();
+        ::edition_lint_paths::foo();
+    }
+}