Fix macro hygiene regression

[jseyfried]

The regression was caused by #32923, which is currently in beta.

The following is an example of regressed code:

fn main() {
    let x = 0;
    macro_rules! foo { () => {
        println!("{}", x); // prints `0` on stable and after this PR, prints `1` on beta and nightly
    } }

    let x = 1;
    foo!();
}

For code to regress, the following is necessary (but not sufficient):

• There must be a local variable before a macro in a block, and the macro must use the variable.
• There must be a second local variable with the same name after the macro.
• The macro must be invoked in a statement position after the second local variable.

For example, if the let x = 0; from the breaking example were commented out, it would (correctly) not compile on beta/nightly. If the semicolon were removed from foo!();, it would (correctly) print 0 on beta and nightly.

r? @nrc

[jseyfried]

This is the simplest way I could come up with to fix the regression while keeping #32922, #31856, #10681, and #26223 fixed. While I am very confident that it is correct, we (understandably) might not want to backport this big of a change to beta.

In that case, I think we should consider reverting #32923 on beta to avoid regressing hygiene on stable. However, the hygiene bug that #32923 fixes, i.e.:

fn main() {
    let x = 0;
    macro_rules! foo { () => {
        let x = 1;
        println!("{}", x); // prints `0` on stable, should print `1`
    } }
    m!();
}

is arguably worse than the regression.

[nrc]

What is the motivation for removing all the unit tests? r+ other than that.

[jseyfried]

@nrc
Some tests were testing functionality that I removed (resolve_table_hashing_test, test_marksof, xorpush_test) and some tests were using functionality that I removed (e.g. run_renaming_test was using mtwt::marksof). Also, the signatures and semantics of many of the functions changed, so I'd have to almost completely rewrite the tests. Finally, the tests are difficult to read, and I haven't entirely grokked what they are doing.

I believe it would be a better use of time to write more run-pass and compile-fail tests like the ones I added in the last two commits. Perhaps if we had written more of these tests earlier, we would have caught #32922 and this regression sooner. Also, these tests will be far more useful for checking future changes to our hygiene implementation.

That being said, I can try to rewrite the unit tests if you think it's worth it.

[jseyfried]

Also, I'm working on dramatically simplifying the hygiene algorithm (without changing semantics) to make it feasible to expand macros in an arbitrary order (needed for macro modularization). I discovered this regression while testing my implementation. Most of the unit tests are not relevant to the simplified implementation.

EDIT: Finished the hygiene simplification, see PR #34570.

[jseyfried]

I added some more hygiene tests. r? @nrc

[nrc]

@bors: r+

[bors]

📌 Commit eef8485 has been approved by nrc

[bors]

⌛️ Testing commit eef8485 with merge 4960f2f...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

[alexcrichton]

cc @rust-lang/compiler

Thoughts on backporting? We hope to produce a new beta soon and would love to include this.

[nrc]

somewhat ambiguous about backporting. I think the patch is right, but it is pretty big for a backport, so I'd want to see it properly motivated, i.e., the regression is causing trouble, it's not just a technicality. If that is the case, then happy to backport.

[alexcrichton]

@jseyfried do you know if this was causing trouble in the wild?

[alexcrichton]

Note that we plan on building the stable 1.10 release tomorrow, so this is unlikely at this point to make a backport. If it's critical to include, though, we can push schedules back and make it work!

[jseyfried]

@alexcrichton No, as far as we know this has no impact in the wild, definitely not critical to include.

I still think it would be better to include it, but it's not a big deal -- not worth pushing back the release.

[alexcrichton]

Ok, thanks for the update @jseyfried! In that case I'm going to remove the beta-nominated tag from this, but again if anything comes up we can make it work.