Auto merge of #46248 - zackmdavis:one_time_private_enum_variant_reexport_error, r=estebank

one-time diagnostics for private enum variants glob reëxport

![private_enum_reexport](https://user-images.githubusercontent.com/1076988/33224719-4e5805f0-d121-11e7-8bc0-a708a277a5db.png)

r? @estebank

Author: bors

src/librustc/lint/mod.rs

@@ -37,7 +37,7 @@ use errors::{DiagnosticBuilder, DiagnosticId};
 use hir::def_id::{CrateNum, LOCAL_CRATE};
 use hir::intravisit::{self, FnKind};
 use hir;
-use session::Session;
+use session::{Session, DiagnosticMessageId};
 use std::hash;
 use syntax::ast;
 use syntax::codemap::MultiSpan;
@@ -423,7 +423,7 @@ pub fn struct_lint_level<'a>(sess: &'a Session,
         LintSource::Default => {
             sess.diag_note_once(
                 &mut err,
-                lint,
+                DiagnosticMessageId::from(lint),
                 &format!("#[{}({})] on by default", level.as_str(), name));
         }
         LintSource::CommandLine(lint_flag_val) => {
@@ -437,24 +437,25 @@ pub fn struct_lint_level<'a>(sess: &'a Session,
             if lint_flag_val.as_str() == name {
                 sess.diag_note_once(
                     &mut err,
-                    lint,
+                    DiagnosticMessageId::from(lint),
                     &format!("requested on the command line with `{} {}`",
                              flag, hyphen_case_lint_name));
             } else {
                 let hyphen_case_flag_val = lint_flag_val.as_str().replace("_", "-");
                 sess.diag_note_once(
                     &mut err,
-                    lint,
+                    DiagnosticMessageId::from(lint),
                     &format!("`{} {}` implied by `{} {}`",
                              flag, hyphen_case_lint_name, flag,
                              hyphen_case_flag_val));
             }
         }
         LintSource::Node(lint_attr_name, src) => {
-            sess.diag_span_note_once(&mut err, lint, src, "lint level defined here");
+            sess.diag_span_note_once(&mut err, DiagnosticMessageId::from(lint),
+                                     src, "lint level defined here");
             if lint_attr_name.as_str() != name {
                 let level_str = level.as_str();
-                sess.diag_note_once(&mut err, lint,
+                sess.diag_note_once(&mut err, DiagnosticMessageId::from(lint),
                                     &format!("#[{}({})] implied by #[{}({})]",
                                              level_str, name, level_str, lint_attr_name));
             }

src/librustc/session/mod.rs

@@ -161,6 +161,7 @@ pub struct PerfStats {
 enum DiagnosticBuilderMethod {
     Note,
     SpanNote,
+    SpanSuggestion(String), // suggestion
     // add more variants as needed to support one-time diagnostics
 }
 
@@ -173,6 +174,12 @@ pub enum DiagnosticMessageId {
     StabilityId(u32) // issue number
 }
 
+impl From<&'static lint::Lint> for DiagnosticMessageId {
+    fn from(lint: &'static lint::Lint) -> Self {
+        DiagnosticMessageId::LintId(lint::LintId::of(lint))
+    }
+}
+
 impl Session {
     pub fn local_crate_disambiguator(&self) -> CrateDisambiguator {
         match *self.crate_disambiguator.borrow() {
@@ -358,33 +365,50 @@ impl Session {
     fn diag_once<'a, 'b>(&'a self,
                          diag_builder: &'b mut DiagnosticBuilder<'a>,
                          method: DiagnosticBuilderMethod,
-                         lint: &'static lint::Lint, message: &str, span: Option<Span>) {
+                         msg_id: DiagnosticMessageId,
+                         message: &str,
+                         span_maybe: Option<Span>) {
 
-        let lint_id = DiagnosticMessageId::LintId(lint::LintId::of(lint));
-        let id_span_message = (lint_id, span, message.to_owned());
+        let id_span_message = (msg_id, span_maybe, message.to_owned());
         let fresh = self.one_time_diagnostics.borrow_mut().insert(id_span_message);
         if fresh {
             match method {
                 DiagnosticBuilderMethod::Note => {
                     diag_builder.note(message);
                 },
                 DiagnosticBuilderMethod::SpanNote => {
-                    diag_builder.span_note(span.expect("span_note expects a span"), message);
+                    let span = span_maybe.expect("span_note needs a span");
+                    diag_builder.span_note(span, message);
+                },
+                DiagnosticBuilderMethod::SpanSuggestion(suggestion) => {
+                    let span = span_maybe.expect("span_suggestion needs a span");
+                    diag_builder.span_suggestion(span, message, suggestion);
                 }
             }
         }
     }
 
     pub fn diag_span_note_once<'a, 'b>(&'a self,
                                        diag_builder: &'b mut DiagnosticBuilder<'a>,
-                                       lint: &'static lint::Lint, span: Span, message: &str) {
-        self.diag_once(diag_builder, DiagnosticBuilderMethod::SpanNote, lint, message, Some(span));
+                                       msg_id: DiagnosticMessageId, span: Span, message: &str) {
+        self.diag_once(diag_builder, DiagnosticBuilderMethod::SpanNote,
+                       msg_id, message, Some(span));
     }
 
     pub fn diag_note_once<'a, 'b>(&'a self,
                                   diag_builder: &'b mut DiagnosticBuilder<'a>,
-                                  lint: &'static lint::Lint, message: &str) {
-        self.diag_once(diag_builder, DiagnosticBuilderMethod::Note, lint, message, None);
+                                  msg_id: DiagnosticMessageId, message: &str) {
+        self.diag_once(diag_builder, DiagnosticBuilderMethod::Note, msg_id, message, None);
+    }
+
+    pub fn diag_span_suggestion_once<'a, 'b>(&'a self,
+                                             diag_builder: &'b mut DiagnosticBuilder<'a>,
+                                             msg_id: DiagnosticMessageId,
+                                             span: Span,
+                                             message: &str,
+                                             suggestion: String) {
+        self.diag_once(diag_builder, DiagnosticBuilderMethod::SpanSuggestion(suggestion),
+                       msg_id, message, Some(span));
     }
 
     pub fn codemap<'a>(&'a self) -> &'a codemap::CodeMap {

src/librustc_resolve/resolve_imports.rs

@@ -21,6 +21,7 @@ use rustc::ty;
 use rustc::lint::builtin::PUB_USE_OF_PRIVATE_EXTERN_CRATE;
 use rustc::hir::def_id::DefId;
 use rustc::hir::def::*;
+use rustc::session::DiagnosticMessageId;
 use rustc::util::nodemap::{FxHashMap, FxHashSet};
 
 use syntax::ast::{Ident, Name, SpannedIdent, NodeId};
@@ -72,7 +73,7 @@ impl<'a> ImportDirective<'a> {
     }
 }
 
-#[derive(Clone, Default)]
+#[derive(Clone, Default, Debug)]
 /// Records information about the resolution of a name in a namespace of a module.
 pub struct NameResolution<'a> {
     /// The single imports that define the name in the namespace.
@@ -867,12 +868,59 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
             }
 
             match binding.kind {
-                NameBindingKind::Import { binding: orig_binding, .. } => {
+                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, \
-                                           consider declaring its enum as `pub`", ident);
-                        self.session.span_err(binding.span, &msg);
+                        !orig_binding.vis.is_at_least(binding.vis, &*self) {
+                            let msg = match directive.subclass {
+                                ImportDirectiveSubclass::SingleImport { .. } => {
+                                    format!("variant `{}` is private and cannot be reexported",
+                                            ident)
+                                },
+                                ImportDirectiveSubclass::GlobImport { .. } => {
+                                    let msg = "enum is private and its variants \
+                                               cannot be reexported".to_owned();
+                                    let error_id = (DiagnosticMessageId::ErrorId(0), // no code?!
+                                                    Some(binding.span),
+                                                    msg.clone());
+                                    let fresh = self.session.one_time_diagnostics
+                                        .borrow_mut().insert(error_id);
+                                    if !fresh {
+                                        continue;
+                                    }
+                                    msg
+                                },
+                                ref s @ _ => bug!("unexpected import subclass {:?}", s)
+                            };
+                            let mut err = self.session.struct_span_err(binding.span, &msg);
+
+                            let imported_module = directive.imported_module.get()
+                                .expect("module should exist");
+                            let resolutions = imported_module.parent.expect("parent should exist")
+                                .resolutions.borrow();
+                            let enum_path_segment_index = directive.module_path.len() - 1;
+                            let enum_ident = directive.module_path[enum_path_segment_index].node;
+
+                            let enum_resolution = resolutions.get(&(enum_ident, TypeNS))
+                                .expect("resolution should exist");
+                            let enum_span = enum_resolution.borrow()
+                                .binding.expect("binding should exist")
+                                .span;
+                            let enum_def_span = self.session.codemap().def_span(enum_span);
+                            let enum_def_snippet = self.session.codemap()
+                                .span_to_snippet(enum_def_span).expect("snippet should exist");
+                            // potentially need to strip extant `crate`/`pub(path)` for suggestion
+                            let after_vis_index = enum_def_snippet.find("enum")
+                                .expect("`enum` keyword should exist in snippet");
+                            let suggestion = format!("pub {}",
+                                                     &enum_def_snippet[after_vis_index..]);
+
+                            self.session
+                                .diag_span_suggestion_once(&mut err,
+                                                           DiagnosticMessageId::ErrorId(0),
+                                                           enum_def_span,
+                                                           "consider making the enum public",
+                                                           suggestion);
+                            err.emit();
                     }
                 }
                 NameBindingKind::Ambiguity { b1, b2, .. }

src/test/compile-fail/issue-46209-private-enum-variant-reexport.rs

@@ -0,0 +1,51 @@
+// Copyright 2017 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(crate_visibility_modifier)]
+
+mod rank {
+    pub use self::Professor::*;
+    //~^ ERROR enum is private and its variants cannot be reexported
+    pub use self::Lieutenant::{JuniorGrade, Full};
+    //~^ ERROR variant `JuniorGrade` is private and cannot be reexported
+    //~| ERROR variant `Full` is private and cannot be reexported
+    pub use self::PettyOfficer::*;
+    //~^ ERROR enum is private and its variants cannot be reexported
+    pub use self::Crewman::*;
+    //~^ ERROR enum is private and its variants cannot be reexported
+
+    enum Professor {
+        Adjunct,
+        Assistant,
+        Associate,
+        Full
+    }
+
+    enum Lieutenant {
+        JuniorGrade,
+        Full,
+    }
+
+    pub(in rank) enum PettyOfficer {
+        SecondClass,
+        FirstClass,
+        Chief,
+        MasterChief
+    }
+
+    crate enum Crewman {
+        Recruit,
+        Apprentice,
+        Full
+    }
+
+}
+
+fn main() {}

src/test/compile-fail/private-variant-reexport.rs

@@ -9,19 +9,19 @@
 // except according to those terms.
 
 mod m1 {
-    pub use ::E::V; //~ ERROR variant `V` is private, and cannot be reexported
+    pub use ::E::V; //~ ERROR variant `V` is private and cannot be reexported
 }
 
 mod m2 {
-    pub use ::E::{V}; //~ ERROR variant `V` is private, and cannot be reexported
+    pub use ::E::{V}; //~ ERROR variant `V` is private and cannot be reexported
 }
 
 mod m3 {
-    pub use ::E::V::{self}; //~ ERROR variant `V` is private, and cannot be reexported
+    pub use ::E::V::{self}; //~ ERROR variant `V` is private and cannot be reexported
 }
 
 mod m4 {
-    pub use ::E::*; //~ ERROR variant `V` is private, and cannot be reexported
+    pub use ::E::*; //~ ERROR enum is private and its variants cannot be reexported
 }
 
 enum E { V }