Skip to content

/rust

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

Compile fail stable #43949

Merged
merged 3 commits into from Sep 15, 2017

Conversation

Reviewers

@frewsxcv frewsxcv

@QuietMisdreavus QuietMisdreavus

Assignees

@nrc nrc

Labels
I-nominated S-waiting-on-team T-dev-tools relnotes
Projects
None yet
Milestone
No milestone
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Aug 17, 2017 •
edited

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 rust-highfive assigned alexcrichton Aug 17, 2017

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile_fail_stable branch from 1c52987 to a81681c Aug 17, 2017

@Gankro

This comment has been minimized.

Sign in to view
Copy link
Contributor

Gankro commented Aug 17, 2017

I'm a big fan of having "this doesn't compile" on stable, but as per our historical gripes with compile-fail tests, I'm not sure if we should allow stable code to check error codes, since I'm like 99.9% sure those aren't stable (in that something may stop compiling for a different reason).

@GuillaumeGomez GuillaumeGomez referenced this pull request Aug 17, 2017

Closed

Add a cfail mode for doc comments #24565

@GuillaumeGomez

This comment has been minimized.

Sign in to view
Copy link
Member Author

GuillaumeGomez commented Aug 17, 2017

If there is a consensus for not stabilizing the error code check, I'll just remove the second commit.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 17, 2017

Has this been discussed for stabilization? Was this discussed with @rust-lang/dev-tools?

I would personally still be against stabilizing error codes, and do we have sufficient documentation to stabilize this feature as well?
@kennytm

This comment has been minimized.

Sign in to view
Copy link
Member

kennytm commented Aug 17, 2017 •
edited

cc #42288 for documentation (https://github.com/rust-lang/rust/blob/master/src/doc/rustdoc/src/documentation-tests.md)

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile_fail_stable branch from a81681c to a52176f Aug 22, 2017

@GuillaumeGomez

This comment has been minimized.

Sign in to view
Copy link
Member Author

GuillaumeGomez commented Aug 22, 2017

Ok so I removed the error codes from the stabilization and added the doc for the compile_fail directive (and improved the doc a bit too).
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 22, 2017

Are there any tests for this in the src/test/rustdoc folder? I'd expect that if we had existing tests this'd remove the feature gates from those tests.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 22, 2017

I'll I'm going to defer to @nrc for review as I think the dev-tools team will want to sign off on this.

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned alexcrichton Aug 22, 2017

@GuillaumeGomez

This comment has been minimized.

Sign in to view
Copy link
Member Author

GuillaumeGomez commented Aug 22, 2017

The tests are literally all over the docs. Where would you want me to add them?
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 22, 2017

Can you add a unit test for this in src/test/rustdoc to ensure that it doesn't require a feature gate?

@arielb1 arielb1 added the S-waiting-on-review label Aug 22, 2017

@GuillaumeGomez

This comment has been minimized.

Sign in to view
Copy link
Member Author

GuillaumeGomez commented Aug 23, 2017

Sure.
@frewsxcv

frewsxcv reviewed Aug 23, 2017
View changes

src/doc/rustdoc/src/documentation-tests.md Outdated
```text
/// Some documentation.
# fn foo() {}
```

This comment has been minimized.

Sign in to view
Copy link
@frewsxcv

frewsxcv Aug 23, 2017

Member

why did these get indented?

This comment has been minimized.

Sign in to view
Copy link
@GuillaumeGomez

GuillaumeGomez Aug 23, 2017

Author Member

To be able to see the backticks when rendered.

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Aug 24, 2017

Member

But that's not the point. The point is to show what happens when you hide a line with #, not to highlight the text differently.

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Aug 24, 2017

Member

also it caused a travis failure:

[01:04:47] ---- /checkout/src/doc/rustdoc/src/documentation-tests.md - Documentation_tests (line 68) stdout ----
[01:04:47] 	error: unknown start of token: `

@steveklabnik steveklabnik referenced this pull request Aug 23, 2017

Closed

Add dev-tools peer groups to rfcbot #153

@nrc nrc added T-dev-tools I-nominated labels Aug 24, 2017

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member

nrc commented Aug 24, 2017

I don't think we can yet get an auto-rfc-bot thing for the dev-tools/docs team, so manually. I propose we should accept this (stabilise the flag) - seems like a fairly harmless feature that has not changed in the last 1.5 years and is considered useful by some people.

Please sign off - @michaelwoerister @killercup @QuietMisdreavus @steveklabnik @jonathandturner @japaric
@QuietMisdreavus

This comment has been minimized.

Sign in to view
Copy link
Member

QuietMisdreavus commented Aug 24, 2017

I approve this change.

In the docs team meeting yesterday, we mentioned that it might be prudent to include a note in the documentation stating that "this code doesn't compile" isn't guaranteed to be stable from release to release. Language features like NLL, struct field init shorthand, using a nightly feature without a feature flag (if it's stabilized as-is), etc, can make code that was previously broken start compiling. Making it explicit that some things may change between releases would be helpful in terms of cutting down on "where is this documented" questions.

@carols10cents carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Aug 28, 2017

@arielb1 arielb1 added S-waiting-on-team and removed S-waiting-on-author labels Aug 29, 2017

@frewsxcv frewsxcv referenced this pull request Sep 1, 2017

Closed

Document compile_fail for doc tests #42288

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile_fail_stable branch from a52176f to e8da3e1 Sep 1, 2017

@GuillaumeGomez

This comment has been minimized.

Sign in to view
Copy link
Member Author

GuillaumeGomez commented Sep 1, 2017

I think I made the changes you required @QuietMisdreavus. Or did I miss anything?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile_fail_stable branch from e8da3e1 to 56fb9ef Sep 2, 2017

@QuietMisdreavus

QuietMisdreavus reviewed Sep 2, 2017
View changes

src/doc/rustdoc/src/documentation-tests.md Outdated
@@ -136,7 +149,7 @@ explanation.
Another case where the use of `#` is handy is when you want to ignore
error handling. Lets say you want the following,

```rust,ignore
```rust

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Sep 2, 2017

Member

Note that taking the ignore off of this keeps making the test fail, because it's a doc comment that doesn't document anything.

This comment has been minimized.

Sign in to view
Copy link
@GuillaumeGomez

GuillaumeGomez Sep 2, 2017

Author Member

Erf, good point!
src/doc/rustdoc/src/documentation-tests.md Outdated
@@ -233,6 +246,16 @@ not actually pass as a test.
# fn foo() {}
```

`compile_fail` tells `rustdoc` that the compilation should fail. If it
compiles, then the test will fail.

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Sep 2, 2017

Member

Still looking for a note here about how things may start compiling in later releases.
src/doc/rustdoc/src/documentation-tests.md
@@ -16,6 +16,19 @@ The basic idea is this:
The triple backticks start and end code blocks. If this were in a file named `foo.rs`,
running `rustdoc --test foo.rs` will extract this example, and then run it as a test.

Please note that by default, if no language is set for the block code, `rustdoc`
assumes it is `Rust` code. So the following:

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Sep 2, 2017

Member

This block doesn't seem related to the rest of this PR? Like, it's worth noting, but it's not part of "making compile_fail stable".

This comment has been minimized.

Sign in to view
Copy link
@GuillaumeGomez

GuillaumeGomez Sep 2, 2017

Author Member

Do you want me to put this change in another commit?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile_fail_stable branch 2 times, most recently from a2f7aa5 to 82c04af Sep 2, 2017

@QuietMisdreavus

QuietMisdreavus reviewed Sep 2, 2017
View changes

src/doc/rustdoc/src/documentation-tests.md Outdated
@@ -233,6 +246,17 @@ not actually pass as a test.
# fn foo() {}
```

`compile_fail` tells `rustdoc` that the compilation should fail. If it
compiles, then the test will fail. However please note that a code
failing with the current rustc version may works in a future release.

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Sep 2, 2017

Member

Light phrasing nit: "However please note that code failing with the current Rust release may work in a future release, as new features are added."

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile_fail_stable branch from 82c04af to a6ab711 Sep 2, 2017

@QuietMisdreavus

QuietMisdreavus reviewed Sep 2, 2017
View changes

Copy link
Member

QuietMisdreavus left a comment

More travis failures
src/doc/rustdoc/src/documentation-tests.md Outdated

```rust
let x = 5;
```

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Sep 2, 2017

Member

Don't indent these docblocks. It's treating the ``` as part of the doctest and failing to parse it.
src/doc/rustdoc/src/documentation-tests.md
/// let x = 5;
/// x += 2; // shouldn't compile!
/// ```
```

This comment has been minimized.

Sign in to view
Copy link
@QuietMisdreavus

QuietMisdreavus Sep 2, 2017

Member

"Documentation comment that doesn't document anything"

Probably good to stick ignore on this one.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:compile_fail_stable branch from a6ab711 to ebc195d Sep 3, 2017

@GuillaumeGomez

This comment has been minimized.

Sign in to view
Copy link
Member Author

GuillaumeGomez commented Sep 3, 2017

Finally all good!

@QuietMisdreavus QuietMisdreavus referenced this pull request Sep 7, 2017

Merged

Codeblock color #44397

@GuillaumeGomez

This comment has been minimized.

Sign in to view
Copy link
Member Author

GuillaumeGomez commented Sep 10, 2017

Any news in here?
@carols10cents

This comment has been minimized.

Sign in to view
Copy link
Member

carols10cents commented Sep 11, 2017 •
edited by nrc

ping the @rust-lang/dev-tools team for signoffs; here's some manual ticky boxes for you:
@killercup

This comment has been minimized.

Sign in to view
Copy link
Member

killercup commented Sep 14, 2017

Assuming all the stuff from #43949 (comment) is addressed (it appears so), LGTM.

@carolbot reviewed
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Sep 14, 2017

@rfcbot reviewed
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Sep 14, 2017

@bors: r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 14, 2017

📌 Commit ebc195d has been approved by alexcrichton
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 15, 2017

⌛️ Testing commit ebc195d with merge fd4bef5...

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

@bors
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
Loading status checks…
fd4bef5
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing fd4bef5 to master...

@bors bors merged commit ebc195d into rust-lang:master Sep 15, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:compile_fail_stable branch Sep 15, 2017

@alexcrichton alexcrichton added the relnotes label Sep 15, 2017

@QuietMisdreavus QuietMisdreavus referenced this pull request Sep 21, 2017

Merged

Deprecate several flags in rustdoc #44138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.