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

Proc macro hygiene regression #46489

Open
SimonSapin opened this issue Dec 4, 2017 · 20 comments
Open

Proc macro hygiene regression #46489

SimonSapin opened this issue Dec 4, 2017 · 20 comments
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

Something in the range bb42071...f9b0897, likely #46343 (CC @jseyfried), broke Servo.

We have a #[dom_struct] attribute implemented in a dom_struct crate like this:

#[proc_macro_attribute]
pub fn dom_struct(args: TokenStream, input: TokenStream) -> TokenStream {
    if !args.is_empty() {
        panic!("#[dom_struct] takes no arguments");
    }
    let attributes = quote! {
        #[derive(DenyPublicFields, DomObject, JSTraceable, MallocSizeOf)]
        #[must_root]
        #[repr(C)]
    };
    iter::once(attributes).chain(iter::once(input)).collect()
}

Each of the derives is defined in a respective crate. The script crate depends on all of these crates and uses #[dom_struct]. Some of the derives generate code that reference items defined in script. For example, #[derive(DomObject)] implements the script::dom::bindings::refector::DomObject trait.

Since rustc 1.24.0-nightly (f9b0897 2017-12-02), every use of #[dom_struct] fails with:

error[E0433]: failed to resolve. Could not find `js` in `{{root}}`
  --> components/script/dom/attr.rs:28:1
   |
28 | #[dom_struct]
   | ^^^^^^^^^^^^^ Could not find `js` in `{{root}}`

error[E0433]: failed to resolve. Could not find `dom` in `{{root}}`
  --> components/script/dom/attr.rs:28:1
   |
28 | #[dom_struct]
   | ^^^^^^^^^^^^^ Could not find `dom` in `{{root}}`

error[E0433]: failed to resolve. Could not find `js` in `{{root}}`
  --> components/script/dom/attr.rs:28:1
   |
28 | #[dom_struct]
   | ^^^^^^^^^^^^^ Could not find `js` in `{{root}}`

error[E0433]: failed to resolve. Could not find `malloc_size_of` in `{{root}}`
  --> components/script/dom/attr.rs:28:1
   |
28 | #[dom_struct]
   | ^^^^^^^^^^^^^ Could not find `malloc_size_of` in `{{root}}`

I suppose that these errors come from code generated by the derives, and that {{root}} refers to the root of the dom_struct crate where the #[derive(…)] tokens come from. Indeed, the js and dom are not an cannot be available there, they’re in the script crate which depends on dom_struct.

We can work around this by erasing hygiene data in the #[derive(…)] tokens:

--- a/components/dom_struct/lib.rs
+++ b/components/dom_struct/lib.rs
@@ -17,7 +17,9 @@ pub fn dom_struct(args: TokenStream, input: TokenStream) -> TokenStream {
     let attributes = quote! {
         #[derive(DenyPublicFields, DomObject, JSTraceable, MallocSizeOf)]
         #[must_root]
         #[repr(C)]
     };
+    let attributes = attributes.to_string().parse().unwrap();
     iter::once(attributes).chain(iter::once(input)).collect()
 }

… but this seems like a valid pattern that shouldn’t necessitate such hacks.

@Mark-Simulacrum Mark-Simulacrum added C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 4, 2017
@Mark-Simulacrum
Copy link
Member

This also broke perf.rlo (due to breaking servo).

@SimonSapin
Copy link
Contributor Author

rust-lang/rustc-perf#171 applies the work around to the servo copy in perf.rlo.

@nrc
Copy link
Member

nrc commented Dec 7, 2017

This seems like correct behaviour. The derive names must be in scope where the macro is defined, not where it is used. I don't recall how derives are looked up though, so I'm not sure if that mechanism works correctly with the proper hygiene here - we should re-open or open another issue if that doesn't work.

Stripping the hygiene info as in the work-around (which is actually setting all the hygiene info to the macro use site) is not recommended.

@nrc nrc closed this as completed Dec 7, 2017
@nrc
Copy link
Member

nrc commented Dec 7, 2017

Urgh, sorry, I misunderstood what is going on here - it seems that we're looking up and expanding the derives fine. Given that the derives (unhygienic) are being expanded inside a hygienic context, I'm not even sure where I would expect them to look for their names.

@nrc nrc reopened this Dec 7, 2017
@SimonSapin
Copy link
Contributor Author

is not recommended

I do realize this is a hack that I’d rather was not needed, but this is the only way I’ve found to make this existing code compile with a newer compiler, which unblocks unrelated work.

@victorporof
Copy link

This broke rsx. Latest nightly that works is from Dec 1.

This doesn't work anymore:

let foo = 42;
...
my_proc_macro! { foo }

@victorporof
Copy link

victorporof commented Dec 12, 2017

A more explicative use case for this in case of rsx would be the following:

let stylesheet = ...;
let node = rsx! {
  <view style={stylesheet.get(".root")}>
      Hello world!
  </view>
};

where rsx! is a procedural macro.

@bhuztez
Copy link

bhuztez commented Dec 29, 2017

This also breaks my code

I have a proc_macro like this:

#![feature(proc_macro)]
#![crate_type = "proc-macro"]

extern crate proc_macro;

use proc_macro::{quote, TokenStream, TokenTree, TokenTreeIter, TokenNode, Delimiter};

fn parse(iter: &mut TokenTreeIter) -> TokenStream {
    iter.next();
    iter.next();
    if let Some(TokenTree{span: _, kind: TokenNode::Group(Delimiter::None, stream)}) = iter.next() {
        quote!($stream)
    } else {
        TokenStream::empty()
    }
}

#[proc_macro]
pub fn my_macro(stream: TokenStream) -> TokenStream {
    parse(&mut stream.into_iter())
}

now I got error[E0425]: cannot find value x in this scope

#![feature(proc_macro)]
extern crate my_macro;

pub use my_macro::my_macro;

macro_rules! m {
    ($($arg:expr),*) => (
        $crate::my_macro!($crate $(, $arg)*)
    )
}

fn main() {
    let x = 1usize;
    let y = m!(&x);
    println!("{}", y);
}

@parched
Copy link
Contributor

parched commented Jan 5, 2018

I think I'm hitting this too, here's the simplest repro I could do

#![feature(proc_macro)]
extern crate proc_macro;
use proc_macro::TokenStream;
use std::str::FromStr;

#[proc_macro]
pub fn add(token_stream: TokenStream) -> TokenStream {
    TokenStream::from_str(&token_stream.to_string().replace(",", "+")).unwrap()
}
#![feature(proc_macro)]
extern crate add;
use add::add;

#[test]
fn test_add() {
    let a = 1;
    let b = 2;
    let c = add!(a, b);
    assert_eq!(3, c);
}
error[E0425]: cannot find value `b` in this scope
  --> tests/simple.rs:11:13
   |
11 |     let c = add!(a, b);
   |             ^^^^^^^^^^ not found in this scope

@semtexzv
Copy link

Also hitting this error in my tests of proc_macro. Example :
https://gist.github.com/semtexzv/77facc8bf206b1d585214cea65757b1f

@M-Adoo
Copy link

M-Adoo commented Jan 17, 2018

I m hinting this too , same like with @parched demo code.

@jseyfried
Copy link
Contributor

I'm working on a PR to fix this now.

MichaelOultram added a commit to MichaelOultram/Auto_Parallelise that referenced this issue Mar 5, 2018
…into playground so that a clean compile can be achieved. In response to this bug in the nightly compiler: rust-lang/rust#46489
@rushmorem
Copy link

Is there any workaround for this? One of my libraries is depending on proc-macro-hack which is now broken.

@SimonSapin
Copy link
Contributor Author

@rushmorem In the specific code originally in this issue, I’ve worked around by replacing let attributes = quote! {…}; with let attributes = quote! {…}.to_string().parse().unwrap();. This works around hygiene by "erasing" the span information from these tokens.

@rushmorem
Copy link

@SimonSapin Thank you. I will try that.

@rail44
Copy link

rail44 commented Apr 15, 2018

I wrote function to make macro can use call_site variable, like

fn into_call_site_span(stream: TokenStream) -> TokenStream {
    let call_site_span = Span::call_site();
    let iter = stream.into_iter().map(|mut tree| {
        match tree {
            TokenTree::Group(g) => {
                TokenTree::Group(Group::new(g.delimiter(), into_call_site_span(g.stream())))
            }
            _ => {
                tree.set_span(call_site_span);
                tree
            }
        }
    });
    TokenStream::from_iter(iter)
}

@jeremydavis519
Copy link

@rail44 That's exactly the workaround I needed! Do you mind if I use it?

@rail44
Copy link

rail44 commented May 10, 2018

@jeremydavis519 Of course.

@joshlf
Copy link
Contributor

joshlf commented Feb 15, 2019

Still getting this issue with feature(proc_macro_hygiene); I assume that's expected behavior? The code in question:

Proc macro
#[proc_macro]
pub fn ip(input: TokenStream) -> TokenStream {
    // format and remove all spaces, or else IP parsing will fail
    let s = format!("{}", input).replace(" ", "");
    match ip_helper(&s) {
        Ok(stream) => stream,
        Err(_) => panic!("invalid IP address or subnet: {}", s),
    }
}

fn ip_helper(s: &str) -> Result<TokenStream, ()> {
    Ok(match s.parse() {
        Ok(IpAddr::V4(v4)) => {
            let octets = v4.octets();
            quote!(::ip::Ipv4Addr::new([#(#octets),*])).into()
        }
        Ok(IpAddr::V6(v6)) => {
            let octets = v6.octets();
            quote!(::ip::Ipv6Addr::new([#(#octets),*])).into()
        }
        Err(_) => {
            // try to parse as a subnet before returning error
            if !s.contains("/") {
                return Err(());
            }
            let parts: Vec<&str> = s.split("/").collect();
            if parts.len() != 2 {
                return Err(());
            }
            let ip: IpAddr = parts[0].parse().map_err(|_| ())?;
            let prefix: u8 = parts[1].parse().map_err(|_| ())?;
            match ip {
                IpAddr::V4(v4) => {
                    if prefix > 32 {
                        return Err(());
                    }
                    let octets = v4.octets();
                    quote!(::ip::Subnet::new(::ip::Ipv4Addr::new([#(#octets),*]), #prefix)).into()
                }
                IpAddr::V6(v6) => {
                    if prefix > 128 {
                        return Err(());
                    }
                    let octets = v6.octets();
                    quote!(::ip::Subnet::new(::ip::Ipv6Addr::new([#(#octets),*]), #prefix)).into()
                }
            }
        }
    })
}

Calling code:

pub const GATEWAY_ADDR: Ipv4Addr = ip!(10.123.0.3);

Error:

error[E0433]: failed to resolve: could not find `ip` in `{{root}}`
   --> src/wire/testdata.rs:301:40
    |
301 |     pub const GATEWAY_ADDR: Ipv4Addr = ip!(10.123.0.3);
    | 

@Enselic Enselic added the A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) label Nov 19, 2023
@Enselic
Copy link
Member

Enselic commented Nov 19, 2023

Triage: I see no use of any nightly exclusive feature, so this seems to be a stable-stable regression. Labeling as such.

@Enselic Enselic added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests