Skip to content
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

Avoid processing features on unconfigured crates #34969

Merged
merged 2 commits into from
Jul 28, 2016

Conversation

jseyfried
Copy link
Contributor

Fixes #34932, a regression caused by #34272.
r? @nrc

@retep998
Copy link
Member

Why a compile fail test? Isn't the idea that disabling the crate with #![cfg] should allow it to compile despite use of #![feature]?

@jseyfried
Copy link
Contributor Author

jseyfried commented Jul 21, 2016

@retep998 feature doesn't prevent compilation on the nightlies since it is only disabled on stable/beta.

This test still tests that feature on an unconfigured crate is not processed. On stable/beta, processing feature causes an error (since feature is disabled), but on nightlies, it prevents gated feature errors.

@retep998
Copy link
Member

retep998 commented Jul 22, 2016

For example

#![cfg(feature = "nightly")]
#![feature(some_feature)]
#[test] fn foo() {
    // code which relies on some_feature
}

If the nightly feature isn't enabled, it will compile on the current stable. However it will not compile in the current beta (or at least whatever is on the current playground). The regression is that the stability check for features is done even when the entire crate is disabled with a cfg.

So the correct test would need to make sure that this example would still compile in a stable environment where features are disabled. And compile-fail is supposed to make sure that the code fails to compile, right? Or am I crazy and it actually tests to make sure that it does compile?

@jseyfried
Copy link
Contributor Author

@retep998 I know, this fixes that regression. The test I added is equivalent since features are stability checked when they are processed, so testing that features are not processed also tests that they are not stability checked.

If you know of a more direct way to test this, let me know -- I'm not sure how to add a test with stable/beta feature stability checking.

@eddyb
Copy link
Member

eddyb commented Jul 22, 2016

// compile-flags:--test
// rustc-env:RUSTC_BOOTSTRAP_KEY=

#![cfg(feature="does-not-exist")]
#![feature(iter_arith_traits)]

// nothing below actually matters, but I added it anyway
#[test]
fn dummy() {
    let () = "this should not reach type-checking";
    panic!("this should not run");
}

At least when building beta/stable, this test will require no feature's to be observed, in order to compile.
Although the feature being used should be something we know we aren't going to ever stabilize (rustc_attrs?).

@retep998
Copy link
Member

retep998 commented Jul 22, 2016

You could add a #![deny(unused_features)] to make sure it is tested on nightly as well, since currently on nightly it passes with a warning about an unused feature.

@jseyfried
Copy link
Contributor Author

@eddyb @retep998 I amended the regression test with your suggestions.

@eddyb
Copy link
Member

eddyb commented Jul 22, 2016

LGTM. cc @alexcrichton Is this the best we can do about testing?

@alexcrichton
Copy link
Member

I don't have too many opinions, so for me if it works it works!

// except according to those terms.

// compile-flags:--test
// rustc-env:RUSTC_BOOTSTRAP_KEY=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also need (found by Travis):

// ignore-pretty : (#23623) problems when  ending with // comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@nrc
Copy link
Member

nrc commented Jul 25, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 25, 2016

📌 Commit 64d36cc has been approved by nrc

@eddyb eddyb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 25, 2016
@bors
Copy link
Contributor

bors commented Jul 25, 2016

⌛ Testing commit 64d36cc with merge bc81a3f...

@bors
Copy link
Contributor

bors commented Jul 25, 2016

💔 Test failed - auto-win-gnu-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Sun, Jul 24, 2016 at 11:24 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/5057


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34969 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95OpRVtuUcZ7tagwxEz2XH4p5ieWaks5qZFaLgaJpZM4JSTZy
.

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Jul 26, 2016

⌛ Testing commit 64d36cc with merge 362d04d...

@bors
Copy link
Contributor

bors commented Jul 26, 2016

💔 Test failed - auto-win-msvc-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jul 26, 2016 at 3:36 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt/builds/5074


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34969 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95CIJfzLkYEJD09uJsE5m2uR6CzoAks5qZeMcgaJpZM4JSTZy
.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 28, 2016
Avoid processing `feature`s on unconfigured crates

Fixes rust-lang#34932, a regression caused by rust-lang#34272.
r? @nrc
bors added a commit that referenced this pull request Jul 28, 2016
Rollup of 7 pull requests

- Successful merges: #34951, #34963, #34969, #35013, #35037, #35040, #35058
- Failed merges:
@bors bors merged commit 64d36cc into rust-lang:master Jul 28, 2016
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 28, 2016
@nikomatsakis
Copy link
Contributor

Accepting for backport: regression, small.

@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 2, 2016
@jseyfried jseyfried deleted the fix_cfg_feature branch October 16, 2016 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants