Auto merge of #38271 - jseyfried:rfc_1560_warning_cycle, r=nrc

resolve: change most backwards incompatible ambiguity errors to `legacy_imports` warnings

Fixes #38176.
r? @nrc or @nikomatsakis

Author: bors

src/librustc/lint/builtin.rs

@@ -211,6 +211,12 @@ declare_lint! {
      not named `mod.rs`"
 }
 
+declare_lint! {
+    pub LEGACY_IMPORTS,
+    Warn,
+    "detects names that resolve to ambiguous glob imports with RFC 1560"
+}
+
 declare_lint! {
     pub DEPRECATED,
     Warn,
@@ -257,6 +263,7 @@ impl LintPass for HardwiredLints {
             PATTERNS_IN_FNS_WITHOUT_BODY,
             EXTRA_REQUIREMENT_IN_IMPL,
             LEGACY_DIRECTORY_OWNERSHIP,
+            LEGACY_IMPORTS,
             DEPRECATED
         )
     }

src/librustc_lint/lib.rs

@@ -234,6 +234,10 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             id: LintId::of(LEGACY_DIRECTORY_OWNERSHIP),
             reference: "issue #37872 <https://github.com/rust-lang/rust/issues/37872>",
         },
+        FutureIncompatibleInfo {
+            id: LintId::of(LEGACY_IMPORTS),
+            reference: "issue #38260 <https://github.com/rust-lang/rust/issues/38260>",
+        },
         ]);
 
     // Register renamed and removed lints

src/librustc_resolve/lib.rs

@@ -64,7 +64,7 @@ use syntax::ast::{Item, ItemKind, ImplItem, ImplItemKind};
 use syntax::ast::{Local, Mutability, Pat, PatKind, Path};
 use syntax::ast::{PathSegment, PathParameters, QSelf, TraitItemKind, TraitRef, Ty, TyKind};
 
-use syntax_pos::{Span, DUMMY_SP};
+use syntax_pos::{Span, DUMMY_SP, MultiSpan};
 use errors::DiagnosticBuilder;
 
 use std::cell::{Cell, RefCell};
@@ -896,6 +896,7 @@ enum NameBindingKind<'a> {
     Ambiguity {
         b1: &'a NameBinding<'a>,
         b2: &'a NameBinding<'a>,
+        legacy: bool,
     }
 }
 
@@ -907,13 +908,15 @@ struct AmbiguityError<'a> {
     lexical: bool,
     b1: &'a NameBinding<'a>,
     b2: &'a NameBinding<'a>,
+    legacy: bool,
 }
 
 impl<'a> NameBinding<'a> {
     fn module(&self) -> Option<Module<'a>> {
         match self.kind {
             NameBindingKind::Module(module) => Some(module),
             NameBindingKind::Import { binding, .. } => binding.module(),
+            NameBindingKind::Ambiguity { legacy: true, b1, .. } => b1.module(),
             _ => None,
         }
     }
@@ -923,6 +926,7 @@ impl<'a> NameBinding<'a> {
             NameBindingKind::Def(def) => def,
             NameBindingKind::Module(module) => module.def().unwrap(),
             NameBindingKind::Import { binding, .. } => binding.def(),
+            NameBindingKind::Ambiguity { legacy: true, b1, .. } => b1.def(),
             NameBindingKind::Ambiguity { .. } => Def::Err,
         }
     }
@@ -1349,11 +1353,14 @@ impl<'a> Resolver<'a> {
                 self.record_use(name, ns, binding, span)
             }
             NameBindingKind::Import { .. } => false,
-            NameBindingKind::Ambiguity { b1, b2 } => {
+            NameBindingKind::Ambiguity { b1, b2, legacy } => {
                 self.ambiguity_errors.push(AmbiguityError {
-                    span: span, name: name, lexical: false, b1: b1, b2: b2,
+                    span: span, name: name, lexical: false, b1: b1, b2: b2, legacy: legacy,
                 });
-                true
+                if legacy {
+                    self.record_use(name, ns, b1, span);
+                }
+                !legacy
             }
             _ => false
         }
@@ -3065,26 +3072,39 @@ impl<'a> Resolver<'a> {
         self.report_shadowing_errors();
         let mut reported_spans = FxHashSet();
 
-        for &AmbiguityError { span, name, b1, b2, lexical } in &self.ambiguity_errors {
+        for &AmbiguityError { span, name, b1, b2, lexical, legacy } in &self.ambiguity_errors {
             if !reported_spans.insert(span) { continue }
             let participle = |binding: &NameBinding| {
                 if binding.is_import() { "imported" } else { "defined" }
             };
             let msg1 = format!("`{}` could resolve to the name {} here", name, participle(b1));
             let msg2 = format!("`{}` could also resolve to the name {} here", name, participle(b2));
-            self.session.struct_span_err(span, &format!("`{}` is ambiguous", name))
-                .span_note(b1.span, &msg1)
-                .span_note(b2.span, &msg2)
-                .note(&if !lexical && b1.is_glob_import() {
-                    format!("consider adding an explicit import of `{}` to disambiguate", name)
-                } else if let Def::Macro(..) = b1.def() {
-                    format!("macro-expanded {} do not shadow",
-                            if b1.is_import() { "macro imports" } else { "macros" })
-                } else {
-                    format!("macro-expanded {} do not shadow when used in a macro invocation path",
-                            if b1.is_import() { "imports" } else { "items" })
-                })
-                .emit();
+            let note = if !lexical && b1.is_glob_import() {
+                format!("consider adding an explicit import of `{}` to disambiguate", name)
+            } else if let Def::Macro(..) = b1.def() {
+                format!("macro-expanded {} do not shadow",
+                        if b1.is_import() { "macro imports" } else { "macros" })
+            } else {
+                format!("macro-expanded {} do not shadow when used in a macro invocation path",
+                        if b1.is_import() { "imports" } else { "items" })
+            };
+            if legacy {
+                let id = match b2.kind {
+                    NameBindingKind::Import { directive, .. } => directive.id,
+                    _ => unreachable!(),
+                };
+                let mut span = MultiSpan::from_span(span);
+                span.push_span_label(b1.span, msg1);
+                span.push_span_label(b2.span, msg2);
+                let msg = format!("`{}` is ambiguous", name);
+                self.session.add_lint(lint::builtin::LEGACY_IMPORTS, id, span, msg);
+            } else {
+                self.session.struct_span_err(span, &format!("`{}` is ambiguous", name))
+                    .span_note(b1.span, &msg1)
+                    .span_note(b2.span, &msg2)
+                    .note(&note)
+                    .emit();
+            }
         }
 
         for &PrivacyError(span, name, binding) in &self.privacy_errors {

src/librustc_resolve/macros.rs

@@ -288,6 +288,7 @@ impl<'a> Resolver<'a> {
                         Some(shadower) if shadower.def() != binding.def() => {
                             self.ambiguity_errors.push(AmbiguityError {
                                 span: span, name: name, b1: shadower, b2: binding, lexical: true,
+                                legacy: false,
                             });
                             return Ok(shadower);
                         }

src/librustc_resolve/resolve_imports.rs

@@ -159,6 +159,7 @@ impl<'a> Resolver<'a> {
                        binding.def() != shadowed_glob.def() {
                         self.ambiguity_errors.push(AmbiguityError {
                             span: span, name: name, lexical: false, b1: binding, b2: shadowed_glob,
+                            legacy: false,
                         });
                     }
                 }
@@ -336,9 +337,9 @@ impl<'a> Resolver<'a> {
     }
 
     pub fn ambiguity(&mut self, b1: &'a NameBinding<'a>, b2: &'a NameBinding<'a>)
-                 -> &'a NameBinding<'a> {
+                     -> &'a NameBinding<'a> {
         self.arenas.alloc_name_binding(NameBinding {
-            kind: NameBindingKind::Ambiguity { b1: b1, b2: b2 },
+            kind: NameBindingKind::Ambiguity { b1: b1, b2: b2, legacy: false },
             vis: if b1.vis.is_at_least(b2.vis, self) { b1.vis } else { b2.vis },
             span: b1.span,
             expansion: Mark::root(),
@@ -726,7 +727,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
         }
 
         for (&(name, ns), resolution) in module.resolutions.borrow().iter() {
-            let resolution = resolution.borrow();
+            let resolution = &mut *resolution.borrow_mut();
             let binding = match resolution.binding {
                 Some(binding) => binding,
                 None => continue,
@@ -743,14 +744,34 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
                 }
             }
 
-            if let NameBindingKind::Import { binding: orig_binding, directive, .. } = binding.kind {
-                if ns == TypeNS && orig_binding.is_variant() &&
-                   !orig_binding.vis.is_at_least(binding.vis, self) {
-                    let msg = format!("variant `{}` is private, and cannot be reexported \
-                                       (error E0364), consider declaring its enum as `pub`",
-                                      name);
-                    self.session.add_lint(PRIVATE_IN_PUBLIC, directive.id, binding.span, msg);
+            match binding.kind {
+                NameBindingKind::Import { binding: orig_binding, directive, .. } => {
+                    if ns == TypeNS && orig_binding.is_variant() &&
+                       !orig_binding.vis.is_at_least(binding.vis, self) {
+                        let msg = format!("variant `{}` is private, and cannot be reexported \
+                                           (error E0364), consider declaring its enum as `pub`",
+                                          name);
+                        self.session.add_lint(PRIVATE_IN_PUBLIC, directive.id, binding.span, msg);
+                    }
+                }
+                NameBindingKind::Ambiguity { b1, b2, .. }
+                        if b1.is_glob_import() && b2.is_glob_import() => {
+                    let (orig_b1, orig_b2) = match (&b1.kind, &b2.kind) {
+                        (&NameBindingKind::Import { binding: b1, .. },
+                         &NameBindingKind::Import { binding: b2, .. }) => (b1, b2),
+                        _ => continue,
+                    };
+                    let (b1, b2) = match (orig_b1.vis, orig_b2.vis) {
+                        (ty::Visibility::Public, ty::Visibility::Public) => continue,
+                        (ty::Visibility::Public, _) => (b1, b2),
+                        (_, ty::Visibility::Public) => (b2, b1),
+                        _ => continue,
+                    };
+                    resolution.binding = Some(self.arenas.alloc_name_binding(NameBinding {
+                        kind: NameBindingKind::Ambiguity { b1: b1, b2: b2, legacy: true }, ..*b1
+                    }));
                 }
+                _ => {}
             }
         }
 

src/test/compile-fail/imports/rfc-1560-warning-cycle.rs

@@ -0,0 +1,30 @@
+// Copyright 2016 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.
+
+#![feature(rustc_attrs)]
+#![allow(unused)]
+
+pub struct Foo;
+
+mod bar {
+    struct Foo;
+
+    mod baz {
+        use *; //~ NOTE `Foo` could resolve to the name imported here
+        use bar::*; //~ NOTE `Foo` could also resolve to the name imported here
+        fn f(_: Foo) {}
+        //~^ WARN `Foo` is ambiguous
+        //~| WARN hard error in a future release
+        //~| NOTE see issue #38260
+    }
+}
+
+#[rustc_error]
+fn main() {} //~ ERROR compilation successful