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

ABIs are validated before cfg-ing #27873

Closed
huonw opened this issue Aug 17, 2015 · 3 comments · Fixed by #36482
Closed

ABIs are validated before cfg-ing #27873

huonw opened this issue Aug 17, 2015 · 3 comments · Fixed by #36482
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@huonw
Copy link
Member

huonw commented Aug 17, 2015

The ABI string for extern "..." is validated while being parsed, meaning one can get errors about in cfg'd off modules:

#[cfg(foo)]
mod foo {
    extern "foo" {}
}
<anon>:3:12: 3:17 error: illegal ABI: expected one of [cdecl, stdcall, fastcall, aapcs, win64, Rust, C, system, rust-intrinsic, rust-call], found `foo`
<anon>:3     extern "foo" {}
                    ^~~~~

(Compiled without --cfg foo.)

This isn't necessarily wrong, but it is weird and somewhat annoying if one needs it. (It can be worked-around by hiding the extern block behind a macro: macro_rules! annoying ( () => { extern "foo" {} }); annoying!().)

@brson brson added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 17, 2015
@nagisa
Copy link
Member

nagisa commented Aug 17, 2015

Any ideas where we could move the check into?

Any of borrowck, typeck, lints, privacy and resolve don’t exactly fit. I think trans thematically is the best place to have it, but it is a shame to have it error out so late for such an easy to detect and obvious error.

I’m personally fine keeping it a parse error, IMO there’s no reason to have cfg blocks with code that does not compile.

@huonw
Copy link
Member Author

huonw commented Aug 18, 2015

Any ideas where we could move the check into?

Any of borrowck, typeck, lints, privacy and resolve don’t exactly fit. I think trans thematically is the best place to have it, but it is a shame to have it error out so late for such an easy to detect and obvious error.

NB. we can add a special abick pass if necessary (i.e. there's no reason for us to compromise by putting the error in trans).

IMO there’s no reason to have cfg blocks with code that does not compile.

Yeah, of course. However, there are cases when the cfgd code does compile, e.g. I encountered this in #27169 where I added an ABI and had to use it in core inside a #[cfg(not(stage0))] module. Similarly, if we add ABIs in future, someone might have code designed to compile with Rust 1.0.0, but can take advantage of the newer ABI if it is available.

@cesarb
Copy link
Contributor

cesarb commented Aug 18, 2015

I’m personally fine keeping it a parse error, IMO there’s no reason to have cfg blocks with code that does not compile.

The cfg block might be to gate nightly-only code. The code within the gated block is compiled only when a cargo feature is enabled; the code outside it works both on stable and nightly.

I hit it myself; I'm playing with the SIMD work from @huonw's pull request, but I want to keep the crate working on stable. I worked around it by using a conditional include!(...).

bors added a commit that referenced this issue Sep 17, 2016
…ules, r=nrc

Avoid loading and parsing unconfigured non-inline modules.

For example, `#[cfg(any())] mod foo;` will always compile after this PR, even if `foo.rs` and `foo/mod.rs` do not exist or do not contain valid Rust.

Fixes #36478 and fixes #27873.

r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants