Skip to content

[rustdoc] Give more information into extracted doctest information #141399

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
4 changes: 2 additions & 2 deletions src/librustdoc/doctest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1044,14 +1044,14 @@ fn doctest_run_fn(
let report_unused_externs = |uext| {
unused_externs.lock().unwrap().push(uext);
};
let (full_test_code, full_test_line_offset) = doctest.generate_unique_doctest(
let (wrapper, full_test_line_offset) = doctest.generate_unique_doctest(
&scraped_test.text,
scraped_test.langstr.test_harness,
&global_opts,
Some(&global_opts.crate_name),
);
let runnable_test = RunnableDocTest {
full_test_code,
full_test_code: wrapper.to_string(),
Comment on lines +1047 to +1054
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/wrapper/wrapped/, same reasoning as before

full_test_line_offset,
test_opts,
global_opts,
Expand Down
34 changes: 30 additions & 4 deletions src/librustdoc/doctest/extracted.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use serde::Serialize;

use super::make::DocTestWrapResult;
use super::{BuildDocTestBuilder, ScrapedDocTest};
use crate::config::Options as RustdocOptions;
use crate::html::markdown;
Expand All @@ -14,7 +15,7 @@ use crate::html::markdown;
/// This integer is incremented with every breaking change to the API,
/// and is returned along with the JSON blob into the `format_version` root field.
/// Consuming code should assert that this value matches the format version(s) that it supports.
const FORMAT_VERSION: u32 = 1;
const FORMAT_VERSION: u32 = 2;

#[derive(Serialize)]
pub(crate) struct ExtractedDocTests {
Expand Down Expand Up @@ -42,7 +43,7 @@ impl ExtractedDocTests {
.edition(edition)
.lang_str(&langstr)
.build(None);
let (full_test_code, size) = doctest.generate_unique_doctest(
let (wrapper, _size) = doctest.generate_unique_doctest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (wrapper, _size) = doctest.generate_unique_doctest(
let (wrapped, _size) = doctest.generate_unique_doctest(

&text,
langstr.test_harness,
opts,
Expand All @@ -52,21 +53,46 @@ impl ExtractedDocTests {
file: filename.prefer_remapped_unconditionaly().to_string(),
line,
doctest_attributes: langstr.into(),
doctest_code: if size != 0 { Some(full_test_code) } else { None },
doctest_code: match wrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
doctest_code: match wrapper {
doctest_code: match wrapped {

DocTestWrapResult::Valid { crate_level_code, wrapper, code } => Some(DocTest {
crate_level: crate_level_code,
code,
wrapper: wrapper.map(
|super::make::WrapperInfo { before, after, returns_result, .. }| {
WrapperInfo { before, after, returns_result }
},
),
}),
DocTestWrapResult::SyntaxError { .. } => None,
},
original_code: text,
name,
});
}
}

#[derive(Serialize)]
pub(crate) struct WrapperInfo {
before: String,
after: String,
returns_result: bool,
}

#[derive(Serialize)]
pub(crate) struct DocTest {
crate_level: String,
code: String,
wrapper: Option<WrapperInfo>,
}

#[derive(Serialize)]
pub(crate) struct ExtractedDocTest {
file: String,
line: usize,
doctest_attributes: LangString,
original_code: String,
/// `None` if the code syntax is invalid.
doctest_code: Option<String>,
doctest_code: Option<DocTest>,
name: String,
}

Expand Down
139 changes: 98 additions & 41 deletions src/librustdoc/doctest/make.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,73 @@ pub(crate) struct DocTestBuilder {
pub(crate) can_be_merged: bool,
}

/// Contains needed information for doctest to be correctly generated with expected "wrapping".
pub(crate) struct WrapperInfo {
pub(crate) before: String,
pub(crate) after: String,
pub(crate) returns_result: bool,
insert_indent_space: bool,
}

impl WrapperInfo {
fn len(&self) -> usize {
self.before.len() + self.after.len()
}
}

/// Contains a doctest information. Can be converted into code with the `to_string()` method.
pub(crate) enum DocTestWrapResult {
Valid {
crate_level_code: String,
wrapper: Option<WrapperInfo>,
code: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure code is descriptive enough. it doesn't make it clear whether it the original unprocessed code, the fully processed code, or something else. in reality what exactly it contains depends on if we added a main function, so perhaps we could just add a doc comment to the field instead of trying to encapsulate all that complexity in a name.

If we do rename it, we should probably also rename any local variables whose name is derived from this due to the struct pattern sugar, such as in DocTestWrapResult::to_string.

},
/// Contains the original source code.
SyntaxError(String),
}

impl std::string::ToString for DocTestWrapResult {
fn to_string(&self) -> String {
match self {
Self::SyntaxError(s) => s.clone(),
Self::Valid { crate_level_code, wrapper, code } => {
let mut prog_len = code.len() + crate_level_code.len();
if let Some(wrapper) = wrapper {
prog_len += wrapper.len();
if wrapper.insert_indent_space {
prog_len += code.lines().count() * 4;
}
}
let mut prog = String::with_capacity(prog_len);

prog.push_str(crate_level_code);
if let Some(wrapper) = wrapper {
prog.push_str(&wrapper.before);

// add extra 4 spaces for each line to offset the code block
if wrapper.insert_indent_space {
write!(
prog,
"{}",
fmt::from_fn(|f| code
.lines()
.map(|line| fmt::from_fn(move |f| write!(f, " {line}")))
.joined("\n", f))
)
.unwrap();
} else {
prog.push_str(code);
}
prog.push_str(&wrapper.after);
} else {
prog.push_str(code);
}
prog
}
}
}
}

impl DocTestBuilder {
fn invalid(
crate_attrs: String,
Expand All @@ -210,53 +277,53 @@ impl DocTestBuilder {
/// lines before the test code begins.
pub(crate) fn generate_unique_doctest(
&self,
test_code: &str,
doctest_code: &str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: in extracted.rs, we use the name doctest_code to refer to the fully processed code, while here we are using it to refer to the unprocessed source. this seems needlessly confusing.

dont_insert_main: bool,
opts: &GlobalTestOptions,
crate_name: Option<&str>,
) -> (String, usize) {
) -> (DocTestWrapResult, usize) {
if self.invalid_ast {
// If the AST failed to compile, no need to go generate a complete doctest, the error
// will be better this way.
debug!("invalid AST:\n{test_code}");
return (test_code.to_string(), 0);
debug!("invalid AST:\n{doctest_code}");
return (DocTestWrapResult::SyntaxError(doctest_code.to_string()), 0);
}
let mut line_offset = 0;
let mut prog = String::new();
let everything_else = self.everything_else.trim();
let mut crate_level_code = String::new();
let processed_code = self.everything_else.trim().to_string();
if opts.attrs.is_empty() {
// If there aren't any attributes supplied by #![doc(test(attr(...)))], then allow some
// lints that are commonly triggered in doctests. The crate-level test attributes are
// commonly used to make tests fail in case they trigger warnings, so having this there in
// that case may cause some tests to pass when they shouldn't have.
prog.push_str("#![allow(unused)]\n");
crate_level_code.push_str("#![allow(unused)]\n");
line_offset += 1;
}

// Next, any attributes that came from the crate root via #![doc(test(attr(...)))].
for attr in &opts.attrs {
prog.push_str(&format!("#![{attr}]\n"));
crate_level_code.push_str(&format!("#![{attr}]\n"));
line_offset += 1;
}

// Now push any outer attributes from the example, assuming they
// are intended to be crate attributes.
if !self.crate_attrs.is_empty() {
prog.push_str(&self.crate_attrs);
crate_level_code.push_str(&self.crate_attrs);
if !self.crate_attrs.ends_with('\n') {
prog.push('\n');
crate_level_code.push('\n');
}
}
if !self.maybe_crate_attrs.is_empty() {
prog.push_str(&self.maybe_crate_attrs);
crate_level_code.push_str(&self.maybe_crate_attrs);
if !self.maybe_crate_attrs.ends_with('\n') {
prog.push('\n');
crate_level_code.push('\n');
}
}
if !self.crates.is_empty() {
prog.push_str(&self.crates);
crate_level_code.push_str(&self.crates);
if !self.crates.ends_with('\n') {
prog.push('\n');
crate_level_code.push('\n');
}
}

Expand All @@ -270,21 +337,24 @@ impl DocTestBuilder {
// NOTE: this is terribly inaccurate because it doesn't actually
// parse the source, but only has false positives, not false
// negatives.
test_code.contains(crate_name)
doctest_code.contains(crate_name)
{
// rustdoc implicitly inserts an `extern crate` item for the own crate
// which may be unused, so we need to allow the lint.
prog.push_str("#[allow(unused_extern_crates)]\n");
crate_level_code.push_str("#[allow(unused_extern_crates)]\n");

prog.push_str(&format!("extern crate r#{crate_name};\n"));
crate_level_code.push_str(&format!("extern crate r#{crate_name};\n"));
line_offset += 1;
}

// FIXME: This code cannot yet handle no_std test cases yet
if dont_insert_main || self.has_main_fn || prog.contains("![no_std]") {
prog.push_str(everything_else);
let wrapper = if dont_insert_main
|| self.has_main_fn
|| crate_level_code.contains("![no_std]")
{
None
} else {
let returns_result = everything_else.ends_with("(())");
let returns_result = processed_code.ends_with("(())");
// Give each doctest main function a unique name.
// This is for example needed for the tooling around `-C instrument-coverage`.
let inner_fn_name = if let Some(ref test_id) = self.test_id {
Expand Down Expand Up @@ -318,28 +388,15 @@ impl DocTestBuilder {
// /// ``` <- end of the inner main
line_offset += 1;

prog.push_str(&main_pre);

// add extra 4 spaces for each line to offset the code block
if opts.insert_indent_space {
write!(
prog,
"{}",
fmt::from_fn(|f| everything_else
.lines()
.map(|line| fmt::from_fn(move |f| write!(f, " {line}")))
.joined("\n", f))
)
.unwrap();
} else {
prog.push_str(everything_else);
};
prog.push_str(&main_post);
}

debug!("final doctest:\n{prog}");
Some(WrapperInfo {
before: main_pre,
after: main_post,
returns_result,
insert_indent_space: opts.insert_indent_space,
})
};

(prog, line_offset)
(DocTestWrapResult::Valid { code: processed_code, wrapper, crate_level_code }, line_offset)
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/doctest/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ fn make_test(
builder = builder.test_id(test_id.to_string());
}
let doctest = builder.build(None);
let (code, line_offset) =
let (wrapper, line_offset) =
doctest.generate_unique_doctest(test_code, dont_insert_main, opts, crate_name);
(code, line_offset)
(wrapper.to_string(), line_offset)
Comment on lines +20 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should s/wrapper/wrapped/, same reasoning as before.

}

/// Default [`GlobalTestOptions`] for these unit tests.
Expand Down
3 changes: 2 additions & 1 deletion src/librustdoc/html/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,8 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for CodeBlocks<'_, 'a, I> {
builder = builder.crate_name(krate);
}
let doctest = builder.build(None);
let (test, _) = doctest.generate_unique_doctest(&test, false, &opts, krate);
let (wrapped, _) = doctest.generate_unique_doctest(&test, false, &opts, krate);
let test = wrapped.to_string();
let channel = if test.contains("#![feature(") { "&amp;version=nightly" } else { "" };

let test_escaped = small_url_encode(test);
Expand Down
11 changes: 11 additions & 0 deletions tests/rustdoc-ui/extract-doctests-result.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Test to ensure that it generates expected output for `--output-format=doctest` command-line
// flag.

//@ compile-flags:-Z unstable-options --output-format=doctest
//@ normalize-stdout: "tests/rustdoc-ui" -> "$$DIR"
//@ check-pass

//! ```
//! let x = 12;
//! Ok(())
//! ```
1 change: 1 addition & 0 deletions tests/rustdoc-ui/extract-doctests-result.stdout
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"format_version":2,"doctests":[{"file":"$DIR/extract-doctests-result.rs","line":8,"doctest_attributes":{"original":"","should_panic":false,"no_run":false,"ignore":"None","rust":true,"test_harness":false,"compile_fail":false,"standalone_crate":false,"error_codes":[],"edition":null,"added_css_classes":[],"unknown":[]},"original_code":"let x = 12;\nOk(())","doctest_code":{"crate_level":"#![allow(unused)]\n","code":"let x = 12;\nOk(())","wrapper":{"before":"fn main() { fn _inner() -> core::result::Result<(), impl core::fmt::Debug> {\n","after":"\n} _inner().unwrap() }","returns_result":true}},"name":"$DIR/extract-doctests-result.rs - (line 8)"}]}
2 changes: 1 addition & 1 deletion tests/rustdoc-ui/extract-doctests.stdout
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"format_version":1,"doctests":[{"file":"$DIR/extract-doctests.rs","line":8,"doctest_attributes":{"original":"ignore (checking attributes)","should_panic":false,"no_run":false,"ignore":"All","rust":true,"test_harness":false,"compile_fail":false,"standalone_crate":false,"error_codes":[],"edition":null,"added_css_classes":[],"unknown":[]},"original_code":"let x = 12;\nlet y = 14;","doctest_code":"#![allow(unused)]\nfn main() {\nlet x = 12;\nlet y = 14;\n}","name":"$DIR/extract-doctests.rs - (line 8)"},{"file":"$DIR/extract-doctests.rs","line":13,"doctest_attributes":{"original":"edition2018,compile_fail","should_panic":false,"no_run":true,"ignore":"None","rust":true,"test_harness":false,"compile_fail":true,"standalone_crate":false,"error_codes":[],"edition":"2018","added_css_classes":[],"unknown":[]},"original_code":"let","doctest_code":null,"name":"$DIR/extract-doctests.rs - (line 13)"}]}
{"format_version":2,"doctests":[{"file":"$DIR/extract-doctests.rs","line":8,"doctest_attributes":{"original":"ignore (checking attributes)","should_panic":false,"no_run":false,"ignore":"All","rust":true,"test_harness":false,"compile_fail":false,"standalone_crate":false,"error_codes":[],"edition":null,"added_css_classes":[],"unknown":[]},"original_code":"let x = 12;\nlet y = 14;","doctest_code":{"crate_level":"#![allow(unused)]\n","code":"let x = 12;\nlet y = 14;","wrapper":{"before":"fn main() {\n","after":"\n}","returns_result":false}},"name":"$DIR/extract-doctests.rs - (line 8)"},{"file":"$DIR/extract-doctests.rs","line":13,"doctest_attributes":{"original":"edition2018,compile_fail","should_panic":false,"no_run":true,"ignore":"None","rust":true,"test_harness":false,"compile_fail":true,"standalone_crate":false,"error_codes":[],"edition":"2018","added_css_classes":[],"unknown":[]},"original_code":"let","doctest_code":null,"name":"$DIR/extract-doctests.rs - (line 13)"}]}
Loading