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

feature_gate::MacroVisitor misses macro invocations nested in macro invocations #22234

Closed
pnkfelix opened this issue Feb 12, 2015 · 6 comments · Fixed by #22383
Closed

feature_gate::MacroVisitor misses macro invocations nested in macro invocations #22234

pnkfelix opened this issue Feb 12, 2015 · 6 comments · Fixed by #22383
Labels
A-syntaxext Area: Syntax extensions P-medium Medium priority
Milestone

Comments

@pnkfelix
Copy link
Member

Today, this program is correctly rejected:

fn main() {
    unsafe {
        let x = asm!("");
        println!("hi {:?}", x);
    }
}

due to a missing #![feature(asm)]

Unfortunately, this variant program is accepted by the compiler:

fn main() {
    unsafe {
        println!("hi {:?}", asm!(""));
    }
}

The reason (I think) is that the fn visit_mac in feature_gate::MacroVisitor needs to recursively invoke the visitor on the expression inputs to the macro.

@pnkfelix
Copy link
Member Author

(investigating; I thought this would be a simple matter of calling visit::walk_mac, but that implementation is empty! well, because that's the only sane thing to do since our macro invocations are just token trees)

@pnkfelix
Copy link
Member Author

also: Nominating, since this strikes me as a hole in our feature checking.

@pnkfelix
Copy link
Member Author

cc @cmr @brson

@pnkfelix pnkfelix added the A-syntaxext Area: Syntax extensions label Feb 12, 2015
@pnkfelix
Copy link
Member Author

(I imagine we could just walk the token tree looking for occurrences of ` '!'`` and then assume that might be a macro invocation. Where would that break down, hmm...)

@pnkfelix
Copy link
Member Author

Another potential approach to handling these scenarios could be to have the expansions inject a token that is effectively a no-op into the generated AST that marks it as being generated via asm! (or whatever other macro expansion we may want to gate), and have the post expansion feature-gate checking look for occurrences of that token. A "danger" there I suppose is that it is possible that another macro could cause the token to be dropped (i.e. a macro that discards some of its inputs), but I think we should be willing to live with that scenario.

@pnkfelix pnkfelix changed the title feature_gate::MacroVisitor misses arguments on macro invocations feature_gate::MacroVisitor misses macro invocations nested in macro invocations Feb 12, 2015
@pnkfelix pnkfelix added this to the 1.0 milestone Feb 12, 2015
@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Feb 12, 2015
@pnkfelix
Copy link
Member Author

1.0 polish issue, P-high.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Feb 12, 2015
pnkfelix added a commit to pnkfelix/rust that referenced this issue Feb 15, 2015
pnkfelix added a commit to pnkfelix/rust that referenced this issue Feb 16, 2015
The other cases: `concat_idents!`, `log_syntax!`, and `trace_macros!`,
(these macros, with `asm!`, are handled (eagerly) in feature_gate.rs).
Manishearth added a commit to Manishearth/rust that referenced this issue Feb 17, 2015
…-expansion, r=huonw

 Pass features along during expansion

Use the set of passed features to detect uses of feature-gated macros without the corresponding feature enabled.

Fix rust-lang#22234.

----

Also, the framework this add (passing along a reference to the features in the expansion context) is a necessary precursor for landing a properly feature-gated desugaring-based overloaded-`box` and placement-`in` (rust-lang#22181).

----

This is fixing a bug, but since there might be code out there that is unknowingly taking advantage of that bug, I feel obligated to mark this as a:

[breaking-change]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-syntaxext Area: Syntax extensions P-medium Medium priority
Projects
None yet
1 participant