TokenStream::parse does not resolve with Span::call_site()

[golddranks]

Updated description

The following procedural macro:

#![feature(proc_macro)]

extern crate proc_macro;

use proc_macro::*;

#[proc_macro]
pub fn foo(input: TokenStream) -> TokenStream {
    let mut body = Vec::<TokenTree>::new();

    // input = `let a = 2;`
    body.extend(input.into_iter());

    // manually add `let b = a;`
    body.push(Term::new("let", Span::call_site()).into());
    body.push(Term::new("b", Span::call_site()).into());
    body.push(Op::new('=', Spacing::Alone).into());
    body.push(Term::new("a", Span::call_site()).into());
    body.push(Op::new(';', Spacing::Alone).into());

    // parse and add `let c = b;`
    body.extend("let c = b;".parse::<TokenStream>().unwrap().into_iter());

    let mut ret = Vec::<TokenTree>::new();
    ret.push(Term::new("fn", Span::call_site()).into());
    ret.push(Term::new("main", Span::call_site()).into());
    ret.push(Group::new(Delimiter::Parenthesis, TokenStream::empty()).into());
    ret.push(Group::new(Delimiter::Brace, body.into_iter().collect()).into());
    ret.into_iter().collect()
}

fails when invoked as:

#![feature(proc_macro)]

extern crate foo;

foo::foo! {
    let a = 3;
}

with the error message:

error[E0425]: cannot find value `b` in this scope
 --> src/main.rs:6:1
  |
6 | / foo::foo! {
7 | |     let a = 3;
8 | | }
  | |_^ not found in this scope

error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.

Original description

Linking the futures-await crate and a crate that useserror-chain-derive together breaks the build. Here's a simple example: https://github.com/golddranks/test_futures_await_error_chain_derive Either of the two dependencies (futures-await and dotenv, which uses error-chain-derive) builds on its own (using the latest nightly), but together they refuse to build.

This seems to have something to do with the defsite hygiene: error-chain-derive is using derive macros 1.1, and the syntax for using it is as follows: (from https://github.com/purpliminal/rust-dotenv/blob/master/src/errors.rs )

#[derive(Debug, ErrorChain)]
#[cfg_attr(not(feature = "backtrace"), error_chain(backtrace = "false"))]
pub enum ErrorKind {
    // generic error string, required by derive_error_chain
    Msg(String),
    #[error_chain(custom)]
    #[error_chain(description = r#"|_| "Parsing Error""#)]
    #[error_chain(display = r#"|l| write!(f, "Error parsing line: '{}'", l)"#)]
    LineParse(String),
    #[error_chain(foreign)]
    Io(::std::io::Error),
    #[error_chain(foreign)]
    EnvVar(::std::env::VarError),
}

|l| write!(f, "Error parsing line: '{}'", l) breaks, because it can't find f: cannot find value f in this scope

It seems like just the presence of futures-await somehow affects how the Derive macros 1.1 hygiene works; however, the derive macros are stable, so this is an undesirable thing.

[golddranks]

Looking at the symptoms, likely to be the same bug as #50061

[golddranks]

This still happens on the latest nightly (79252ff4e 2018-04-29), even after the patches #50152 and alexcrichton@e934873 landed.

[golddranks]

Ping @petrochenkov @alexcrichton

[alexcrichton]

I've updated the OP and issue title to reflect the underlying issue, minimized at https://gist.github.com/alexcrichton/6b4c56549c6732e697eb0aa9986bf398.

The tl;dr; is that a TokenStream created through TokenStream::from_str (aka parsing code) cannot refer to tokens with the span Span::call_site() (nor tokens input to the macro itself). This means that for whatever reason the hygienic context applied to tokens created through parsing strings is different than that of Span::call_site, exposing "oddness" of hygiene.

@petrochenkov or @nrc, do y'all know where this might be occurring?

[petrochenkov]

@alexcrichton
A guess: this may be a regression from changes in src/libsyntax/ext/quote.rs in #49154 (DUMMY_SP -> ident.span for ast::Ident and ast::Lifetime).
Could you test whether reverting changes in those two lines fixes the regression?
If it doesn't, then I'll try to investigate further sometime during this week.

[alexcrichton]

Sure yeah, I will test out that hypothesis tomorrow morning!

[alexcrichton]

Ah unfortunately this diff on 5a662bf produced the same error:

$ ./build/x86_64-unknown-linux-gnu/stage2/bin/rustc foo.rs --crate-type proc-macro
$ ./build/x86_64-unknown-linux-gnu/stage2/bin/rustc bar.rs -L .
error[E0425]: cannot find value `b` in this scope
 --> bar.rs:5:1
  |
5 | / foo::foo! {
6 | |     let a = 3;
7 | | }
  | |_^ not found in this scope

error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.

[jan-hudec]

@alexcrichton,

The tl;dr; is that a TokenStream created through TokenStream::from_str (aka parsing code) cannot refer to tokens with the span Span::call_site() (nor tokens input to the macro itself). This means that for whatever reason the hygienic context applied to tokens created through parsing strings is different than that of Span::call_site, exposing "oddness" of hygiene.

I am probably missing something, but to me it sounds about right. I would expect manually parsed code to be its own context. That's the hygiene. Similar to literal code in template macros, except proc macros don't have definition site, so the context is simply fresh. To refer to the symbol from the call site, you can replace it with the Token from input or manually created with Span::call_site(), no?

[alexcrichton]

@jan-hudec I think you're right yeah, but the problem is that we've been advocating Macros 1.1 and 1.2 as "hygiene comes later, we're just copy/pasting code". That seems to not be the case and there's actual hygiene involved, which is unintended currently

[jan-hudec]

Except if parse::<TokenStream> is now made unhygienic, making it hygienic later won't be backward compatible. So it can't be as easy as just copy/pasting code.

[alexcrichton]

@jan-hudec oh it was always the intention that stringifcation loses all hygiene information. The upcoming proc_macro API is how we'll allow adding hygiene in the future (aka via Span)

[SergioBenitez]

I'm running into this on multiple projects: it's a show-stopper. I believe some form of this may also be causing SergioBenitez/Rocket#635.

One possible band-aid could be to add a set_span(&mut self, span: Span) method to TokenStream that sets all internal spans to span. Thus, as a work-around, one could do stream.set_span(Span::call_site());. I think we'll want such a method when hygiene is implemented in earnest, so it wouldn't hurt to have.

[SergioBenitez]

The following stream results in the same issue:

let code: &str = /* some Rust code with an identifier */;
let stream: proc_macro::TokenStream = code.parse().unwrap();
let stream: proc_macro::TokenStream = stream.into_iter()
    .map(|mut tree| { tree.set_span(Span::call_site()); tree })
    .collect();

Thus, the set_span method wouldn't help. This leads me to believe that the initial diagnosis by @alexcrichton either isn't the root cause of this bug or what I'm experiencing (most similar to #50061) is different.

In particular, the issue I have is that name resolution information is lost when a proc_macro emits a call to a declarative macro by going through a TokenStream. Full reproducible example is below.

In lib.rs:

#![feature(proc_macro)]

extern crate proc_macro;
extern crate proc_macro2;
extern crate syn;
#[macro_use] extern crate quote;

use syn::Ident;
use proc_macro::{Span, TokenStream};
use quote::Tokens;

fn create(ident: &Ident) -> Tokens {
    // // This works.
    // quote!(do_f!(#ident))

    // // This does as well.
    // let new_ident = Ident::from(ident.to_string());
    // quote!(do_f!(#new_ident))

    // // So does this.
    // quote!(do_f!(x))

    // // This does not.
    // let source = format!("do_f!({})", ident);
    // let expr: syn::Expr = syn::parse_str(&source).unwrap();
    // quote!(#expr)

    // // Neither does this.
    // let source = format!("do_f!({})", ident);
    // let stream: proc_macro2::TokenStream = source.parse().unwrap();
    // quote!(#stream)

    // Nor this.
    let source = format!("do_f!({})", ident);
    let stream: proc_macro2::TokenStream = source.parse().unwrap();
    quote_spanned!(Span::call_site() => #stream)
}

#[proc_macro_attribute]
pub fn demo(_args: TokenStream, _input: TokenStream) -> TokenStream {
    let ident = Ident::from("x");
    let expr = create(&ident);
    let tokens: Tokens = quote! {
        macro_rules! do_f {
            ($id:ident) => ($id + 1)
        }

        fn f(#ident: usize) -> usize {
            #expr
        }
    };

    tokens.into()
}

In main.rs:

#![feature(proc_macro)]

extern crate codegen;
use codegen::demo;

#[demo] struct X;

fn main() { }

Results in:

error[E0425]: cannot find value `x` in this scope
 --> src/main.rs:7:1
  |
7 | #[demo]
  | ^^^^^^^ not found in this scope

[petrochenkov]

FWIW, interaction between "modern" macros (macro, proc_macro, proc_macro_attribute, proc_macro_derive) and "legacy" macros (macro_rules!, builtin derive and dummy spans from roundtrip through a string as well) is the single most complex part of our macro system.
If it were only possible, I'd prefer to avoid stabilizing anything in this area.

I haven't looked into it yet and I suspect that right now only @jseyfried knows how it works in detail, and relying on work of one person that's not sanity checked by anyone else, and that never went through RFC or any other discussion (and in addition to that the person in question is not around anymore) is and almost guaranteed way to shoot the language in the foot.

[SergioBenitez]

@petrochenkov This appears to be unrelated to whether macro_rules! or macro is used. The following fails just the same:

#[proc_macro_attribute]
pub fn demo(_args: TokenStream, _input: TokenStream) -> TokenStream {
    let ident = Ident::from("x");
    let expr = create(&ident);
    let tokens: Tokens = quote! {
        macro do_f($id:ident) {
            $id + 1
        }

        fn f(#ident: usize) -> usize {
            #expr
        }
    };

    tokens.into()
}

Furthermore, this issue was introduced in a recent nightly, somewhere in the last few months. For instance, the author of SergioBenitez/Rocket#635 reports that their particular issue is resolved by falling back to nightly-2018-04-18. Falling back to that nightly does not, however, fix the example I give here.

[petrochenkov]

Ok, that's good.
(I still plan to investigate this issue soon, this weekend most likely.)

[SergioBenitez]

I did some more testing. I'm using proc-macro2 in my code, as seen in the example above. It turns out the issue only occurs (cc @alexcrichton, @dtolnay) if the nightly feature is enabled. Without it, everything works as expected. This is my temporary work-around.

[Arnavion]

Yes, because pm2 without the nightly feature stringifies the input and parses that. Only with the nightly feature does it use pm types directly.

Edit: Also note that even if rocket doesn't enable the feature, something else in the deps tree might. That's what's going on in the OP (futures-await enables the feature and breaks derive-error-chain).

[petrochenkov]

OK, here's a fix for the "Updated description" in the original comment:
petrochenkov@ecb73eb

The problem is that the pre-fix behavior was intended - tokens parsed from strings have different context from Span::call_site.
They have Span::macro_rules_def_site context if I'd have to give it a name - it means that identifiers produced by strings behave like identifiers produced by macro_rules.

This means that produced local variables are hygienic, but everything else is not. Moreover, each string is a different macro_rules so in addition to Span::call_site incompatibility you can't refer to local variables generated by one string from expression generated any another string.

Tokens parsed from strings had Span::call_site context originally (#40939 and before), but then they were changed in #43179 (cc @oli-obk) to fix macro backtraces.
I don't think that was a correct and desirable change - we should not pull legacy hygiene machinery into anything related to macros 2.0 (and macros 1.1/1.2 as subsets of macros 2.0) and should not introduce inconsistency between tokens manually generated with Span::call_site and parsed from strings. Even Macro 1.1 RFC specified that TokenStream::from_str should use "same expansion context as the derive attribute itself" (i.e. call-site).

My commit above makes tokens parsed from strings Span::call_site again, but breaks macro backtraces, edition hygiene (#50307) and it should also break macro detection for lints in clippy because Span::call_site is literally the span of the macro call expression, nothing in it tells that it was generated by a macro.
I do think that fixing name resolution is more important that breaking backtraces/clippy, but broken backtraces/clippy are still pretty bad.

---

(Implementation section)

A solution giving both Span::call_site name resolution and Span::def_site backtraces and macro detection for lints would requires a bit more implementation work.
I think we'd need a new Mark kind (MarkKind::Transparent or something) that would be stripped before doing resolution for local variables in the same way as MarkKind::Legacys are stripped before resolving other names.
So the new Mark would keep all the information about token belonging to a macro necessary for backtraces/edition-hygiene/clippy, but wouldn't affect name resolution in any way - name resolution would be performed as if the span was call-site.
We actually need to modify Span::call_site to return a span with this new mark instead of the call-site span literally, so it would affect both parsed and manually created tokens.

[alexcrichton]

Thanks for the investigation @petrochenkov! Do you want to open a PR with your fix? I'd agree as well yeah that name resolution comes before backtraces/clippy

[petrochenkov]

If I won't be able to implement MarkKind::Transparent (or something equivalent) quickly enough, I'll open a PR with that patch.

[alexcrichton]

Ok awesome, thanks @petrochenkov!

[petrochenkov]

Status update: I started working on MarkKind::Transparent and have more detailed understanding of how it should work now, but tomorrow I'm leaving on vacation so finishing this work is going to be postponed for 1-2 weeks.

[petrochenkov]

Some status update: I have an implementation for MarkKind::Transparent and it works by itself and interacts with "opaque"/"modern" macros (declarative macro or procedural Span::def_site()) correctly, but interactions with macro_rules are buggy.
In particular stuff like this:

macro_rules! m {
    () => {
        let_x_transparent!();
        let y = x;
    }
}

m!();

generates an "unresolved x" error while it shouldn't

So far it looks like I need to really dig into the core hygiene/resolution algorithms to fix this properly, perhaps by refactoring hygiene data in some way in which these opaque/transparent/semi-transparent interactions can be expressed more naturally than the current "syntax context transplantation" technique (see the large comment in fn apply_mark).

In the meantime I'll probably just sumbit a PR with the buggy version this weekend.

[petrochenkov]

#51762 is submitted.
(It has the "transparent vs macro_rules" issue mentioned in the previous comment fixed.)

[SergioBenitez]

Is this fully fixed? We're still seeing issues on SergioBenitez/Rocket#635, and I can also externally confirm that I'm seeing similar issues with other proc-macro/macro interactions.

[petrochenkov]

@SergioBenitez
The issue as described in #50050 (comment) is fully fixed and the the snippet from the issue description now works.
There can be other issues, of course, please file a separate ticket with minimized reproduction if something looks like a bug.