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

Add compile-fail test in rustdoc #30726

Merged
merged 5 commits into from Feb 13, 2016

Conversation

Projects
None yet
7 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 5, 2016

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.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 5, 2016

This feature will be real awesome.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch from f67cc66 to 61a1146 Jan 10, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 11, 2016

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.

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"),

This comment has been minimized.

@rphmeier

rphmeier Jan 11, 2016

Contributor

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

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 11, 2016

Member

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

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 11, 2016

@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

This comment has been minimized.

Copy link
Contributor

brson commented Jan 12, 2016

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

This comment has been minimized.

Copy link
Contributor

brson commented Jan 12, 2016

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 12, 2016

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

  1. As-is I don't necessarily think that this is too useful to have in the long run.
  2. 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.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch 2 times, most recently from c80fc19 to 943d205 Jan 12, 2016

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 14, 2016

@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 GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch 5 times, most recently from b1918e7 to a65e549 Jan 16, 2016

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 17, 2016

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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 17, 2016

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch 2 times, most recently from 778fa6a to 05bd4c1 Jan 17, 2016

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 20, 2016

@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

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 20, 2016

@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

This comment has been minimized.

Copy link
Contributor

brson commented Jan 27, 2016

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

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 27, 2016

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

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 27, 2016

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2016

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

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 29, 2016

Feature flag is ok by me.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch 2 times, most recently from 219003d to 19d5420 Jan 30, 2016

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch 6 times, most recently from 91e535a to 194f1ca Feb 10, 2016

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 11, 2016

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 11, 2016

📌 Commit 03dd31c has been approved by brson

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

fn get_unstable_features_setting() -> bool {

This comment has been minimized.

@alexcrichton

alexcrichton Feb 11, 2016

Member

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

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 11, 2016

Member

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

This comment has been minimized.

@alexcrichton

alexcrichton Feb 11, 2016

Member

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.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 11, 2016

Member

Can you r- please?

let b_cfg = AssertRecoverSafe::new(cfg.clone());
let b_input = AssertRecoverSafe::new(&input);
let b_out = AssertRecoverSafe::new(&out);
let b_control = AssertRecoverSafe::new(&control);

This comment has been minimized.

@alexcrichton

alexcrichton Feb 11, 2016

Member

Was AssertRecoverSafe needed for all of these variables? It may be best to match what the compiler does and spawn a new thread (or use a monitor function) rather than try to catch the panic here.

I doubt that much of this is actually recover safe

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 11, 2016

Member

I tried out but all variables need to be wrapped it seems. And it does recover on panic.

This comment has been minimized.

@alexcrichton

alexcrichton Feb 11, 2016

Member

The point of the AssertRecoverSafe wrapper is to signal that this is recover-safe. I highly doubt any of these types are (they're not meant to be used in that situation), so this should follow the same strategy as the rest of the compiler (spawning a new function)

let b_control = AssertRecoverSafe::new(&control);

panic::recover(|| {
AssertRecoverSafe::new(driver::compile_input(&b_sess, &b_cstore, (*b_cfg).clone(),

This comment has been minimized.

@alexcrichton

alexcrichton Feb 11, 2016

Member

The AssertRecoverSafe wrapper shouldn't be needed for the return type

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 11, 2016

Member

Last time I checked it did. Might worth it to retry.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 11, 2016

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch from 03dd31c to abd5ffb Feb 11, 2016

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 11, 2016

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

@@ -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() {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 12, 2016

Member

@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?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 12, 2016

Member

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile-fail branch from abd5ffb to eb7664b Feb 12, 2016

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 12, 2016

Test is good! \o/

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 12, 2016

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2016

📌 Commit eb7664b has been approved by brson

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2016

⌛️ Testing commit eb7664b with merge ce4b75f...

bors added a commit that referenced this pull request Feb 12, 2016

Auto merge of #30726 - GuillaumeGomez:compile-fail, r=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

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 13, 2016

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

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

1 of 2 checks passed

homu Testing commit eb7664b with merge ce4b75f...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:compile-fail branch Feb 13, 2016

@michaelsproul

This comment has been minimized.

Copy link
Contributor

michaelsproul commented Feb 13, 2016

Well done @GuillaumeGomez, this looks great!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 13, 2016

@michaelsproul: Thanks! :)

bors added a commit that referenced this pull request Sep 15, 2017

Auto merge of #43949 - GuillaumeGomez:compile_fail_stable, r=alexcric…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment