Error codes checkup and rustdoc test fix #67850

GuillaumeGomez · 2020-01-04T00:44:31Z

This PR does a few things:

fix how rustdoc checks that an error code has been thrown (it only checked for "E0XXX" so if it appeared in the output because the file has it in its name or wherever, it passed the test, which was incorrect)
fix the failing code examples that weren't throwing the expected error code

rust-highfive · 2020-01-04T01:06:36Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-01-04T00:45:09.2620133Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-04T00:45:10.0570101Z ##[command]git config gc.auto 0
2020-01-04T00:45:10.0577246Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-04T00:45:10.0582873Z ##[command]git config --get-all http.proxy
2020-01-04T00:45:10.0586443Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67850/merge:refs/remotes/pull/67850/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Mark-Simulacrum · 2020-01-04T01:12:17Z

I will review once #67837 has been merged and this is rebased atop of it (also, looks like there's a CI failure).

GuillaumeGomez · 2020-01-04T11:46:10Z

Forgot to run rustfmt on the new checker... Let's wait for #67837 now. :)

GuillaumeGomez · 2020-01-06T13:08:46Z

@Mark-Simulacrum #67837 has been merged. :)

src/bootstrap/test.rs

src/tools/error-codes/main.rs

ollie27 · 2020-01-10T23:30:38Z

src/bootstrap/test.rs

@@ -704,6 +704,39 @@ impl Step for RustdocUi {
    }
 }

+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+pub struct ErrorCodesCheck {


There is already a step to run rustdoc tests on the error index:

rust/src/bootstrap/test.rs

Line 1393 in 1756313

pub struct ErrorIndex {

Does it not work properly?

This is not for the same purpose. ErrorIndex generates it, but it doesn't test that the code examples inside the explanations are compiling (or not) as expected.

Yes it does. It uses error_index_generator to concatenate all of the error code docs into a single error-index.md file and then runs rustdoc --test on that:

rust/src/bootstrap/test.rs

Lines 1411 to 1436 in 1756313

/// Runs the error index generator tool to execute the tests located in the error

/// index.

///

/// The `error_index_generator` tool lives in `src/tools` and is used to

/// generate a markdown file from the error indexes of the code base which is

/// then passed to `rustdoc --test`.

fn run(self, builder: &Builder<'_>) {

let compiler = self.compiler;

builder.ensure(compile::Std { compiler, target: compiler.host });

let dir = testdir(builder, compiler.host);

t!(fs::create_dir_all(&dir));

let output = dir.join("error-index.md");

let mut tool = tool::ErrorIndex::command(

builder,

builder.compiler(compiler.stage, builder.config.build),

);

tool.arg("markdown").arg(&output).env("CFG_BUILD", &builder.config.build);

builder.info(&format!("Testing error-index stage{}", compiler.stage));

let _time = util::timeit(&builder);

builder.run_quiet(&mut tool);

markdown_test(builder, compiler, &output);

}

And I can tell that it doesn't work since a lot of error codes I fixed in this PR weren't detected.

But why doesn't it work? If we have existing code that overlaps this one, ideally we'd fix the pre-existing code, rather than replace it wholesale (or at least, we should make sure that we're replacing it, rather than just also adding atop it).

This is a good question. Let me check again.

ollie27 · 2020-01-10T23:35:44Z

src/bootstrap/test.rs

+    fn run(self, builder: &Builder<'_>) {
+        builder.ensure(compile::Std { compiler: self.compiler, target: self.compiler.host });
+
+        let mut cmd = builder.tool_cmd(Tool::ErrorCodesCheck);


Does this really need to be done using a new tool? Rustbuild can already run rustdoc --test on all .md files in a directory using the DocTest step.

Either way this will be slower than the current ErrorIndex step because it will have to call rustdoc --test once for each error code rather than just once for all of them.

You need to write a tool in any case to import the markdown files. So instead of having everything at once, at least we get a very precise error output. Also, the run is a bit longer, but nothing much bigger.

GuillaumeGomez · 2020-01-11T17:50:20Z

Ping @Mark-Simulacrum

Mark-Simulacrum · 2020-01-11T18:50:55Z

I think @ollie27's comments around error-index-generator need to get resolved and then I can take another look (or not, if it turns out we're already doing everything necessary here).

GuillaumeGomez · 2020-01-12T16:47:26Z

@ollie27 was absolutely right: adding this new check was useless. I discovered this issue, believing that the change of the error explanations format somehow messed with the run of rustdoc --test so I fixed rustdoc and adding the new (and useless) checker. Luckily, it was detected and I removed it. Thanks a lot @ollie27!

I think this is ready now.

Mark-Simulacrum · 2020-01-12T16:53:05Z

Please update the PR description (some of it seems a bit outdated). After that, r=me.

GuillaumeGomez · 2020-01-13T10:26:35Z

@bors: r=Mark-Simulacrum

bors · 2020-01-13T10:26:37Z

📌 Commit 5b7d64e has been approved by Mark-Simulacrum

bors · 2020-01-13T12:49:23Z

⌛ Testing commit 5b7d64e with merge bf84eb5...

…acrum Error codes checkup and rustdoc test fix This PR does a few things: * fix how rustdoc checks that an error code has been thrown (it only checked for "E0XXX" so if it appeared in the output because the file has it in its name or wherever, it passed the test, which was incorrect) * fix the failing code examples that weren't throwing the expected error code

bors · 2020-01-13T16:11:22Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing bf84eb5 to master...

rust-highfive assigned Mark-Simulacrum Jan 4, 2020

Mark-Simulacrum added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jan 4, 2020

GuillaumeGomez force-pushed the err-codes-checkup branch from 0dce9d2 to 157a3d1 Compare January 4, 2020 11:45

Fix error code failure check in rustdoc test

9876f6b

GuillaumeGomez force-pushed the err-codes-checkup branch from 157a3d1 to 9d84816 Compare January 6, 2020 13:08

Mark-Simulacrum reviewed Jan 6, 2020

View reviewed changes

src/bootstrap/test.rs Outdated Show resolved Hide resolved

src/tools/error-codes/main.rs Outdated Show resolved Hide resolved

src/tools/error-codes/main.rs Outdated Show resolved Hide resolved

ollie27 reviewed Jan 10, 2020

View reviewed changes

Fix error codes explanation' code examples

5b7d64e

GuillaumeGomez force-pushed the err-codes-checkup branch from 2e8db67 to 5b7d64e Compare January 12, 2020 16:45

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jan 13, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2020

bors merged commit 5b7d64e into rust-lang:master Jan 13, 2020

GuillaumeGomez deleted the err-codes-checkup branch January 13, 2020 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error codes checkup and rustdoc test fix #67850

Error codes checkup and rustdoc test fix #67850

GuillaumeGomez commented Jan 4, 2020 •

edited

Loading

rust-highfive commented Jan 4, 2020

Mark-Simulacrum commented Jan 4, 2020

GuillaumeGomez commented Jan 4, 2020

GuillaumeGomez commented Jan 6, 2020

ollie27 Jan 10, 2020

GuillaumeGomez Jan 11, 2020

ollie27 Jan 11, 2020

GuillaumeGomez Jan 11, 2020

Mark-Simulacrum Jan 11, 2020

GuillaumeGomez Jan 12, 2020

ollie27 Jan 10, 2020

GuillaumeGomez Jan 11, 2020

GuillaumeGomez commented Jan 11, 2020

Mark-Simulacrum commented Jan 11, 2020

GuillaumeGomez commented Jan 12, 2020

Mark-Simulacrum commented Jan 12, 2020

GuillaumeGomez commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

	/// Runs the error index generator tool to execute the tests located in the error
	/// index.
	///
	/// The `error_index_generator` tool lives in `src/tools` and is used to
	/// generate a markdown file from the error indexes of the code base which is
	/// then passed to `rustdoc --test`.
	fn run(self, builder: &Builder<'_>) {
	let compiler = self.compiler;

	builder.ensure(compile::Std { compiler, target: compiler.host });

	let dir = testdir(builder, compiler.host);
	t!(fs::create_dir_all(&dir));
	let output = dir.join("error-index.md");

	let mut tool = tool::ErrorIndex::command(
	builder,
	builder.compiler(compiler.stage, builder.config.build),
	);
	tool.arg("markdown").arg(&output).env("CFG_BUILD", &builder.config.build);

	builder.info(&format!("Testing error-index stage{}", compiler.stage));
	let _time = util::timeit(&builder);
	builder.run_quiet(&mut tool);
	markdown_test(builder, compiler, &output);
	}

Error codes checkup and rustdoc test fix #67850

Error codes checkup and rustdoc test fix #67850

Conversation

GuillaumeGomez commented Jan 4, 2020 • edited Loading

rust-highfive commented Jan 4, 2020

Mark-Simulacrum commented Jan 4, 2020

GuillaumeGomez commented Jan 4, 2020

GuillaumeGomez commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 11, 2020

Mark-Simulacrum commented Jan 11, 2020

GuillaumeGomez commented Jan 12, 2020

Mark-Simulacrum commented Jan 12, 2020

GuillaumeGomez commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

GuillaumeGomez commented Jan 4, 2020 •

edited

Loading