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 2 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(),
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::DocTestWrapper;
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(
&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 {
DocTestWrapper::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 }
},
),
}),
DocTestWrapper::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
124 changes: 86 additions & 38 deletions src/librustdoc/doctest/make.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,64 @@ 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 DocTestWrapper {
Valid { crate_level_code: String, wrapper: Option<WrapperInfo>, code: String },
Comment on lines +202 to +203
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The name DocTestWrapper seems a bit confusing since we already have DocTest and WrapperInfo (and unlike WrapperInfo, this also includes the code itself), maybe this could be DocTestWrapResult as it is the result of wrapping the doctest, and it has an error variant like Result?
  2. couldn't we avoid specifying all the fields twice by leveraging the existing struct? like so: Valid(DocTest)

SyntaxError(String),
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
SyntaxError(String),
/// syntactically invalid code that should be passed through verbatim.
SyntaxError(String),

It wasn't immediately obvious that the String here is source code and not a description of the error, as errstrs are a fairly common pattern in rust.

I assume we do this partially for compile_fail tests and also because rustc provides better diagnostics?

}

impl std::string::ToString for DocTestWrapper {
fn to_string(&self) -> String {
match self {
Self::SyntaxError(s) => s.clone(),
Self::Valid { crate_level_code, wrapper, code } => {
let prog_len = code.len()
+ crate_level_code.len()
+ wrapper.as_ref().map(|w| w.len()).unwrap_or(0);
Comment on lines +212 to +214
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're doing all this to estimate capacity, should we also do + 4 * code.lines().count(), or is that counterproductive? how much do we care about avoiding allocations?

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 Down Expand Up @@ -214,49 +272,49 @@ impl DocTestBuilder {
dont_insert_main: bool,
opts: &GlobalTestOptions,
crate_name: Option<&str>,
) -> (String, usize) {
) -> (DocTestWrapper, 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);
return (DocTestWrapper::SyntaxError(test_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 code = self.everything_else.trim().to_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could we call this inner_code to disambiguate?

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 @@ -274,17 +332,20 @@ impl DocTestBuilder {
{
// 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 = 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 +379,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)
(DocTestWrapper::Valid { 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)
}

/// 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 (wrapper, _) = doctest.generate_unique_doctest(&test, false, &opts, krate);
let test = wrapper.to_string();
Comment on lines +311 to +312
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, _) = doctest.generate_unique_doctest(&test, false, &opts, krate);
let test = wrapper.to_string();
let (wrapped, _) = doctest.generate_unique_doctest(&test, false, &opts, krate);
let test = wrapped.to_string();

Nit: it's not just a wrapper, but the wrapper in combination with the inner code that has been wrapped. we make this distinction in the JSON output, so maybe we should stick to that terminology here.

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