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

Remap instrument-coverage line numbers in doctests #79762

Merged
merged 3 commits into from Dec 25, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion compiler/rustc_mir/src/transform/coverage/mod.rs
Expand Up @@ -30,6 +30,7 @@ use rustc_middle::mir::{
};
use rustc_middle::ty::TyCtxt;
use rustc_span::def_id::DefId;
use rustc_span::source_map::SourceMap;
use rustc_span::{CharPos, Pos, SourceFile, Span, Symbol};

/// A simple error message wrapper for `coverage::Error`s.
Expand Down Expand Up @@ -311,7 +312,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
self.mir_body,
counter_kind,
self.bcb_leader_bb(bcb),
Some(make_code_region(file_name, &self.source_file, span, body_span)),
Some(make_code_region(source_map, file_name, &self.source_file, span, body_span)),
);
}
}
Expand Down Expand Up @@ -489,6 +490,7 @@ fn inject_intermediate_expression(mir_body: &mut mir::Body<'tcx>, expression: Co

/// Convert the Span into its file name, start line and column, and end line and column
fn make_code_region(
source_map: &SourceMap,
file_name: Symbol,
source_file: &Lrc<SourceFile>,
span: Span,
Expand All @@ -508,6 +510,8 @@ fn make_code_region(
} else {
source_file.lookup_file_pos(span.hi())
};
let start_line = source_map.doctest_offset_line(&source_file.name, start_line);
Copy link
Contributor

Choose a reason for hiding this comment

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

These look like the only pertinent changes in this file. Let me know if I'm missing something.

Very exciting if this works.

let end_line = source_map.doctest_offset_line(&source_file.name, end_line);
CodeRegion {
file_name,
start_line: start_line as u32,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_span/src/lib.rs
Expand Up @@ -182,7 +182,7 @@ impl std::fmt::Display for FileName {
use FileName::*;
match *self {
Real(RealFileName::Named(ref path)) => write!(fmt, "{}", path.display()),
// FIXME: might be nice to display both compoments of Devirtualized.
// FIXME: might be nice to display both components of Devirtualized.
// But for now (to backport fix for issue #70924), best to not
// perturb diagnostics so its obvious test suite still works.
Real(RealFileName::Devirtualized { ref local_path, virtual_name: _ }) => {
Expand Down
76 changes: 50 additions & 26 deletions src/librustdoc/doctest.rs
Expand Up @@ -247,9 +247,10 @@ fn run_test(
edition: Edition,
outdir: DirState,
path: PathBuf,
test_id: &str,
) -> Result<(), TestFailure> {
let (test, line_offset, supports_color) =
make_test(test, Some(cratename), as_test_harness, opts, edition);
make_test(test, Some(cratename), as_test_harness, opts, edition, Some(test_id));

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

Expand Down Expand Up @@ -387,6 +388,7 @@ crate fn make_test(
dont_insert_main: bool,
opts: &TestOptions,
edition: Edition,
test_id: Option<&str>,
) -> (String, usize, bool) {
let (crate_attrs, everything_else, crates) = partition_source(s);
let everything_else = everything_else.trim();
Expand Down Expand Up @@ -542,16 +544,41 @@ crate fn make_test(
prog.push_str(everything_else);
} else {
let returns_result = everything_else.trim_end().ends_with("(())");
// Give each doctest main function a unique name.
// This is for example needed for the tooling around `-Z instrument-coverage`.
let inner_fn_name = if let Some(test_id) = test_id {
format!("_doctest_main_{}", test_id)
Swatinem marked this conversation as resolved.
Show resolved Hide resolved
} else {
"_inner".into()
};
let inner_attr = if test_id.is_some() { "#[allow(non_snake_case)] " } else { "" };
let (main_pre, main_post) = if returns_result {
(
"fn main() { fn _inner() -> Result<(), impl core::fmt::Debug> {",
"}\n_inner().unwrap() }",
format!(
"fn main() {{ {}fn {}() -> Result<(), impl core::fmt::Debug> {{\n",
inner_attr, inner_fn_name
),
format!("\n}}; {}().unwrap() }}", inner_fn_name),
)
} else if test_id.is_some() {
(
format!("fn main() {{ {}fn {}() {{\n", inner_attr, inner_fn_name),
format!("\n}}; {}() }}", inner_fn_name),
)
} else {
("fn main() {\n", "\n}")
("fn main() {\n".into(), "\n}".into())
};
prog.extend([main_pre, everything_else, main_post].iter().cloned());
// Note on newlines: We insert a line/newline *before*, and *after*
// the doctest and adjust the `line_offset` accordingly.
// In the case of `-Z instrument-coverage`, this means that the generated
// inner `main` function spans from the doctest opening codeblock to the
// closing one. For example
// /// ``` <- start of the inner main
// /// <- code under doctest
// /// ``` <- end of the inner main
line_offset += 1;

prog.extend([&main_pre, everything_else, &main_post].iter().cloned());
}

debug!("final doctest:\n{}", prog);
Expand Down Expand Up @@ -749,28 +776,24 @@ impl Tester for Collector {
_ => PathBuf::from(r"doctest.rs"),
};

// For example `module/file.rs` would become `module_file_rs`
let file = filename
.to_string()
.chars()
.map(|c| if c.is_ascii_alphanumeric() { c } else { '_' })
.collect::<String>();
let test_id = format!(
"{file}_{line}_{number}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This basically reverts half of #79413 since rust-lang/cargo#8954 is a proper solution to this. CC @jyn514

Copy link
Member

@jyn514 jyn514 Dec 11, 2020

Choose a reason for hiding this comment

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

Sure, seems reasonable. I always want to do things through cargo instead of rustdoc where possible because cargo has more information available.

file = file,
line = line,
number = {
// Increases the current test number, if this file already
// exists or it creates a new entry with a test number of 0.
self.visited_tests.entry((file.clone(), line)).and_modify(|v| *v += 1).or_insert(0)
},
);
let outdir = if let Some(mut path) = options.persist_doctests.clone() {
// For example `module/file.rs` would become `module_file_rs`
let folder_name = filename
.to_string()
.chars()
.map(|c| if c == '\\' || c == '/' || c == '.' { '_' } else { c })
Swatinem marked this conversation as resolved.
Show resolved Hide resolved
.collect::<String>();

path.push(format!(
"{krate}_{file}_{line}_{number}",
krate = cratename,
file = folder_name,
line = line,
number = {
// Increases the current test number, if this file already
// exists or it creates a new entry with a test number of 0.
self.visited_tests
.entry((folder_name.clone(), line))
.and_modify(|v| *v += 1)
.or_insert(0)
},
));
path.push(&test_id);

std::fs::create_dir_all(&path)
.expect("Couldn't create directory for doctest executables");
Expand Down Expand Up @@ -817,6 +840,7 @@ impl Tester for Collector {
edition,
outdir,
path,
&test_id,
);

if let Err(err) = res {
Expand Down
69 changes: 52 additions & 17 deletions src/librustdoc/doctest/tests.rs
Expand Up @@ -11,7 +11,7 @@ fn main() {
assert_eq!(2+2, 4);
}"
.to_string();
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION);
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None);
assert_eq!((output, len), (expected, 2));
}

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

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

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

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

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

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

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

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

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

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

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

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

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

Expand All @@ -242,7 +242,7 @@ assert_eq!(2+2, 4);
}"
.to_string();

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

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

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

Expand All @@ -274,6 +274,41 @@ test_wrapper! {
}"
.to_string();

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

#[test]
fn make_test_returns_result() {
// creates an inner function and unwraps it
let opts = TestOptions::default();
let input = "use std::io;
let mut input = String::new();
io::stdin().read_line(&mut input)?;
Ok::<(), io:Error>(())";
let expected = "#![allow(unused)]
fn main() { fn _inner() -> Result<(), impl core::fmt::Debug> {
use std::io;
let mut input = String::new();
io::stdin().read_line(&mut input)?;
Ok::<(), io:Error>(())
}; _inner().unwrap() }"
.to_string();
let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None);
assert_eq!((output, len), (expected, 2));
}

#[test]
fn make_test_named_wrapper() {
// creates an inner function with a specific name
let opts = TestOptions::default();
let input = "assert_eq!(2+2, 4);";
let expected = "#![allow(unused)]
fn main() { #[allow(non_snake_case)] fn _doctest_main__some_unique_name() {
assert_eq!(2+2, 4);
}; _doctest_main__some_unique_name() }"
.to_string();
let (output, len, _) =
make_test(input, None, false, &opts, DEFAULT_EDITION, Some("_some_unique_name"));
assert_eq!((output, len), (expected, 2));
}
2 changes: 1 addition & 1 deletion src/librustdoc/html/markdown.rs
Expand Up @@ -248,7 +248,7 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for CodeBlocks<'_, 'a, I> {
.join("\n");
let krate = krate.as_ref().map(|s| &**s);
let (test, _, _) =
doctest::make_test(&test, krate, false, &Default::default(), edition);
doctest::make_test(&test, krate, false, &Default::default(), edition, None);
let channel = if test.contains("#![feature(") { "&amp;version=nightly" } else { "" };

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