Invocation of feature-gated macros inside `#[cfg]`-guarded code is not checked.

[LeoTestard]

While reading libsyntax code, I came across this piece of code:

impl<'a, 'v> Visitor<'v> for MacroVisitor<'a> {
    fn visit_mac(&mut self, mac: &ast::Mac) {
        let path = &mac.node.path;
        let name = path.segments.last().unwrap().identifier.name.as_str();

        // Issue 22234: If you add a new case here, make sure to also
        // add code to catch the macro during or after expansion.
        //
        // We still keep this MacroVisitor (rather than *solely*
        // relying on catching cases during or after expansion) to
        // catch uses of these macros within conditionally-compiled
        // code, e.g. `#[cfg]`-guarded functions.

        // [...]
    }

   // [...]
}

The comment clearly states that this visitor (MacroVisitor, called by the driver using syntax::feature_gate::check_macros) is there only to check use of such macros inside #[cfg]-guarded code.

Except that this is not the case:

#[cfg(foo)]
fn pwet() { asm!() }

fn main() {}

This compiles on both stable and nightly. The reason for that is that the call to check_crate_macros occur after the call to strip_unconfigured_items. If we still want to catch those invocations, it should be the opposite, although this may have other implications (a comment above the call to strip_unconfigured_items states that this must come before anything else because of #[cfg_attr], I'm not sure if this is relevant for feature-gate checking).
Otherwise, if we want to allow them, then this whole piece of code is useless and should be removed.
I think a lot of crates use #[cfg] to differenciate between stable and nightly builds, especially crates using syntax extensions. For example, we might want to allow this:

#![cfg_attr(nightly, feature(asm))]

#[cfg(nightly)]      fn main() { unsafe { asm!("") } }
#[cfg(not(nightly))] fn main() {}

(I took asm!() as example here because it's one of the feature-gated macros but others might be more relevant I guess.)

(cc @pnkfelix: This is the weird thing I told you about earlier.)

[durka]

I don't think your code examples are problems. The whole point of #[cfg] is to stop code from being considered after parsing, and as you say one of the main uses is to choose between unstable code and stable alternatives. When the cfg'ed code lives beyond cfg stripping, as in

#[cfg(not(foo))]
fn pwet() { asm!() }

fn main() {}

the stability error is generated.

[LeoTestard]

Well, there is a comment in libsyntax that explicitely states that a whole piece of code is used only to find such invocations in #[cfg]-guarded code. If this is not wanted, then this code should probably be removed. :)

[pnkfelix]

Just to clarify, as I remember it, my original motivation was to catch uses of unstable things on other targets, like if you're on a unix system and have code with #[cfg(windows)] ...

This may have mattered more when we were first introducing the macro stability checks than it does now... (I remain pretty sure that this did work at some point...)

[pnkfelix]

(but as the others have noted above, the fact that this is used in practice to distinguish between nightly and stable builds is probably a sign that we cannot hope to actually put in this check pre-cfg stripping, at least not without making it some sort of hack that also inspects the particular cfg and skips some but not others...)

[LeoTestard]

Okay so I tried to remove this piece of code to see what would break, and some test do, but I think it is legit.

Basically, MacroVisitor checks:

• Use of feature-gated macros such as asm!, concat_idents!, but also featured-gated sugar such as box. Those are already checked during expansion and the only benefit of checking them first here is to catch them inside conditionalized code, which I think shouldn't be done
• Use of #[plugin]. Again, I really think we should NOT catch uses of this inside #[cfg]-flagged code since crates use this to write syntax extensions that use plugins on nightly and syntex on stable. (I do this in RustLex, for example, and I'm pretty sure I'm not the only one to do so.) So I suggest we move the check to the actual ‶expansion″ of #[plugin] instead.
• Use of reserved attributes, like #[rustc_*] or #[derive_*]. I see no specific reason why those should be checked before macro expansion, but we might want to check them even inside conditionalized code (note: this is not the case at the moment, since right now MacroVisitor comes after stripping of #[cfg], so fixing it for doing so might be a breaking change).
• Use of #[allow_internal_unstable]. This one is also checked during macro expansion, and I'm not sure wether it's important to catch it in conditionalized code (as above, it's not the case now).

However, for the third item, examples in the test suite are not catched by the second checking pass, I'm not sure why, maybe they are stripped in the meantime. I'll investigate on this.

Once this is fixed, I suggest we move checking of #[plugin] to during expansion and allow all of this to appear inside conditionalized code, then remove this pass.

[LeoTestard]

• Use of reserved attributes, like #[rustc_] or #[derive_]. I see no specific reason why those should be checked before macro expansion, but we might want to check them even inside conditionalized code (note: this is not the case at the moment, since right now MacroVisitor comes after stripping of #[cfg], so fixing it for doing so might be a breaking change).

Well, simply to catch use of such attributes on macro definitions themselves... I guess this can be checked during compilation of macros, just as it's the case for #[allow_internal_unstable] now.