Stability lint for nested macros

[elinorbgr]

Finishes the job of #17286.

Now the stability lint will successfully detect patterns such as:

first_macro!(second_macro!(deprecated_function()));

macro_rules! foo (
    ($e: expr) => (bar!($e))
)
foo!(deprected_function());

and

println!("{}", deprecated_function());

even with more levels of nesting, such as

println!("{}", foo!(bar!(deprecated_function())));

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

[aturon]

@vberger Thanks so much for doing this!

I don't know the macro internals well enough to do more than rubber-stamp this; I'm going to ask someone who does to review.

[aturon]

r? @pnkfelix

[pnkfelix]

hmm. do we use this sort of span comparison logic elsewhere in a significant manner?

I ask because I spent some time investigating our span calculations and found that they are not always sound

[elinorbgr]

@pnkfelix I must admit I assumed they where correct (as that's what is used to align the ^~~~~ in the warning message).

The only situations where I encountered some that where not reliable was when dealing with compiler built-ins, that's pretty much why I skip them.

I couldn't find any other way to check whether an expression actually was from the macro arguments or was created by the macro. I couldn't find informations about macro expansion anywhere else.
If you know where I could look to seek more reliable data, I'd be glad to use it.

[aturon]

@pnkfelix Ping. Would be good to move forward with this.

[aturon]

@huonw Do you know the relevant macro stuff enough to properly review this?

[huonw]

s/firt/fisrt/

[huonw]

Haha, whoops: first.

[huonw]

(Just checking my understanding.)

Would it be accurate to describe this as:

• traverse to the top-most* macro in the macro expansion stack,
• check if the expression is in the arguments of that macro (that is, checks that the span of the original expression is contained within the span of that top macro invocation)
• (attempt to) emit a stability warning if it is?

*I understand that it is checking the spans at each step, but it seems that overwrites skip for each layer.

---

If someone wishes to experiment with the macro stack to understand better, I used a lint like the following to check what was going on:

// span-lint.rs
#![feature(phase, plugin_registrar)]
#![crate_type = "dylib"]

extern crate syntax;
#[phase(plugin, link)] extern crate rustc;

use syntax::ast;
use rustc::plugin::Registry;
use rustc::lint::{Context, LintArray, LintPass};
#[plugin_registrar]
pub fn plugin_registrar(reg: &mut Registry) {
    reg.register_lint_pass(box SpanLint);
}

declare_lint! { SPAN_LINT, Warn, "..." }

struct SpanLint;

impl LintPass for SpanLint {
    fn get_lints(&self) -> LintArray {
        lint_array!(SPAN_LINT)
    }

    fn check_expr(&mut self, cx: &Context, e: &ast::Expr) {
        let mut sp = e.span;
        let mut stop = false;
        cx.tcx.sess.span_warn(sp, "starts here");
        while !stop {
            cx.tcx.sess.codemap().with_expn_info(sp.expn_id, |expn_info| {
                match expn_info {
                    None => stop = true,
                    Some(ref info) => {
                        sp = info.call_site;
                        cx.tcx.sess.span_note(sp, "continues");
                    }
                }
            })
        }
    }
}

#![feature(phase, macro_rules)]
#[phase(plugin)]
extern crate "span-lint" as _lint;

macro_rules! bar {
    ($e: expr) => { $e }
}

macro_rules! foo {
    ($e: expr) => { bar!($e) };
    () => { 1u }
}

fn main() {
    foo!(1u);
    foo!();
}

test-span-lint.rs:15:10: 15:12 warning: starts here
test-span-lint.rs:15     foo!(1u);
                              ^~
test-span-lint.rs:5:1: 7:2 note: in expansion of bar!
test-span-lint.rs:10:21: 10:29 note: expansion site
test-span-lint.rs:9:1: 12:2 note: in expansion of foo!
test-span-lint.rs:15:5: 15:14 note: expansion site
test-span-lint.rs:10:21: 10:29 note: continues
test-span-lint.rs:10     ($e: expr) => { bar!($e) };
                                         ^~~~~~~~
test-span-lint.rs:9:1: 12:2 note: in expansion of foo!
test-span-lint.rs:15:5: 15:14 note: expansion site
test-span-lint.rs:15:5: 15:14 note: continues
test-span-lint.rs:15     foo!(1u);
                         ^~~~~~~~~
test-span-lint.rs:11:13: 11:15 warning: starts here
test-span-lint.rs:11     () => { 1u }
                                 ^~
test-span-lint.rs:9:1: 12:2 note: in expansion of foo!
test-span-lint.rs:16:5: 16:12 note: expansion site
test-span-lint.rs:16:5: 16:12 note: continues
test-span-lint.rs:16     foo!();
                         ^~~~~~~

(I don't know how to disable the conventional stack trace printing, so just focus on the starts here and continues lines.)

[elinorbgr]

@huonw I just corrected including your comments.

And yes, this is exactly how it works. This structure of over-writing the is_internal (former skip) variable each turn was a mean to make the whole thing more compact (to avoid having two match { } blocks), but I can change it to something more explicit if preferred.

[elinorbgr]

@huonw @pnkfelix So finally, is it acceptable of use the spans to discriminate expressions from the arguments of a macro, or not ?

[pnkfelix]

assigning self to review. (I'm not sure I'd be comfortable with using the spans, though of course lints are subject to less stringent correctness conditions than e.g. type-check analyses...)

[huonw]

It's not obvious to me that there is a solution without using the spans; certainly not one as easy as using the spans. (Maybe I'm just not being imaginative enough...)

[aturon]

Would really like to get this merged. Should I just rubber-stamp it at this point? @huonw @pnkfelix

[huonw]

I'm personally happy with it (enough to r+), but @pnkfelix may have some desired tweaks.

[pnkfelix]

I'm going to review this today, in part because I want to learn how this technique (a la #17286 ) works well enough to apply it to #18102

[elinorbgr]

Oops, I lost my second commit (fixing deprecated calls in the libs) while rebasing. I'll make it back.

[elinorbgr]

It seems that all changes implied by that commit are no longer relevant. I'm thus good for review @pnkfelix .