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.

Sign up for GitHub

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

Rustdoc test compiler output color #74293

Merged
merged 9 commits into from
Nov 17, 2020
Merged

Rustdoc test compiler output color #74293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
96a6a5f
Add color in rustdoc --test output
GuillaumeGomez Jul 13, 2020
54e8216
Update error code detection in compile_fail doctests
GuillaumeGomez Jul 14, 2020
57bab5e
Add check to get windows console type to decide to use colors or not
GuillaumeGomez Jul 16, 2020
704001b
Update lock file
GuillaumeGomez Jul 16, 2020
32d64ed
Simplfy color availability check
GuillaumeGomez Nov 11, 2020
fd4a33c
Update doctest tests
GuillaumeGomez Nov 11, 2020
95ee1fc
Correctly detect color support
GuillaumeGomez Nov 12, 2020
63785c8
Add comment explaining why we can't split on `error[{}]: ` because of…
GuillaumeGomez Nov 13, 2020
ec10824
Remove unused import
GuillaumeGomez Nov 15, 2020
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
17 changes: 17 additions & 0 deletions compiler/rustc_errors/src/emitter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,11 @@ pub trait Emitter {
true
}

/// Checks if we can use colors in the current output stream.
fn supports_color(&self) -> bool {
false
}

fn source_map(&self) -> Option<&Lrc<SourceMap>>;

/// Formats the substitutions of the primary_span
Expand Down Expand Up @@ -504,6 +509,10 @@ impl Emitter for EmitterWriter {
fn should_show_explain(&self) -> bool {
!self.short_message
}

fn supports_color(&self) -> bool {
self.dst.supports_color()
}
}

/// An emitter that does nothing when emitting a diagnostic.
Expand Down Expand Up @@ -2057,6 +2066,14 @@ impl Destination {
Destination::Raw(ref mut t, true) => WritableDst::ColoredRaw(Ansi::new(t)),
}
}

fn supports_color(&self) -> bool {
match *self {
Self::Terminal(ref stream) => stream.supports_color(),
Self::Buffered(ref buffer) => buffer.buffer().supports_color(),
Self::Raw(_, supports_color) => supports_color,
}
}
}

impl<'a> WritableDst<'a> {
Expand Down
42 changes: 33 additions & 9 deletions src/librustdoc/doctest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use rustc_ast as ast;
use rustc_data_structures::sync::Lrc;
use rustc_errors::ErrorReported;
use rustc_errors::{ColorConfig, ErrorReported};
use rustc_hir as hir;
use rustc_hir::intravisit;
use rustc_hir::{HirId, CRATE_HIR_ID};
use rustc_interface::interface;
use rustc_middle::hir::map::Map;
use rustc_middle::ty::TyCtxt;
use rustc_session::config::{self, CrateType};
use rustc_session::config::{self, CrateType, ErrorOutputType};
use rustc_session::{lint, DiagnosticOutput, Session};
use rustc_span::edition::Edition;
use rustc_span::source_map::SourceMap;
Expand Down Expand Up @@ -248,7 +248,8 @@ fn run_test(
outdir: DirState,
path: PathBuf,
) -> Result<(), TestFailure> {
let (test, line_offset) = make_test(test, Some(cratename), as_test_harness, opts, edition);
let (test, line_offset, supports_color) =
make_test(test, Some(cratename), as_test_harness, opts, edition);

let output_file = outdir.path().join("rust_out");

Expand Down Expand Up @@ -293,6 +294,20 @@ fn run_test(
path.to_str().expect("target path must be valid unicode").to_string()
}
});
if let ErrorOutputType::HumanReadable(kind) = options.error_format {
let (_, color_config) = kind.unzip();
match color_config {
ColorConfig::Never => {
compiler.arg("--color").arg("never");
}
ColorConfig::Always => {
compiler.arg("--color").arg("always");
}
ColorConfig::Auto => {
compiler.arg("--color").arg(if supports_color { "always" } else { "never" });
}
}
}

compiler.arg("-");
compiler.stdin(Stdio::piped());
Expand Down Expand Up @@ -320,7 +335,10 @@ fn run_test(
(true, false) => {}
(false, true) => {
if !error_codes.is_empty() {
error_codes.retain(|err| !out.contains(&format!("error[{}]: ", err)));
// We used to check if the output contained "error[{}]: " but since we added the
// colored output, we can't anymore because of the color escape characters before
// the ":".
error_codes.retain(|err| !out.contains(&format!("error[{}]", err)));
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's because of the added terminal color codes. But it's been so long since I wrote this code that I can't remember for sure...

Copy link
Member

Choose a reason for hiding this comment

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

Can you double check why this breaks with : added, then add that as a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Member Author

Choose a reason for hiding this comment

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

So with the following file:

/// You are supposed to call it like this
///
/// # Example
///
/// ```compile_fail,E0599
/// let x = 12i32;
/// x.divo();
/// ```
pub struct Example {}

When I run it:

$ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustdoc --test foo.rs | cat -e
$
running 1 test$
test foo.rs - Example (line 5) ... FAILED$
$
failures:$
$
---- foo.rs - Example (line 5) stdout ----$
^[[0m^[[1m^[[38;5;9merror[E0599]^[[0m^[[0m^[[1m: no method named `divo` found for type `i32` in the current scope^[[0m$
^[[0m ^[[0m^[[0m^[[1m^[[38;5;12m--> ^[[0m^[[0mfoo.rs:7:3^[[0m$
^[[0m  ^[[0m^[[0m^[[1m^[[38;5;12m|^[[0m$
^[[0m^[[1m^[[38;5;12m4^[[0m^[[0m ^[[0m^[[0m^[[1m^[[38;5;12m| ^[[0m^[[0mx.divo();^[[0m$
^[[0m  ^[[0m^[[0m^[[1m^[[38;5;12m| ^[[0m^[[0m  ^[[0m^[[0m^[[1m^[[38;5;9m^^^^^[[0m^[[0m ^[[0m^[[0m^[[1m^[[38;5;9mmethod not found in `i32`^[[0m$
$
^[[0m^[[1m^[[38;5;9merror^[[0m^[[0m^[[1m: aborting due to previous error^[[0m$
$
^[[0m^[[1mFor more information about this error, try `rustc --explain E0599`.^[[0m$
Some expected error codes were not found: ["E0599"]$
$
failures:$
    foo.rs - Example (line 5)$
$
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out$
$

It doesn't find the error code because of the terminal color code characters. (termcap, my old nemesis)

So no, we need to remove the ": " part. :)

Copy link
Member

Choose a reason for hiding this comment

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

Ok great, please say that as a comment.

Suggested change
error_codes.retain(|err| !out.contains(&format!("error[{}]", err)));
// only look for `error[...]` since there may be color escape characters before and after
error_codes.retain(|err| !out.contains(&format!("error[{}]", err)));


if !error_codes.is_empty() {
return Err(TestFailure::MissingErrorCodes(error_codes));
Expand Down Expand Up @@ -362,18 +380,19 @@ fn run_test(
}

/// Transforms a test into code that can be compiled into a Rust binary, and returns the number of
/// lines before the test code begins.
/// lines before the test code begins as well as if the output stream supports colors or not.
crate fn make_test(
s: &str,
cratename: Option<&str>,
dont_insert_main: bool,
opts: &TestOptions,
edition: Edition,
) -> (String, usize) {
) -> (String, usize, bool) {
let (crate_attrs, everything_else, crates) = partition_source(s);
let everything_else = everything_else.trim();
let mut line_offset = 0;
let mut prog = String::new();
let mut supports_color = false;

if opts.attrs.is_empty() && !opts.display_warnings {
// If there aren't any attributes supplied by #![doc(test(attr(...)))], then allow some
Expand All @@ -399,7 +418,7 @@ crate fn make_test(
// crate already is included.
let result = rustc_driver::catch_fatal_errors(|| {
rustc_span::with_session_globals(edition, || {
use rustc_errors::emitter::EmitterWriter;
use rustc_errors::emitter::{Emitter, EmitterWriter};
use rustc_errors::Handler;
use rustc_parse::maybe_new_parser_from_source_str;
use rustc_session::parse::ParseSess;
Expand All @@ -411,8 +430,13 @@ crate fn make_test(
// Any errors in parsing should also appear when the doctest is compiled for real, so just
// send all the errors that librustc_ast emits directly into a `Sink` instead of stderr.
let sm = Lrc::new(SourceMap::new(FilePathMapping::empty()));
supports_color =
EmitterWriter::stderr(ColorConfig::Auto, None, false, false, Some(80), false)
.supports_color();
Copy link
Member

Choose a reason for hiding this comment

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

Whether we support color does not depend on the test so this isn't the correct place to check this.

It's also not the correct way to check it. First of all the output for tests goes to stdout not stderr. Also the output is handled by the test crate which doesn't use an Emitter.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't really matter: EmitterWriter provides a method which allows to detect if the output stream support colors. So even if the test crate doesn't use an Emitter, it still relies on termcolor and we need to tell it if we want colors or not.

As for stderr/stdout, I didn't see a case yet where they were handled differently. But if you have one, I'm very curious about it (always interesting to discover new corner cases).


let emitter =
EmitterWriter::new(box io::sink(), None, false, false, false, None, false);

// FIXME(misdreavus): pass `-Z treat-err-as-bug` to the doctest parser
let handler = Handler::with_emitter(false, None, box emitter);
let sess = ParseSess::with_span_handler(handler, sm);
Expand Down Expand Up @@ -482,7 +506,7 @@ crate fn make_test(
Err(ErrorReported) => {
// If the parser panicked due to a fatal error, pass the test code through unchanged.
// The error will be reported during compilation.
return (s.to_owned(), 0);
return (s.to_owned(), 0, false);
}
};

Expand Down Expand Up @@ -532,7 +556,7 @@ crate fn make_test(

debug!("final doctest:\n{}", prog);

(prog, line_offset)
(prog, line_offset, supports_color)
}

// FIXME(aburka): use a real parser to deal with multiline attributes
Expand Down
68 changes: 34 additions & 34 deletions src/librustdoc/doctest/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ fn main() {
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -26,8 +26,8 @@ fn main() {
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -44,8 +44,8 @@ use asdf::qwop;
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 3));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 3));
}

#[test]
Expand All @@ -61,8 +61,8 @@ use asdf::qwop;
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -79,8 +79,8 @@ use std::*;
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("std"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, Some("std"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -98,8 +98,8 @@ use asdf::qwop;
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -115,8 +115,8 @@ use asdf::qwop;
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -134,8 +134,8 @@ use asdf::qwop;
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 3));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 3));

// Adding more will also bump the returned line offset.
opts.attrs.push("feature(hella_dope)".to_string());
Expand All @@ -147,8 +147,8 @@ use asdf::qwop;
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 4));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 4));
}

#[test]
Expand All @@ -164,8 +164,8 @@ fn main() {
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -180,8 +180,8 @@ fn main() {
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 1));
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 1));
}

#[test]
Expand All @@ -196,8 +196,8 @@ fn main() {
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));
}

#[test]
Expand All @@ -210,8 +210,8 @@ assert_eq!(2+2, 4);";
//Ceci n'est pas une `fn main`
assert_eq!(2+2, 4);"
.to_string();
let output = make_test(input, None, true, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 1));
let (output, len, _) = make_test(input, None, true, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 1));
}

#[test]
Expand All @@ -224,8 +224,8 @@ fn make_test_display_warnings() {
assert_eq!(2+2, 4);
}"
.to_string();
let output = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 1));
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 1));
}

#[test]
Expand All @@ -242,8 +242,8 @@ assert_eq!(2+2, 4);
}"
.to_string();

let output = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 2));
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 2));

let input = "extern crate hella_qwop;
assert_eq!(asdf::foo, 4);";
Expand All @@ -256,8 +256,8 @@ assert_eq!(asdf::foo, 4);
}"
.to_string();

let output = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 3));
let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 3));
}

#[test]
Expand All @@ -274,6 +274,6 @@ test_wrapper! {
}"
.to_string();

let output = make_test(input, Some("my_crate"), false, &opts, DEFAULT_EDITION);
assert_eq!(output, (expected, 1));
let (output, len, _) = make_test(input, Some("my_crate"), false, &opts, DEFAULT_EDITION);
assert_eq!((output, len), (expected, 1));
}
3 changes: 2 additions & 1 deletion src/librustdoc/html/markdown.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,8 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for CodeBlocks<'_, 'a, I> {
.collect::<Vec<Cow<'_, str>>>()
.join("\n");
let krate = krate.as_ref().map(|s| &**s);
let (test, _) = doctest::make_test(&test, krate, false, &Default::default(), edition);
let (test, _, _) =
doctest::make_test(&test, krate, false, &Default::default(), edition);
let channel = if test.contains("#![feature(") { "&amp;version=nightly" } else { "" };

let edition_string = format!("&amp;edition={}", edition);
Expand Down