From 883f5e5e657e714650d34cfb271abd8015210d08 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Fri, 24 Nov 2017 10:26:42 -0800 Subject: [PATCH 1/2] one-time diagnostics: span_suggestion, generalize methods for non-lints 304c8b1edabcd made the Session's one-time-diagnostics set take a special-purpose `DiagnosticMessageId` enum rather than a LintID so that it could support more than just lints, but the `diag_span_note_once` and `diag_note_once` methods continued to take references to lints: for API consistency, we now make these methods take a `DiagnosticMessageId` while we add support for one-time span-suggestions. --- src/librustc/lint/mod.rs | 13 ++++++------ src/librustc/session/mod.rs | 40 +++++++++++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 906cae53710ff..f0761ce617865 100644 --- a/src/librustc/lint/mod.rs +++ b/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)); } diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs index df5805bacd41a..36c1966bdc834 100644 --- a/src/librustc/session/mod.rs +++ b/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,10 +365,11 @@ 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) { + msg_id: DiagnosticMessageId, + message: &str, + span_maybe: Option) { - 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 { @@ -369,7 +377,12 @@ impl Session { 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); } } } @@ -377,14 +390,25 @@ impl Session { 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 { From 4fb57e0796cc61bec9d6a8a0392bbfe5855d693e Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Fri, 24 Nov 2017 11:21:43 -0800 Subject: [PATCH 2/2] =?UTF-8?q?one-time=20diagnostic=20and=20suggestion=20?= =?UTF-8?q?for=20re=C3=ABxporting=20private=20variant=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We issue just one message for an erroneous glob private variant reëxport (using the Session's one-time-diagnostics capability), but individual (non-glob) such erroneous reëxports still get their own messages. The suggestion to make the enum public is also one-time. The enum variant reëxport error didn't have an associated error code (and remedying this here is deemed out of the scope of this commit), so we resort to the expediency of using 0 as the `DiagnosticMessageId` value. Adding Debug to NameResolution was helpful in development. This resolves #46209. --- src/librustc_resolve/resolve_imports.rs | 60 +++++++++++++++++-- ...sue-46209-private-enum-variant-reexport.rs | 51 ++++++++++++++++ .../compile-fail/private-variant-reexport.rs | 8 +-- 3 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 src/test/compile-fail/issue-46209-private-enum-variant-reexport.rs diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index d72253e5a8a48..d4a08d643ab67 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/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, .. } diff --git a/src/test/compile-fail/issue-46209-private-enum-variant-reexport.rs b/src/test/compile-fail/issue-46209-private-enum-variant-reexport.rs new file mode 100644 index 0000000000000..5b23e5e815053 --- /dev/null +++ b/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 or the MIT license +// , 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() {} diff --git a/src/test/compile-fail/private-variant-reexport.rs b/src/test/compile-fail/private-variant-reexport.rs index c77a7532e34a2..1280aba3076ab 100644 --- a/src/test/compile-fail/private-variant-reexport.rs +++ b/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 }