-
Notifications
You must be signed in to change notification settings - Fork 14k
Compile fail stable #43949
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
Conversation
1c52987 to
a81681c
Compare
|
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). |
|
If there is a consensus for not stabilizing the error code check, I'll just remove the second commit. |
|
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? |
a81681c to
a52176f
Compare
|
Ok so I removed the error codes from the stabilization and added the doc for the |
|
Are there any tests for this in the |
|
The tests are literally all over the docs. Where would you want me to add them? |
|
Can you add a unit test for this in |
|
Sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did these get indented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be able to see the backticks when rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: `
|
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 |
|
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. |
a52176f to
e8da3e1
Compare
|
I think I made the changes you required @QuietMisdreavus. Or did I miss anything? |
e8da3e1 to
56fb9ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that taking the ignore off of this keeps making the test fail, because it's a doc comment that doesn't document anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Erf, good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looking for a note here about how things may start compiling in later releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to put this change in another commit?
a2f7aa5 to
82c04af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
82c04af to
a6ab711
Compare
QuietMisdreavus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More travis failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't indent these docblocks. It's treating the ``` as part of the doctest and failing to parse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Documentation comment that doesn't document anything"
Probably good to stick ignore on this one.
a6ab711 to
ebc195d
Compare
|
Finally all good! |
|
Any news in here? |
|
ping the @rust-lang/dev-tools team for signoffs; here's some manual ticky boxes for you: |
|
Assuming all the stuff from #43949 (comment) is addressed (it appears so), LGTM. @CAROLBOT reviewed |
|
@rfcbot reviewed |
|
@bors: r+ |
|
📌 Commit ebc195d has been approved by |
…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
|
☀️ Test successful - status-appveyor, status-travis |
Since #30726, we never made the
compile_failflag nor the error code check stable. I think it's time to change this fact.r? @alexcrichton