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

Macro-generated macro hygiene change #46478

Closed
dtolnay opened this Issue Dec 3, 2017 · 22 comments

Comments

Projects
None yet
@dtolnay
Member

dtolnay commented Dec 3, 2017

The following procedural macro works with nightly-2017-12-02 but fails with nightly-2017-12-03. I don't have an intuition for which is the correct behavior but just wanted to make sure this is intentional.

bb42071...f9b0897. #46343 seems relevant and is in the right range so cc @jseyfried and @nrc.

This is relevant to dtolnay/proc-macro-hack#15.

#!/bin/bash

set -x

cargo new mac
echo >>mac/Cargo.toml '
[lib]
name = "mac"
proc-macro = true
'
echo >mac/src/lib.rs '
extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Hygiene, attributes(hygiene))]
pub fn hygiene(_: TokenStream) -> TokenStream {
    let macro_rules = "
        macro_rules! hygiene {
            () => { x }
        }
    ";
    macro_rules.parse().unwrap()
}
'

cargo new repro --bin
echo >>repro/Cargo.toml 'mac = { path = "../mac" }'
echo >repro/src/main.rs '
#[macro_use] extern crate mac;
fn main() {
    let x = 0usize;

    #[derive(Hygiene)]
    #[hygiene(print = "x")]
    struct S;

    println!("{:?}", hygiene!());
}
'

cargo run --manifest-path repro/Cargo.toml
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Dec 7, 2017

Contributor

Seems likely that the behavior was intentional, given that the PR is entitled "Fix hygiene bug". But I'd like to know more definitively.

#46489 <-- possible dup?

Contributor

nikomatsakis commented Dec 7, 2017

Seems likely that the behavior was intentional, given that the PR is entitled "Fix hygiene bug". But I'd like to know more definitively.

#46489 <-- possible dup?

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Dec 7, 2017

Member

I would not expect this to work. Creating the macro_rules macro should be fine because derive is unhygienic. However, when hygiene (the macro_rules macro) expands it creates an x which has the hygiene context of that macro's definition, so unrelated to the declared x in main.

Member

nrc commented Dec 7, 2017

I would not expect this to work. Creating the macro_rules macro should be fine because derive is unhygienic. However, when hygiene (the macro_rules macro) expands it creates an x which has the hygiene context of that macro's definition, so unrelated to the declared x in main.

@nrc nrc closed this Dec 7, 2017

@nikomatsakis nikomatsakis reopened this Dec 7, 2017

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Dec 7, 2017

Member

I was wrong, I think. Since the macro_rules macro is inside main it should be able to see x. The attribute seems to have nothing to do with this example though.

Member

nrc commented Dec 7, 2017

I was wrong, I think. Since the macro_rules macro is inside main it should be able to see x. The attribute seems to have nothing to do with this example though.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Dec 7, 2017

Contributor

@nrc and I were chatting. Let me explain what is happening here for future reference. @dtolnay you can confirm the details. @dtolnay has some kind of horrible hack-y crate wherein it lets you fake a "procedural macro" by translating the definition to some sort of #[derive] that, when executed, winds up generating a struct and macro-rules pair side by side, sort of like this:

fn foo() { 
    let x = 10; 
    struct Bar; 
    macro_rules! foo { () => { x } }; 
    let y = foo!(); 
}

fn main() { }

That example works, but something else must be happening around derive to make this example not work. I'd like to better understand what that something is!

(I'd also like to know @dtolnay how many crates are affected by this regression, directly/indirectly.)

Contributor

nikomatsakis commented Dec 7, 2017

@nrc and I were chatting. Let me explain what is happening here for future reference. @dtolnay you can confirm the details. @dtolnay has some kind of horrible hack-y crate wherein it lets you fake a "procedural macro" by translating the definition to some sort of #[derive] that, when executed, winds up generating a struct and macro-rules pair side by side, sort of like this:

fn foo() { 
    let x = 10; 
    struct Bar; 
    macro_rules! foo { () => { x } }; 
    let y = foo!(); 
}

fn main() { }

That example works, but something else must be happening around derive to make this example not work. I'd like to better understand what that something is!

(I'd also like to know @dtolnay how many crates are affected by this regression, directly/indirectly.)

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Dec 7, 2017

Member

You are correct about how proc-macro-hack works (if you strike the word "horrible").

I looked through the crates on crates.io that would be broken by this. I believe they are:

I think all the other crates that depend on proc-macro-hack are only passing in literals, as in ip!("127.0.0.1").

Member

dtolnay commented Dec 7, 2017

You are correct about how proc-macro-hack works (if you strike the word "horrible").

I looked through the crates on crates.io that would be broken by this. I believe they are:

I think all the other crates that depend on proc-macro-hack are only passing in literals, as in ip!("127.0.0.1").

@jseyfried

This comment has been minimized.

Show comment
Hide comment
@jseyfried

jseyfried Dec 7, 2017

Contributor

@nikomatsakis

That example works, but something else must be happening around derive to make this example not work. I'd like to better understand what that something is!

The reason why this doesn't work is because macros 1.1 isn't totally unhygienic -- it's just mostly unhygienic the same way macro_rules! is mostly unhygienic.

This example fails for the same reason that the following fails (and has failed for years):

macro_rules! hygiene_outer {
    () => {
        macro_rules! hygiene {
            () => { x }
        }
    }
}

fn main() {
    let x = 0usize;
    hygiene_outer!();
    println!("{:?}", hygiene!());
}
Contributor

jseyfried commented Dec 7, 2017

@nikomatsakis

That example works, but something else must be happening around derive to make this example not work. I'd like to better understand what that something is!

The reason why this doesn't work is because macros 1.1 isn't totally unhygienic -- it's just mostly unhygienic the same way macro_rules! is mostly unhygienic.

This example fails for the same reason that the following fails (and has failed for years):

macro_rules! hygiene_outer {
    () => {
        macro_rules! hygiene {
            () => { x }
        }
    }
}

fn main() {
    let x = 0usize;
    hygiene_outer!();
    println!("{:?}", hygiene!());
}
@jseyfried

This comment has been minimized.

Show comment
Hide comment
@jseyfried

jseyfried Dec 8, 2017

Contributor

That being said, I could make macros 1.1 fully unhygienic to avoid breakage here.

The main reason they have macro_rules!-hygiene is because we want the expanded tokens to show up in diagnostics as from the macro, and using macro_rules! hygiene was the simplest way to implement this.

Once it is possible to have a span that shows up as from a macro yet is fully unhygienic, it would be easy to make macros 1.1 expansions fully unhygienic. Since macros 1.1 can only be used on items, I believe this can only make a difference when expanding into a macro_rules!.

Contributor

jseyfried commented Dec 8, 2017

That being said, I could make macros 1.1 fully unhygienic to avoid breakage here.

The main reason they have macro_rules!-hygiene is because we want the expanded tokens to show up in diagnostics as from the macro, and using macro_rules! hygiene was the simplest way to implement this.

Once it is possible to have a span that shows up as from a macro yet is fully unhygienic, it would be easy to make macros 1.1 expansions fully unhygienic. Since macros 1.1 can only be used on items, I believe this can only make a difference when expanding into a macro_rules!.

@xfix

This comment has been minimized.

Show comment
Hide comment
@xfix

xfix Dec 8, 2017

Contributor

@dtolnay array-macro no longer uses proc-macro-hack. An older version did, but the dependency did get removed, and array-macro-internal crate exists only for versions 0.1.0 and 0.1.1, it's no longer used. Not even version 0.1.2 uses it.

Thanks for notifying me about this issue however 👍.

Contributor

xfix commented Dec 8, 2017

@dtolnay array-macro no longer uses proc-macro-hack. An older version did, but the dependency did get removed, and array-macro-internal crate exists only for versions 0.1.0 and 0.1.1, it's no longer used. Not even version 0.1.2 uses it.

Thanks for notifying me about this issue however 👍.

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Dec 9, 2017

Member

I find @jseyfried's example compelling in #46478 (comment). I don't necessarily think we should feel compelled to respect backward compatibility here if the change that broke this is a more correct and consistent design than the old one. Feel free to close this issue.

Member

dtolnay commented Dec 9, 2017

I find @jseyfried's example compelling in #46478 (comment). I don't necessarily think we should feel compelled to respect backward compatibility here if the change that broke this is a more correct and consistent design than the old one. Feel free to close this issue.

@xerofoify

This comment has been minimized.

Show comment
Hide comment
@xerofoify

xerofoify Dec 10, 2017

I disagree. I am new here but shouldn't unless this is a widely known and used feature of rust support it but warn the user. Can't we allow this to be the default but if the user wants the better version force it into there build scripts for instance? @jseyfried you state to want to make it work both ways is it possible to warn users now about it and when it does not break anymore not do so?
Breaking something just because it's better could kill legacy code so I would warn users about it at least. If @jseyfried or others want my help with it that's fine, I will new a little help in terms of answering my questions if any.

xerofoify commented Dec 10, 2017

I disagree. I am new here but shouldn't unless this is a widely known and used feature of rust support it but warn the user. Can't we allow this to be the default but if the user wants the better version force it into there build scripts for instance? @jseyfried you state to want to make it work both ways is it possible to warn users now about it and when it does not break anymore not do so?
Breaking something just because it's better could kill legacy code so I would warn users about it at least. If @jseyfried or others want my help with it that's fine, I will new a little help in terms of answering my questions if any.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 21, 2017

Member

@rfcbot fcp close

After reading the comment thread, at least a subset of the compiler team is inclined to close this (as not-a-bug), following the reasoning given by @jseyfried and @dtolnay . However, in recognition that opinions may differ, we've tagged the lang team as well and want to give them the chance to weigh in before we close this.

Member

pnkfelix commented Dec 21, 2017

@rfcbot fcp close

After reading the comment thread, at least a subset of the compiler team is inclined to close this (as not-a-bug), following the reasoning given by @jseyfried and @dtolnay . However, in recognition that opinions may differ, we've tagged the lang team as well and want to give them the chance to weigh in before we close this.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Dec 21, 2017

Team member @pnkfelix has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Dec 21, 2017

Team member @pnkfelix has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 21, 2017

Contributor

(...) is it possible to warn users now about it and when it does not break anymore not do so?
Breaking something just because it's better could kill legacy code so I would warn users about it at least.

My understanding is that that is the normal course of action for breaking changes, but we do reserve the right to break backwards compatibility for unsoundness bugs. That being said, could we use crater to see if anyone is actually relying on the previous behavior?

Contributor

estebank commented Dec 21, 2017

(...) is it possible to warn users now about it and when it does not break anymore not do so?
Breaking something just because it's better could kill legacy code so I would warn users about it at least.

My understanding is that that is the normal course of action for breaking changes, but we do reserve the right to break backwards compatibility for unsoundness bugs. That being said, could we use crater to see if anyone is actually relying on the previous behavior?

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Dec 21, 2017

Member

@rfcbot reviewed

Member

cramertj commented Dec 21, 2017

@rfcbot reviewed

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 4, 2018

Contributor

@dtolnay

You are correct about how proc-macro-hack works (if you strike the word "horrible").

Indeed. No offense meant, I intended "horrible" in the nicest way possible. =)

Contributor

nikomatsakis commented Jan 4, 2018

@dtolnay

You are correct about how proc-macro-hack works (if you strike the word "horrible").

Indeed. No offense meant, I intended "horrible" in the nicest way possible. =)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 11, 2018

Contributor

@rfcbot reviewed

OK, sorry for the delay. I was hesitant. Like @dtolnay, I find @jseyfriend's example compelling: if what we are doing is aligning the behavior with macro_rules, as opposed to some other unspecified form of hygiene, that has the feeling of "bug fix" to me. And I don't think the impact on the crates ecosystem is particularly large.

That said, I do have some mild concern: @jseyfried, are we going to get stuck with macros 1.1 behaving in some kind of "emulation mode" in the future? It's worth gaming out the plan here in a bit more detail.

Contributor

nikomatsakis commented Jan 11, 2018

@rfcbot reviewed

OK, sorry for the delay. I was hesitant. Like @dtolnay, I find @jseyfriend's example compelling: if what we are doing is aligning the behavior with macro_rules, as opposed to some other unspecified form of hygiene, that has the feeling of "bug fix" to me. And I don't think the impact on the crates ecosystem is particularly large.

That said, I do have some mild concern: @jseyfried, are we going to get stuck with macros 1.1 behaving in some kind of "emulation mode" in the future? It's worth gaming out the plan here in a bit more detail.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jan 26, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Jan 26, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo Jan 30, 2018

Contributor

That would break my tql crate, so I'm wondering if it's going to be fixed for next stable version or not (I cannot figure that out from the last comment by @nikomatsakis).
Thanks.

Contributor

antoyo commented Jan 30, 2018

That would break my tql crate, so I'm wondering if it's going to be fixed for next stable version or not (I cannot figure that out from the last comment by @nikomatsakis).
Thanks.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 30, 2018

Contributor

@antoyo I believe the consensus is that the nightly behavior is the correct behavior -- which I guess means your crate would break? I see that it is sort of a "dual mode" crate, working both on stable and on nightly?

Contributor

nikomatsakis commented Jan 30, 2018

@antoyo I believe the consensus is that the nightly behavior is the correct behavior -- which I guess means your crate would break? I see that it is sort of a "dual mode" crate, working both on stable and on nightly?

@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo Jan 30, 2018

Contributor

Indeed, it uses the proc-macro hack to work on stable.
Isn't it a bit strange to have hygiene on stable (well, currently beta), without any access to the Span (which is unstable)?

Contributor

antoyo commented Jan 30, 2018

Indeed, it uses the proc-macro hack to work on stable.
Isn't it a bit strange to have hygiene on stable (well, currently beta), without any access to the Span (which is unstable)?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 30, 2018

Contributor

My impression was that there wasn't hygiene on stable per se, but rather that Macros 1.1 was roughly matching the macro_rules behavior.

Contributor

nikomatsakis commented Jan 30, 2018

My impression was that there wasn't hygiene on stable per se, but rather that Macros 1.1 was roughly matching the macro_rules behavior.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Feb 5, 2018

The final comment period is now complete.

rfcbot commented Feb 5, 2018

The final comment period is now complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment