Skip to content

Add new diagnostic writer using annotate-snippet library #61407

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

Merged
merged 5 commits into from
Jun 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2789,6 +2789,7 @@ dependencies = [
name = "rustc_errors"
version = "0.0.0"
dependencies = [
"annotate-snippets 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)",
"atty 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)",
"log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
"rustc_cratesio_shim 0.0.0",
Expand Down
10 changes: 10 additions & 0 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2002,6 +2002,9 @@ pub fn build_session_options_and_crate_config(
match matches.opt_str("error-format").as_ref().map(|s| &s[..]) {
None |
Some("human") => ErrorOutputType::HumanReadable(HumanReadableErrorType::Default(color)),
Some("human-annotate-rs") => {
ErrorOutputType::HumanReadable(HumanReadableErrorType::AnnotateRs(color))
},
Some("json") => ErrorOutputType::Json { pretty: false, json_rendered },
Some("pretty-json") => ErrorOutputType::Json { pretty: true, json_rendered },
Some("short") => ErrorOutputType::HumanReadable(HumanReadableErrorType::Short(color)),
Expand Down Expand Up @@ -2038,6 +2041,13 @@ pub fn build_session_options_and_crate_config(
"--error-format=pretty-json is unstable",
);
}
if let ErrorOutputType::HumanReadable(HumanReadableErrorType::AnnotateRs(_)) =
error_format {
early_error(
ErrorOutputType::Json { pretty: false, json_rendered },
"--error-format=human-annotate-rs is unstable",
);
}
}

if debugging_opts.pgo_gen.enabled() && debugging_opts.pgo_use.is_some() {
Expand Down
39 changes: 25 additions & 14 deletions src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use rustc_data_structures::sync::{

use errors::{DiagnosticBuilder, DiagnosticId, Applicability};
use errors::emitter::{Emitter, EmitterWriter};
use errors::emitter::HumanReadableErrorType;
use errors::annotate_rs_emitter::{AnnotateRsEmitterWriter};
use syntax::ast::{self, NodeId};
use syntax::edition::Edition;
use syntax::feature_gate::{self, AttributeType};
Expand Down Expand Up @@ -1031,22 +1033,31 @@ fn default_emitter(
match (sopts.error_format, emitter_dest) {
(config::ErrorOutputType::HumanReadable(kind), dst) => {
let (short, color_config) = kind.unzip();
let emitter = match dst {
None => EmitterWriter::stderr(
color_config,
Some(source_map.clone()),
short,
sopts.debugging_opts.teach,
),
Some(dst) => EmitterWriter::new(
dst,

if let HumanReadableErrorType::AnnotateRs(_) = kind {
let emitter = AnnotateRsEmitterWriter::new(
Some(source_map.clone()),
short,
false, // no teach messages when writing to a buffer
false, // no colors when writing to a buffer
),
};
Box::new(emitter.ui_testing(sopts.debugging_opts.ui_testing))
);
Box::new(emitter.ui_testing(sopts.debugging_opts.ui_testing))
} else {
let emitter = match dst {
None => EmitterWriter::stderr(
color_config,
Some(source_map.clone()),
short,
sopts.debugging_opts.teach,
),
Some(dst) => EmitterWriter::new(
dst,
Some(source_map.clone()),
short,
false, // no teach messages when writing to a buffer
false, // no colors when writing to a buffer
),
};
Box::new(emitter.ui_testing(sopts.debugging_opts.ui_testing))
}
},
(config::ErrorOutputType::Json { pretty, json_rendered }, None) => Box::new(
JsonEmitter::stderr(
Expand Down
1 change: 1 addition & 0 deletions src/librustc_errors/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ rustc_cratesio_shim = { path = "../librustc_cratesio_shim" }
unicode-width = "0.1.4"
atty = "0.2"
termcolor = "1.0"
annotate-snippets = "0.5.0"
212 changes: 212 additions & 0 deletions src/librustc_errors/annotate_rs_emitter.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
/// Emit diagnostics using the `annotate-snippets` library
///
/// This is the equivalent of `./emitter.rs` but making use of the
/// [`annotate-snippets`][annotate_snippets] library instead of building the output ourselves.
///
/// [annotate_snippets]: https://docs.rs/crate/annotate-snippets/

use syntax_pos::{SourceFile, MultiSpan, Loc};
use crate::{
Level, CodeSuggestion, DiagnosticBuilder, Emitter,
SourceMapperDyn, SubDiagnostic, DiagnosticId
};
use crate::emitter::FileWithAnnotatedLines;
use rustc_data_structures::sync::Lrc;
use crate::snippet::Line;
use annotate_snippets::snippet::*;
use annotate_snippets::display_list::DisplayList;
use annotate_snippets::formatter::DisplayListFormatter;


/// Generates diagnostics using annotate-rs
pub struct AnnotateRsEmitterWriter {
source_map: Option<Lrc<SourceMapperDyn>>,
/// If true, hides the longer explanation text
short_message: bool,
/// If true, will normalize line numbers with LL to prevent noise in UI test diffs.
ui_testing: bool,
}

impl Emitter for AnnotateRsEmitterWriter {
/// The entry point for the diagnostics generation
fn emit_diagnostic(&mut self, db: &DiagnosticBuilder<'_>) {
let primary_span = db.span.clone();
let children = db.children.clone();
// FIXME(#59346): Collect suggestions (see emitter.rs)
let suggestions: &[_] = &[];

// FIXME(#59346): Add `fix_multispans_in_std_macros` function from emitter.rs

self.emit_messages_default(&db.level,
db.message(),
&db.code,
&primary_span,
&children,
&suggestions);
}

fn should_show_explain(&self) -> bool {
!self.short_message
}
}

/// Collects all the data needed to generate the data structures needed for the
/// `annotate-snippets` library.
struct DiagnosticConverter<'a> {
source_map: Option<Lrc<SourceMapperDyn>>,
level: Level,
message: String,
code: Option<DiagnosticId>,
msp: MultiSpan,
#[allow(dead_code)]
children: &'a [SubDiagnostic],
#[allow(dead_code)]
suggestions: &'a [CodeSuggestion]
}

impl<'a> DiagnosticConverter<'a> {
/// Turns rustc Diagnostic information into a `annotate_snippets::snippet::Snippet`.
fn to_annotation_snippet(&self) -> Option<Snippet> {
if let Some(source_map) = &self.source_map {
// Make sure our primary file comes first
let primary_lo = if let Some(ref primary_span) =
self.msp.primary_span().as_ref() {
source_map.lookup_char_pos(primary_span.lo())
} else {
// FIXME(#59346): Not sure when this is the case and what
// should be done if it happens
return None
};
let annotated_files = FileWithAnnotatedLines::collect_annotations(
&self.msp,
&self.source_map
);
let slices = self.slices_for_files(annotated_files, primary_lo);

Some(Snippet {
title: Some(Annotation {
label: Some(self.message.to_string()),
id: self.code.clone().map(|c| {
match c {
DiagnosticId::Error(val) | DiagnosticId::Lint(val) => val
}
}),
annotation_type: Self::annotation_type_for_level(self.level),
}),
footer: vec![],
slices: slices,
})
} else {
// FIXME(#59346): Is it ok to return None if there's no source_map?
None
}
}

fn slices_for_files(
&self,
annotated_files: Vec<FileWithAnnotatedLines>,
primary_lo: Loc
) -> Vec<Slice> {
// FIXME(#59346): Provide a test case where `annotated_files` is > 1
annotated_files.iter().flat_map(|annotated_file| {
annotated_file.lines.iter().map(|line| {
let line_source = Self::source_string(annotated_file.file.clone(), &line);
Slice {
source: line_source,
line_start: line.line_index,
origin: Some(primary_lo.file.name.to_string()),
// FIXME(#59346): Not really sure when `fold` should be true or false
fold: false,
annotations: line.annotations.iter().map(|a| {
self.annotation_to_source_annotation(a.clone())
}).collect(),
}
}).collect::<Vec<Slice>>()
}).collect::<Vec<Slice>>()
}

/// Turns a `crate::snippet::Annotation` into a `SourceAnnotation`
fn annotation_to_source_annotation(
&self,
annotation: crate::snippet::Annotation
) -> SourceAnnotation {
SourceAnnotation {
range: (annotation.start_col, annotation.end_col),
label: annotation.label.unwrap_or("".to_string()),
annotation_type: Self::annotation_type_for_level(self.level)
}
}

/// Provides the source string for the given `line` of `file`
fn source_string(
file: Lrc<SourceFile>,
line: &Line
) -> String {
file.get_line(line.line_index - 1).map(|a| a.to_string()).unwrap_or(String::new())
}

/// Maps `Diagnostic::Level` to `snippet::AnnotationType`
fn annotation_type_for_level(level: Level) -> AnnotationType {
match level {
Level::Bug | Level::Fatal | Level::PhaseFatal | Level::Error => AnnotationType::Error,
Level::Warning => AnnotationType::Warning,
Level::Note => AnnotationType::Note,
Level::Help => AnnotationType::Help,
// FIXME(#59346): Not sure how to map these two levels
Level::Cancelled | Level::FailureNote => AnnotationType::Error
}
}
}

impl AnnotateRsEmitterWriter {
pub fn new(
source_map: Option<Lrc<SourceMapperDyn>>,
short_message: bool
) -> Self {
Self {
source_map,
short_message,
ui_testing: false,
}
}

/// Allows to modify `Self` to enable or disable the `ui_testing` flag.
///
/// If this is set to true, line numbers will be normalized as `LL` in the output.
// FIXME(#59346): This method is used via the public interface, but setting the `ui_testing`
// flag currently does not anonymize line numbers. We would have to add the `maybe_anonymized`
// method from `emitter.rs` and implement rust-lang/annotate-snippets-rs#2 in order to
// anonymize line numbers.
pub fn ui_testing(mut self, ui_testing: bool) -> Self {
self.ui_testing = ui_testing;
self
}

fn emit_messages_default(
&mut self,
level: &Level,
message: String,
code: &Option<DiagnosticId>,
msp: &MultiSpan,
children: &[SubDiagnostic],
suggestions: &[CodeSuggestion]
) {
let converter = DiagnosticConverter {
source_map: self.source_map.clone(),
level: level.clone(),
message: message.clone(),
code: code.clone(),
msp: msp.clone(),
children,
suggestions
};
if let Some(snippet) = converter.to_annotation_snippet() {
let dl = DisplayList::from(snippet);
let dlf = DisplayListFormatter::new(true);
// FIXME(#59346): Figure out if we can _always_ print to stderr or not.
// `emitter.rs` has the `Destination` enum that lists various possible output
// destinations.
eprintln!("{}", dlf.format(&dl));
};
}
}
6 changes: 4 additions & 2 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use termcolor::{WriteColor, Color, Buffer};
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum HumanReadableErrorType {
Default(ColorConfig),
AnnotateRs(ColorConfig),
Short(ColorConfig),
}

Expand All @@ -33,6 +34,7 @@ impl HumanReadableErrorType {
match self {
HumanReadableErrorType::Default(cc) => (false, cc),
HumanReadableErrorType::Short(cc) => (true, cc),
HumanReadableErrorType::AnnotateRs(cc) => (false, cc),
}
}
pub fn new_emitter(
Expand Down Expand Up @@ -173,8 +175,8 @@ pub struct EmitterWriter {

#[derive(Debug)]
pub struct FileWithAnnotatedLines {
file: Lrc<SourceFile>,
lines: Vec<Line>,
pub file: Lrc<SourceFile>,
pub lines: Vec<Line>,
multiline_depth: usize,
}

Expand Down
1 change: 1 addition & 0 deletions src/librustc_errors/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use termcolor::{ColorSpec, Color};
mod diagnostic;
mod diagnostic_builder;
pub mod emitter;
pub mod annotate_rs_emitter;
mod snippet;
pub mod registry;
mod styled_buffer;
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/annotate-snippet/missing-type.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// compile-flags: --error-format human-annotate-rs

pub fn main() {
let x: Iter; //~ ERROR cannot find type `Iter` in this scope
}
6 changes: 6 additions & 0 deletions src/test/ui/annotate-snippet/missing-type.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
error[E0412]: cannot find type `Iter` in this scope
--> $DIR/missing-type.rs:4:11
|
4 | let x: Iter;
| ^^^^ not found in this scope
|
2 changes: 2 additions & 0 deletions src/tools/tidy/src/deps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ const WHITELIST_CRATES: &[CrateVersion<'_>] = &[
const WHITELIST: &[Crate<'_>] = &[
Crate("adler32"),
Crate("aho-corasick"),
Crate("annotate-snippets"),
Crate("ansi_term"),
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @rust-lang/compiler we're pulling in a new dependency here, and we have no control over it: ansi_term (annotate-snippets is also a new dep, but it is under the rust-lang org: https://github.com/rust-lang/annotate-snippets-rs).

Copy link
Member

Choose a reason for hiding this comment

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

Two points in favour of allowing it: its license (MIT) is in our list of allowed licenses for external crates; and it is already a transitive dependency of our other core projects such as cargo.

As long as changes to the ansi_term are vetted as part of the annotate-snippets crate and the crate is locked to a specific patch version I’m in favour of the dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

The usage of ansi_term in annotate-snippets seems pretty lightweight so far and we could probably switch to another one as well, if that's a concern.

Crate("arrayvec"),
Crate("atty"),
Crate("autocfg"),
Expand Down