Add compile-fail test in rustdoc #30726

GuillaumeGomez · 2016-01-05T22:40:00Z

I still need to add error code explanation test with this, but I can't figure out a way to generate the .md files in order to test example source codes.

Will fix #27328.

steveklabnik · 2016-01-05T23:10:08Z

This feature will be real awesome.

alexcrichton · 2016-01-11T07:56:32Z

I personally don't think that this ends up being too useful of a feature as-is in practice. Knowing that an example doesn't compile provides no guarantee about preventing something from accidentally regressing in the future because it's far more likely to break for other reasons such as API change.

Along those lines the only "really useful" form of compile-fail tests in my opinion are the ones we have which check error messages. Those are quite flaky, however, as the order, number, and placement of errors changes over time in the compiler.

Overall I don't think that flagging an example as purely "this should not compile" is a useful feature in the long run, and the more useful "this should not compile with this message" is not something we want to provide because it is not stable.

rphmeier · 2016-01-11T08:09:42Z

src/librustdoc/test.rs

+        driver::compile_input(sess, &cstore, cfg, &input, &out, &None, None, control);
+    }).join() {
+        Ok(_) if compile_fail           => panic!("couldn't compile the test"),
+        Err(_) if compile_fail == false => panic!("test compiled while it wasn't supposed to"),


Doesn't Ok here mean that the code does compile (insofar as compilation doesn't panic)? The match here seems to indicate the opposite.

@rphmeier: Indeed, I switched error messages. Thanks for notifying me.

GuillaumeGomez · 2016-01-11T08:12:50Z

@alexcrichton: The point here isn't to prevent regression but to check if examples are still up to date. For the rest, I don't think I'm the best to answer.

brson · 2016-01-12T01:28:37Z

Overall I don't think that flagging an example as purely "this should not compile" is a useful feature in the long run, and the more useful "this should not compile with this message" is not something we want to provide because it is not stable.

This was intended to be the first step. Testing that examples don't compile is better than not testing them, and we can improve later with more precise checking of errors.

Compile-fail tests are an important type of test, and none of our documentation examples that don't compile are tested. We are planning on using this to test the compiler error index, which is filled with such examples.

Given that compile-fail doc tests are important, it is clear to me that the tool we should use to test them is rustdoc, the tool we do all our other documentation testing with.

Do you have other suggestions about how we might test documentation examples that don't compile?

brson · 2016-01-12T01:29:54Z

For the basic case of testing that examples fail to compile, without checking the specific errors, this patch lgtm, barring @alexcrichton's concerns.

alexcrichton · 2016-01-12T01:44:26Z

I guess what I was getting at here was two-fold:

As-is I don't necessarily think that this is too useful to have in the long run.
With the logical extension of testing for precise messages, I don't think we want to commit to that because of stability concerns.

I agree, though, that testing the error index is something we should definitely do, so I'd be happy with ensuring that this is feature gated. We can always stomach changing our own error messages, but it's not necessarily true that everyone else will.

I would personally prefer to see some more extensive changes before landing the initial bits, but that doesn't matter too much.

brson · 2016-01-14T18:43:23Z

@nikomatsakis is also thinking about how to expose compile-fail tests to users. We might want to come up with a single compile-fail scheme that works for both doc tests and unit tests.

GuillaumeGomez · 2016-01-17T13:26:46Z

The first step is done, I added the compile-fail option for rustdoc. Now it remains to add the check of diagnostic codes. For this, I have two questions:

Should I create a test specifically for this (if yes, how?) or do I have do complete an existing one?
How do I run it only on diagnostic codes? How do I get them generated?

bors · 2016-01-17T22:02:00Z

☔ The latest upstream changes (presumably #30978) made this pull request unmergeable. Please resolve the merge conflicts.

brson · 2016-01-20T00:41:51Z

@alexcrichton Again I believe it is important for Rust users to have compile-fail tests, and want to have some path forward for adding that functionality, but you sound like you are against it completely.

How do I run it only on diagnostic codes? How do I get them generated?

There is code somewhere for generating the html index from the error codes. I thought this was going through markdown intermediately, but apparently it is not. To run through rustdoc it should be generating markdown.

Should I create a test specifically for this (if yes, how?) or do I have do complete an existing one?

Yes. I think what needs to happen to make this work is:

error-index-generator should generate markdown, not html.
the build rules for error-index.html should be updated to run the error-index markdown through rustdoc to generate the html.
More doc testing rules need to be added. This'll probably be a new macro like DEF_DOC_TEST that uses error-index-generator to build the markdown, then rustdoc to run the tests. You'll end up with a new set of parameterized build rules like check-stage$(1)-T-$(2)-H-$(3)-doc-error-index-exec.
Add that rule to check-stage$(1)-docs here.

alexcrichton · 2016-01-20T00:49:24Z

@brson sorry, but to clarify, when I said this:

I agree, though, that testing the error index is something we should definitely do, so I'd be happy with ensuring that this is feature gated.

I meant that I'm fine moving forward with this so long as it is feature gated or somehow requires the nightly compiler (e.g. isn't present on stable Rust)

brson · 2016-01-27T01:38:43Z

OK, let's move forward on this but make the feature unstable. @alexcrichton is a command-line flag sufficient, like --allow-compile-fail-tests, or should we try something more aggressive?

@GuillaumeGomez do the rest of the steps for testing the index make sense?

alexcrichton · 2016-01-27T01:45:02Z

It may end up being an equivalent amount of code to detect something like #![feature(rustdoc_cfail)] in rustdoc itself, which I think would be my preferred course of action. That at least meshes with our story elsewhere and actually gates stable/nightly

GuillaumeGomez · 2016-01-27T07:55:03Z

@brson: I didn't test yet, very busy with gtk-rs. I'll have time this week-end.

bors · 2016-01-27T16:11:57Z

☔ The latest upstream changes (presumably #31089) made this pull request unmergeable. Please resolve the merge conflicts.

brson · 2016-01-29T00:54:39Z

Feature flag is ok by me.

brson · 2016-02-11T22:44:07Z

@bors r+ Let's see how this goes...

bors · 2016-02-11T22:44:08Z

📌 Commit 03dd31c has been approved by brson

alexcrichton · 2016-02-11T23:01:57Z

src/librustdoc/html/markdown.rs

@@ -438,6 +440,18 @@ pub fn find_testable_code(doc: &str, tests: &mut ::test::Collector) {
    }
 }

+fn get_unstable_features_setting() -> bool {


Shouldn't this just call the function that's already in the compiler?

This one is a bit shorter but it could (should?).

I have a feeling it will be much shorter if you call that one instead! It's also best to not reproduce logic wherever possible.

Can you r- please?

alexcrichton · 2016-02-11T23:13:07Z

@bors: r-

Some last few details to attend to

GuillaumeGomez · 2016-02-11T23:52:23Z

@alexcrichton: Updated, you were absolutely right, AssertRecover wasn't needed.

GuillaumeGomez · 2016-02-12T08:11:03Z

src/librustdoc/html/markdown.rs

@@ -462,6 +467,11 @@ impl LangString {
        let mut seen_rust_tags = false;
        let mut seen_other_tags = false;
        let mut data = LangString::all_false();
+        let allow_compile_fail = if let UnstableFeatures::Cheat = get_unstable_features_setting() {


@brson: This seems to not work. For what I understood, if it returned UnstableFeatures::Cheat, it means that unstable features were allowed. Did I understood wrong?

env::var("RUSTC_BOOTSTRAP_KEY").ok() always returns None.

GuillaumeGomez · 2016-02-12T17:32:39Z

Test is good! \o/

brson · 2016-02-12T17:46:09Z

@bors r+

bors · 2016-02-12T17:46:10Z

📌 Commit eb7664b has been approved by brson

bors · 2016-02-12T18:25:09Z

⌛ Testing commit eb7664b with merge ce4b75f...

@brson

r? @brson cc @alexcrichton I still need to add error code explanation test with this, but I can't figure out a way to generate the `.md` files in order to test example source codes. Will fix #27328.

alexcrichton · 2016-02-13T00:15:40Z

our linux-cross-opt builder disappeared, but otherwise all test passed, so merging manually.

michaelsproul · 2016-02-13T00:25:43Z

Well done @GuillaumeGomez, this looks great!

GuillaumeGomez · 2016-02-13T11:57:18Z

@michaelsproul: Thanks! :)

@alexcrichton

…hton Compile fail stable Since #30726, we never made the `compile_fail` flag nor the error code check stable. I think it's time to change this fact. r? @alexcrichton

rust-highfive assigned brson Jan 5, 2016

GuillaumeGomez force-pushed the compile-fail branch from f67cc66 to 61a1146 Compare January 10, 2016 23:03

rphmeier reviewed Jan 11, 2016
View reviewed changes

GuillaumeGomez force-pushed the compile-fail branch 2 times, most recently from c80fc19 to 943d205 Compare January 12, 2016 22:05

GuillaumeGomez force-pushed the compile-fail branch 5 times, most recently from b1918e7 to a65e549 Compare January 16, 2016 20:28

GuillaumeGomez force-pushed the compile-fail branch 2 times, most recently from 778fa6a to 05bd4c1 Compare January 18, 2016 08:00

GuillaumeGomez force-pushed the compile-fail branch 2 times, most recently from 219003d to 19d5420 Compare January 31, 2016 19:17

GuillaumeGomez force-pushed the compile-fail branch 6 times, most recently from 91e535a to 194f1ca Compare February 11, 2016 08:21

brson mentioned this pull request Feb 11, 2016

error index tests are not run during make check #31587

Closed

alexcrichton reviewed Feb 11, 2016
View reviewed changes

GuillaumeGomez force-pushed the compile-fail branch from 03dd31c to abd5ffb Compare February 11, 2016 23:51

GuillaumeGomez reviewed Feb 12, 2016
View reviewed changes

GuillaumeGomez force-pushed the compile-fail branch from abd5ffb to eb7664b Compare February 12, 2016 12:30

alexcrichton merged commit eb7664b into rust-lang:master Feb 13, 2016

GuillaumeGomez deleted the compile-fail branch February 13, 2016 00:19

GuillaumeGomez mentioned this pull request Apr 2, 2016

Normalization for long error codes explanations RFC rust-lang/rfcs#1567

Closed

GuillaumeGomez mentioned this pull request Aug 17, 2017

Compile fail stable #43949

Merged

Add compile-fail test in rustdoc #30726

Add compile-fail test in rustdoc #30726

Conversation

GuillaumeGomez commented Jan 5, 2016

steveklabnik commented Jan 5, 2016

alexcrichton commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 11, 2016

brson commented Jan 12, 2016

brson commented Jan 12, 2016

alexcrichton commented Jan 12, 2016

brson commented Jan 14, 2016

GuillaumeGomez commented Jan 17, 2016

bors commented Jan 17, 2016

brson commented Jan 20, 2016

alexcrichton commented Jan 20, 2016

brson commented Jan 27, 2016

alexcrichton commented Jan 27, 2016

GuillaumeGomez commented Jan 27, 2016

bors commented Jan 27, 2016

brson commented Jan 29, 2016

brson commented Feb 11, 2016

bors commented Feb 11, 2016

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

alexcrichton commented Feb 11, 2016

GuillaumeGomez commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 12, 2016

brson commented Feb 12, 2016

bors commented Feb 12, 2016

bors commented Feb 12, 2016

alexcrichton commented Feb 13, 2016

michaelsproul commented Feb 13, 2016

GuillaumeGomez commented Feb 13, 2016