Macro hygiene change breaks Firefox builds.

[emilio]

The following program with paste = "0.1.12" compiles on stable (rustc 1.46.0 (04488afe3 2020-08-24)), but doesn't on Nightly (rustc 1.48.0-nightly (0e2c1281e 2020-09-07)).

This is minimized from Firefox source code, which doesn't build anymore with Rust Nightly.

To be fair, I'm surprised it built in the first place...

#[macro_use] extern crate paste;

#[derive(Clone, Copy)]
struct Bar {
    member: i32,
}

impl Bar {
    fn get_member(&self, _: i32, _: i32) -> i32 {
        self.member
    }
}

macro_rules! foo {
    ($fn_name:ident $(, $arg:ident)*) => {
        paste::expr! {
            Bar::[<get_ $fn_name>](&self.bar $(, $arg)* ,callback)
        }
    };
}

struct Foo {
    bar: Bar,
}

impl Foo {
    fn member(&self, n: i32, callback: i32) -> i32 {
        foo!(member, n)
    }
}

fn main() {}

[emilio]

The Firefox source for reference: https://searchfox.org/mozilla-central/rev/76a83d0a218837ba6937d6a0fac51cb0008c2334/services/fxaccounts/rust-bridge/firefox-accounts-bridge/src/bridge.rs#26-38,111-113

[KamilaBorowska]

This also happens on 1.47.0-beta.2 (84b047b 2020-08-28) making it regression-from-stable-to-beta.

[Mark-Simulacrum]

cc @Aaron1011 @petrochenkov

[emilio]

I'm fixing this in Firefox in https://bugzilla.mozilla.org/show_bug.cgi?id=1663677. There's another macro-related regression that I'm trying to reduce and that seems much harder to work around, but I'll file that separately.

[Aaron1011]

This is intentional - before PR #73084, we were using re-parsed tokens for the call to paste::expr! due to the use of a metavariable. We now use the original tokens, which preserves the hygiene information on self and callback.

[emilio]

I'm ok with wontfix, as I noted in my report I was surprised that this compiled at all. But it seems odd to see breaking changes like that...

[emilio]

Chances are that #76482 is the same too (just with a bit of indirection because of syn), not sure.

[Aaron1011]

@emilio: The breaking changes are unfortunate, but are an unavoidable part of making progress towards fixing #43081. As you noted, the broken code should have never compiled in the first place due to the hygiene rules of macro_rules! macros.

Several Crater runs were performed before landing #73084. Unfortunately, Crater cannot test all Rust code - in this case, it looks like firefox-accounts-bridge could have been tested, but Crater did not know about it (it's on neither crates.io nor GitHub).

[emilio]

In the past, breaking changes at least caused warnings for a release or two, which allowed us to notice the brekage and fix in advance without breaking developer builds. Anyhow I'm ok with this being wontfix, it's just odd to have such breakage being introduced without notice.

[estebank]

@Aaron1011 would it be possible to downgrade catch this and emit a warning instead of error (shirt if cherry picking a revert?)? Regardless, this should be in the release notes. Can you make sure this is part of 1.47's public announcement?

[Aaron1011]

Unfortunately, it's not possible to emit a warning here - see #76482 (comment)

PR #73084 is tagged with relnotes, and is mentioned in #76101

However, I agree that we can do a better job of notifying crates about this kind of breakage. The next PR to cause this kind of breakage will be #75800, which just finished its Crater run. I have a few thoughts on how to make the transition smoother:

• Make some kind of announcement (blog post, forum post, etc) before landing it
• Create a 'breaking changes' Zulip stream (or maybe just a 'unwarnable-macro-breaking-chnges' stream) where we post announcements about this kind of change. This should help with crates that are not (or cannot be) tested by Crater, assuming that crate authors subscribe to it.

I'm open to any suggestions as to how we can improve this process. Unfortunately, the black-box nature of proc-macros means that the usual future-compatibility lint approach cannot work here.

[emilio]

Unfortunately, it's not possible to emit a warning here - see #76482 (comment)

@Aaron1011 I'm not sure I agree with that argument. While I agree that technically a proc macro can do whatever and have a gazillion side-effects, the vast majority of macros don't.

And if you're at the point where the proc macro has errored, which is the only case in which you'd run it twice, I don't see how running it twice could be worse than failing to build?

[Aaron1011]

@emilio: People have made assumptions about the behavior of proc-macros in the past (e.g. trying to store things in thread-locals), so I'm worried that re-running proc-macros without an explicit opt-in could just cause a different kind of breakage.

Another factor I overlooked is that there are two ways for proc-macros to fail: panicking, and emitting an 'error' TokenStream (e.g. containing a call to compile_error!). While we could attempt to detect calls to compile_error! that occur directly in the output TokenStream, we can't really do anything in the general case. A compilation error can occur at any expansion depth, and result from a combination of tokens for multiple macros expansions - in such a case, we can't determine which macro to retry.

It's possible that addressing the simple case of pancicking/compile_error! would be good enough to cover the majority of breakage. cc @petrochenkov

[spastorino]

This doesn't need prioritization so removing I-prioritize

[petrochenkov]

This is unambiguously a bugfix and producing transition warnings for this would be more on the "heroic efforts" territory, so I wouldn't personally do this from the cost / benefit point of view.

A public announce would be great though.

[spastorino]

This was discussed on today's compiler meeting

[Mark-Simulacrum]

This was shipped in 1.47, and included in release notes (IIRC). I don't think there's anything further here so let's go ahead and close.