Skip to content

Treat #[test] like #[cfg(test)] in non-test builds #34002

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

Merged
merged 3 commits into from
Jun 11, 2016

Conversation

jseyfried
Copy link
Contributor

This PR treats #[test] like #[cfg(test)] in non-test builds. In particular, like #[cfg(test)],

  • #[test] nodes are stripped during cfg processing, and
  • #[test] is disallowed on non-optional expressions.

Closes #33946.
r? @nrc

@jseyfried
Copy link
Contributor Author

cc @LeoTestard

@LeoTestard
Copy link
Contributor

👍

@LeoTestard
Copy link
Contributor

Could you add a test similar to the #33946 case?

@nrc nrc added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 1, 2016
@nrc
Copy link
Member

nrc commented Jun 1, 2016

I'd be against doing this (but not strongly so) - I like the orthogonal-ness of #[cfg(test)] vs #[test]. Probably wants a lang team decision though.

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 1, 2016

@nrc These are my reasons for supporting this:

  • It would allow us to use #[cfg(test)] macros in #[test]s (we can already use other #[cfg(test)] items in #[test]s). For example,
#[cfg(test)] fn f() {}
#[cfg(test)] macro_rules! m { () => {} }

#[test]
fn test() {
    f(); //< this is OK,
    m!(); //< but this is currently an error (on non-test builds).
}
  • It would allow us to enable features only for tests, for example
#![cfg_attr(test, feature(stmt_expr_attributes))]

#[test]
fn test() {
    let _ = #[attr] (); //< Currently, this is a gated feature error (on non-test builds).
}
  • If we expand an item, which requires resolving macro invocation paths, I think we should also resolve other paths. Since we can't backwards compatibly resolve all paths in #[test]s, I think we should not resolve any paths in #[test]s, meaning no expansion (on non-test builds).

@nikomatsakis
Copy link
Contributor

lang-team thinks this is good :)

@jseyfried jseyfried force-pushed the strip_tests_in_cfg branch from c18660a to 369a996 Compare June 3, 2016 13:01
@nrc
Copy link
Member

nrc commented Jun 7, 2016

We discussed this at the language meeting last week. The important point I missed earlier was that to some extent #[test] already has the behaviour of stripping the code away in non-test compiles, it just happens later in compilation, so all this PR does is bring that behaviour earlier and thus avoid some errors. It therefore seems like pure win.

@nrc
Copy link
Member

nrc commented Jun 7, 2016

@bors: r+

(Would be good to add a test for 33946, but not essential, keep r=me with the test too)

@bors
Copy link
Collaborator

bors commented Jun 7, 2016

📌 Commit 369a996 has been approved by nrc

@bors
Copy link
Collaborator

bors commented Jun 8, 2016

⌛ Testing commit 369a996 with merge 6af2e10...

@bors
Copy link
Collaborator

bors commented Jun 8, 2016

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

@jseyfried jseyfried force-pushed the strip_tests_in_cfg branch from 369a996 to 29c4b67 Compare June 11, 2016 03:14
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Collaborator

bors commented Jun 11, 2016

📌 Commit 29c4b67 has been approved by nrc

@bors
Copy link
Collaborator

bors commented Jun 11, 2016

⌛ Testing commit 29c4b67 with merge 945ba12...

bors added a commit that referenced this pull request Jun 11, 2016
Treat `#[test]` like `#[cfg(test)]` in non-test builds

This PR treats `#[test]` like `#[cfg(test)]` in non-test builds. In particular, like `#[cfg(test)]`,
 - `#[test]` nodes are stripped during `cfg` processing, and
 - `#[test]` is disallowed on non-optional expressions.

Closes #33946.
r? @nrc
@bors bors merged commit 29c4b67 into rust-lang:master Jun 11, 2016
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants