Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -Z teach flag to provide extended diagnostic help #47652

Merged
merged 10 commits into from
Jan 26, 2018
5 changes: 3 additions & 2 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"treat all errors that occur as bugs"),
external_macro_backtrace: bool = (false, parse_bool, [UNTRACKED],
"show macro backtraces even for non-local macros"),
teach: bool = (false, parse_bool, [TRACKED],
"show extended diagnostic help"),
continue_parse_after_error: bool = (false, parse_bool, [TRACKED],
"attempt to recover from parse errors (experimental)"),
incremental: Option<String> = (None, parse_opt_string, [UNTRACKED],
Expand Down Expand Up @@ -1620,8 +1622,7 @@ pub fn build_session_options_and_crate_config(matches: &getopts::Matches)
let mut debugging_opts = build_debugging_options(matches, error_format);

if !debugging_opts.unstable_options && error_format == ErrorOutputType::Json(true) {
early_error(ErrorOutputType::Json(false),
"--error-format=pretty-json is unstable");
early_error(ErrorOutputType::Json(false), "--error-format=pretty-json is unstable");
}

let mut output_types = BTreeMap::new();
Expand Down
4 changes: 4 additions & 0 deletions src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,10 @@ impl Session {
_ => true,
}
}

pub fn teach(&self, code: &DiagnosticId) -> bool {
self.opts.debugging_opts.teach && !self.parse_sess.span_diagnostic.code_emitted(code)
}
}

pub fn build_session(sopts: config::Options,
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_errors/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub struct Diagnostic {
pub suggestions: Vec<CodeSuggestion>,
}

#[derive(Clone, Debug, PartialEq, Hash, RustcEncodable, RustcDecodable)]
#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum DiagnosticId {
Error(String),
Lint(String),
Expand Down Expand Up @@ -281,6 +281,10 @@ impl Diagnostic {
self
}

pub fn get_code(&self) -> Option<DiagnosticId> {
self.code.clone()
}

pub fn message(&self) -> String {
self.message.iter().map(|i| i.0.to_owned()).collect::<String>()
}
Expand Down
10 changes: 10 additions & 0 deletions src/librustc_errors/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ pub struct Handler {
continue_after_error: Cell<bool>,
delayed_span_bug: RefCell<Option<Diagnostic>>,
tracked_diagnostics: RefCell<Option<Vec<Diagnostic>>>,
tracked_diagnostic_codes: RefCell<FxHashSet<DiagnosticId>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put a comment on this field explaining what it's for?


// This set contains a hash of every diagnostic that has been emitted by
// this handler. These hashes is used to avoid emitting the same error
Expand Down Expand Up @@ -303,6 +304,7 @@ impl Handler {
continue_after_error: Cell::new(true),
delayed_span_bug: RefCell::new(None),
tracked_diagnostics: RefCell::new(None),
tracked_diagnostic_codes: RefCell::new(FxHashSet()),
emitted_diagnostics: RefCell::new(FxHashSet()),
}
}
Expand Down Expand Up @@ -575,13 +577,21 @@ impl Handler {
(ret, diagnostics)
}

pub fn code_emitted(&self, code: &DiagnosticId) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment (short one would suffice)

e.g., /// True if a diagnostic with this code has already been emitted. Used to suppress duplicate messages with -Zteach.

self.tracked_diagnostic_codes.borrow().contains(code)
}

fn emit_db(&self, db: &DiagnosticBuilder) {
let diagnostic = &**db;

if let Some(ref mut list) = *self.tracked_diagnostics.borrow_mut() {
list.push(diagnostic.clone());
}

if let Some(ref code) = diagnostic.code {
self.tracked_diagnostic_codes.borrow_mut().insert(code.clone());
}

let diagnostic_hash = {
use std::hash::Hash;
let mut hasher = StableHasher::new();
Expand Down
10 changes: 6 additions & 4 deletions src/librustc_typeck/check/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,12 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> {
.emit();
}
CastError::SizedUnsizedCast => {
type_error_struct!(fcx.tcx.sess, self.span, self.expr_ty, E0607,
"cannot cast thin pointer `{}` to fat pointer `{}`",
self.expr_ty,
fcx.ty_to_string(self.cast_ty)).emit();
use structured_errors::{SizedUnsizedCastError, StructuredDiagnostic};
SizedUnsizedCastError::new(&fcx.tcx.sess,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd rather something with labels I think, like:

SizedUnsizedCastError {
    span: self.span,
    thin_ty: self.expr_ty,
    fat_ty: fcx.ty_to_string(self.cast_ty) // although if we could do `self.cast_ty` it'd be even better
}.emit(fcx.tcx);

These labels would then also appear in the templating logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that the templating could look something like the following:

diagnostic_template!(
Any independent text will be notes.

Such as this one and the previous one.

Here is where a span would appear:
$secondary_span the label for the span immediately after

I haven't thought about how to deal with suggestions.
)
error[E0XXX]: error message outside of template
  --> $FILE:21:9
   |
21 |         foo();
   |         ^^^^^
   = note: Any independent text will be notes.

          Such as this one and the previous one.
note: Here is where a span would appear:
  --> $FILE:21:9
   |
21 |         foo();
   |         ^^^^^ the label for the span immediately after
   = note: I haven't thought about how to deal with suggestions.

In the background it would convert the template string into calls to err.*().

How does that sound?

Copy link
Contributor

@nikomatsakis nikomatsakis Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Quite different than what I had in mind, but I like it as a start, and maybe more practical than some of my more grandiose ideas =)

I think one key question is whether the --teach output would still have the "basic structure" of the messages that we've been giving (as in this PR), or whether it would change to something more essay like.

I think I had initially imagined that, in --teach mode, we would change "our look" somewhat drastically. e.g (using E0010 as an example):

error[E0010]: boxed values cannot be used in statics
  --> $FILE:21:9
  |
21 |         const CON : Box<i32> = box 0;
   |                                ^^^

The value of statics and constants must be known at compile time, and they live
for the entire lifetime of a program. Creating a boxed value allocates memory on
the heap at runtime, and therefore cannot be done at compile time.

For more complex examples, there might be multiple snippets:

error[E0010]: shadowed constant `CON`
  --> $FILE:21:9
  |
21 |         const CON : Box<i32> = box 0;
   |               ^^^

This code is illegal. The problem is that the const CON 
is shadowed by another constant declared earlier, also named `CON`:

  --> $FILE:18:9
  |
18 |         const CON : Box<i32> = box 22;
   |               ^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I wasn't considering moving in that direction is because I though we wouldn't want to present the same explanation over and over again, so mixing the current diagnostics with the teach diagnostics would be a bit jarring. But if we don't mind either mixing both styles or repeating walls of text, then I don't see why we wouldn't do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think that the differences in output could act like a stylesheet, where the DiagnosticBuilder is still created in the same way, but when emitting it to the output we change how things are presented depending on wether we're in teach mode or not, by removing the left margin on notes, line wrapping, how we deal with suggestions, maybe even highlighting code based on this.

self.span,
self.expr_ty,
fcx.ty_to_string(self.cast_ty))
.diagnostic().emit();
}
CastError::UnknownCastPtrKind |
CastError::UnknownExprPtrKind => {
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ use rustc::ty::maps::Providers;
use rustc::ty::util::{Representability, IntTypeExt};
use rustc::ty::layout::LayoutOf;
use errors::{DiagnosticBuilder, DiagnosticId};

use require_c_abi_if_variadic;
use session::{CompileIncomplete, config, Session};
use TypeAndSubsts;
Expand Down Expand Up @@ -2591,9 +2592,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
// arguments which we skipped above.
if variadic {
fn variadic_error<'tcx>(s: &Session, span: Span, t: Ty<'tcx>, cast_ty: &str) {
type_error_struct!(s, span, t, E0617,
"can't pass `{}` to variadic function, cast to `{}`",
t, cast_ty).emit();
use structured_errors::{VariadicError, StructuredDiagnostic};
VariadicError::new(s, span, t, cast_ty).diagnostic().emit();
}

for arg in args.iter().skip(expected_arg_count) {
Expand Down
7 changes: 4 additions & 3 deletions src/librustc_typeck/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,17 @@ use std::iter;
// registered before they are used.
mod diagnostics;

mod astconv;
mod check;
mod check_unused;
mod astconv;
mod coherence;
mod collect;
mod constrained_type_params;
mod structured_errors;
mod impl_wf_check;
mod coherence;
mod namespace;
mod outlives;
mod variance;
mod namespace;

pub struct TypeAndSubsts<'tcx> {
substs: &'tcx Substs<'tcx>,
Expand Down
150 changes: 150 additions & 0 deletions src/librustc_typeck/structured_errors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
// 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.

use rustc::session::Session;
use syntax_pos::Span;
use errors::{DiagnosticId, DiagnosticBuilder};
use rustc::ty::{Ty, TypeFoldable};

pub trait StructuredDiagnostic<'tcx> {
fn session(&self) -> &Session;

fn code(&self) -> DiagnosticId;

fn common(&self) -> DiagnosticBuilder<'tcx>;

fn diagnostic(&self) -> DiagnosticBuilder<'tcx> {
let err = self.common();
if self.session().teach(&self.code()) {
self.extended(err)
} else {
self.regular(err)
}
}

fn regular(&self, err: DiagnosticBuilder<'tcx>) -> DiagnosticBuilder<'tcx> {
err
}

fn extended(&self, err: DiagnosticBuilder<'tcx>) -> DiagnosticBuilder<'tcx> {
err
}
}

pub struct VariadicError<'tcx> {
sess: &'tcx Session,
span: Span,
t: Ty<'tcx>,
cast_ty: &'tcx str,
}

impl<'tcx> VariadicError<'tcx> {
pub fn new(sess: &'tcx Session,
span: Span,
t: Ty<'tcx>,
cast_ty: &'tcx str) -> VariadicError<'tcx> {
VariadicError { sess, span, t, cast_ty }
}
}

impl<'tcx> StructuredDiagnostic<'tcx> for VariadicError<'tcx> {
fn session(&self) -> &Session { self.sess }

fn code(&self) -> DiagnosticId {
__diagnostic_used!(E0617);
DiagnosticId::Error("E0617".to_owned())
}

fn common(&self) -> DiagnosticBuilder<'tcx> {
let mut err = if self.t.references_error() {
self.sess.diagnostic().struct_dummy()
} else {
self.sess.struct_span_fatal_with_code(
self.span,
&format!("can't pass `{}` to variadic function", self.t),
self.code(),
)
};
if let Ok(snippet) = self.sess.codemap().span_to_snippet(self.span) {
err.span_suggestion(self.span,
&format!("cast the value to `{}`", self.cast_ty),
format!("{} as {}", snippet, self.cast_ty));
} else {
err.help(&format!("cast the value to `{}`", self.cast_ty));
}
err
}

fn extended(&self, mut err: DiagnosticBuilder<'tcx>) -> DiagnosticBuilder<'tcx> {
err.note(&format!("certain types, like `{}`, must be cast before passing them to a \
variadic function, because of arcane ABI rules dictated by the C \
standard",
self.t));
err
}
}

pub struct SizedUnsizedCastError<'tcx> {
sess: &'tcx Session,
span: Span,
expr_ty: Ty<'tcx>,
cast_ty: String,
}

impl<'tcx> SizedUnsizedCastError<'tcx> {
pub fn new(sess: &'tcx Session,
span: Span,
expr_ty: Ty<'tcx>,
cast_ty: String) -> SizedUnsizedCastError<'tcx> {
SizedUnsizedCastError { sess, span, expr_ty, cast_ty }
}
}

impl<'tcx> StructuredDiagnostic<'tcx> for SizedUnsizedCastError<'tcx> {
fn session(&self) -> &Session { self.sess }

fn code(&self) -> DiagnosticId {
__diagnostic_used!(E0607);
DiagnosticId::Error("E0607".to_owned())
}

fn common(&self) -> DiagnosticBuilder<'tcx> {
if self.expr_ty.references_error() {
self.sess.diagnostic().struct_dummy()
} else {
self.sess.struct_span_fatal_with_code(
self.span,
&format!("cannot cast thin pointer `{}` to fat pointer `{}`",
self.expr_ty,
self.cast_ty),
self.code(),
)
}
}

fn extended(&self, mut err: DiagnosticBuilder<'tcx>) -> DiagnosticBuilder<'tcx> {
err.help(
"Thin pointers are \"simple\" pointers: they are purely a reference to a
memory address.

Fat pointers are pointers referencing \"Dynamically Sized Types\" (also
called DST). DST don't have a statically known size, therefore they can
only exist behind some kind of pointers that contain additional
information. Slices and trait objects are DSTs. In the case of slices,
the additional information the fat pointer holds is their size.

To fix this error, don't try to cast directly between thin and fat
pointers.

For more information about casts, take a look at The Book:
https://doc.rust-lang.org/book/first-edition/casting-between-types.html");
err
}
}
18 changes: 12 additions & 6 deletions src/test/compile-fail/E0617.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,22 @@ extern {
fn main() {
unsafe {
printf(::std::ptr::null(), 0f32);
//~^ ERROR can't pass `f32` to variadic function, cast to `c_double` [E0617]
//~^ ERROR can't pass `f32` to variadic function
//~| HELP cast the value to `c_double`
printf(::std::ptr::null(), 0i8);
//~^ ERROR can't pass `i8` to variadic function, cast to `c_int` [E0617]
//~^ ERROR can't pass `i8` to variadic function
//~| HELP cast the value to `c_int`
printf(::std::ptr::null(), 0i16);
//~^ ERROR can't pass `i16` to variadic function, cast to `c_int` [E0617]
//~^ ERROR can't pass `i16` to variadic function
//~| HELP cast the value to `c_int`
printf(::std::ptr::null(), 0u8);
//~^ ERROR can't pass `u8` to variadic function, cast to `c_uint` [E0617]
//~^ ERROR can't pass `u8` to variadic function
//~| HELP cast the value to `c_uint`
printf(::std::ptr::null(), 0u16);
//~^ ERROR can't pass `u16` to variadic function, cast to `c_uint` [E0617]
//~^ ERROR can't pass `u16` to variadic function
//~| HELP cast the value to `c_uint`
printf(::std::ptr::null(), printf);
//~^ ERROR can't pass `unsafe extern "C" fn(*const i8, ...) {printf}` to variadic function, cast to `unsafe extern "C" fn(*const i8, ...)` [E0617]
//~^ ERROR can't pass `unsafe extern "C" fn(*const i8, ...) {printf}` to variadic function
//~| HELP cast the value to `unsafe extern "C" fn(*const i8, ...)`
}
}
3 changes: 2 additions & 1 deletion src/test/compile-fail/issue-32201.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ fn bar(_: *const u8) {}
fn main() {
unsafe {
foo(0, bar);
//~^ ERROR can't pass `fn(*const u8) {bar}` to variadic function, cast to `fn(*const u8)`
//~^ ERROR can't pass `fn(*const u8) {bar}` to variadic function
//~| HELP cast the value to `fn(*const u8)`
}
}
12 changes: 6 additions & 6 deletions src/test/ui/variadic-ffi-3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ fn main() {
//~| expected type `extern "C" fn(isize, u8, ...)`
//~| found type `extern "C" fn(isize, u8) {bar}`

foo(1, 2, 3f32); //~ ERROR can't pass `f32` to variadic function, cast to `c_double`
foo(1, 2, true); //~ ERROR can't pass `bool` to variadic function, cast to `c_int`
foo(1, 2, 1i8); //~ ERROR can't pass `i8` to variadic function, cast to `c_int`
foo(1, 2, 1u8); //~ ERROR can't pass `u8` to variadic function, cast to `c_uint`
foo(1, 2, 1i16); //~ ERROR can't pass `i16` to variadic function, cast to `c_int`
foo(1, 2, 1u16); //~ ERROR can't pass `u16` to variadic function, cast to `c_uint`
foo(1, 2, 3f32); //~ ERROR can't pass `f32` to variadic function
foo(1, 2, true); //~ ERROR can't pass `bool` to variadic function
foo(1, 2, 1i8); //~ ERROR can't pass `i8` to variadic function
foo(1, 2, 1u8); //~ ERROR can't pass `u8` to variadic function
foo(1, 2, 1i16); //~ ERROR can't pass `i16` to variadic function
foo(1, 2, 1u16); //~ ERROR can't pass `u16` to variadic function
}
}
Loading