Simplify the macro hygiene algorithm #34570

jseyfried · 2016-06-30T06:25:55Z

This PR removes renaming from the hygiene algorithm and treats differently marked identifiers as unequal.

This change makes the scope of identifiers in macro_rules! items empty. That is, identifiers in macro_rules! definitions do not inherit any semantics from the macro_rules!'s scope.

Since macro_rules! macros are items, the scope of their identifiers "should" be the same as that of other items; in particular, the scope should contain only items. Since all items are unhygienic today, this would mean the scope should be empty.

However, the scope of an identifier in a macro_rules! statement today is the scope that the identifier would have if it replaced the macro_rules! (excluding anything unhygienic, i.e. locals only).

To continue to support this, this PR tracks the scope of each macro_rules! and uses it in resolve to ensure that an identifier expanded from a macro_rules! gets a chance to resolve to the locals in the macro_rules!'s scope.

This PR is a pure refactoring. After this PR,

syntax::ext::expand is much simpler.
We can expand macros in any order without causing problems for hygiene (needed for macro modularization).
We can deprecate or remove today's macro_rules! scope easily.
Expansion performance improves by 25%, post-expansion memory usage decreases by ~5%.
Expanding a block is no longer quadratic in the number of let statements (fixes Macro expansion is quadratic in the number of let statements. #10607).

r? @nrc

petrochenkov · 2016-07-03T09:38:59Z

src/libsyntax/ast.rs

@@ -46,7 +45,7 @@ pub struct SyntaxContext(pub u32);
 /// An identifier contains a Name (index into the interner
 /// table) and a SyntaxContext to track renaming and
 /// macro expansion per Flatt et al., "Macros That Work Together"
-#[derive(Clone, Copy, Eq)]
+#[derive(Clone, Copy, PartialEq, Eq, Hash)]


So, operator == and hash for Ident did unhygienic comparisons and now they do hygienic comparisons.
I'm not sure what unintended consequences it may have.
I remember derived implementations of PartialEq were used in couple of places for unhygienic comparisons of larger parts of AST including identifiers as their parts (maybe even expressions?), now the meaning of those comparisons changed.
Have you looked at where == for Idents was previously used and how the meaning of that code changed?

We used to panic! when using == to compare two identifiers with different syntax contexts, so if we didn't panic! before then the behavior won't change.

This probably doesn't matter much because the comparisons that changed the meaning could only result in panic before, but still worth checking and maybe fixing later, it's probably not a good idea to compare AST pieces with derived PartialEq anyway.

Agreed -- I'll look into it but I don't think it blocks this PR.

petrochenkov · 2016-07-03T11:30:11Z

Could you add a test with >1 levels of macro_rules and "captured" local variables, something like

fn main() {
    let a = 10;
    macro_rules! m {
        () => (
            let b = 11;
            macro_rules! n {
                () => {
                    (a, b)
                }
            }
        )
    }
    m!();
    let c = n!();
}

petrochenkov · 2016-07-03T11:40:06Z

This is awesome. The last autumn I tried to figure out how this combination of expand and mtwt works in detail but gave up and now it turns out that most of that complexity exists to support one mis-feature!
I reviewed this, but I can't really be sure that b0c7064 correctly replaces the previous scheme, so I'll pass this responsibility to @nrc.

jseyfried · 2016-07-03T19:23:06Z

@petrochenkov The tests that I had written for this PR landed in #34374 -- I think you'll like them :)

nrc · 2016-07-04T03:34:40Z

Hi, I've been looking at this and I had some questions about it, I wanted to do some experimentation, but for some reason I couldn't get this branch to build. I think it is unrelated to the patch here and some weirdness thanks to LLVM (maybe due to an LLVM upgrade). Anyway, thanks for the PR and sorry for the delay in review, but I'm on it just as soon as I fix my build problem.

jseyfried · 2016-07-05T06:11:56Z

@nrc no problem, I've also been having trouble building rustc recently.

durka · 2016-07-06T04:12:16Z

I built this and verified that it works on unborrow, my personal hygiene stress test :)

petrochenkov · 2016-07-07T17:51:29Z

cc @jbclements just in case

jbclements · 2016-07-07T18:33:17Z

sigh... @nrc , any progress on sets of scopes? Honestly, the whole approach of doing macro expansion after into ASTs seems fundamentally awkward, if not completely broken.

bors · 2016-07-10T01:18:25Z

☔ The latest upstream changes (presumably #34365) made this pull request unmergeable. Please resolve the merge conflicts.

nrc · 2016-07-11T21:27:03Z

src/libsyntax/ext/mtwt.rs

@@ -171,15 +104,24 @@ pub fn outer_mark(ctxt: SyntaxContext) -> Mrk {
    })
 }

+/// If `ident` is macro expanded, return the source ident from the macro definition


What does "source ident" mean for a procedural macro?

Currently, this function returns None for idents expanded from procedural macros.

Ideally, I think it should return the unmarked ident along with a "null" macro definition mark (since procedural macros are not expanded into existence).

jseyfried · 2016-07-14T01:36:20Z

Rebased

nrc · 2016-07-14T09:08:34Z

@bors: r+

TBH, I'm still pretty surprised that this works, but I think I've convinced myself that it does (at least for macro_rules) - everything I threw at it worked as expected and any theoretical issue I could think of would not actually be an issue because of some aspect of Rust. I do worry that there might be problems with procedural macros, but I couldn't come up with anything, so lets see if something materlialises.

bors · 2016-07-14T09:08:36Z

📌 Commit 56c4ddf has been approved by nrc

bors · 2016-07-14T12:42:59Z

⌛ Testing commit 56c4ddf with merge df8d15b...

@nrc

Simplify the macro hygiene algorithm This PR removes renaming from the hygiene algorithm and treats differently marked identifiers as unequal. This change makes the scope of identifiers in `macro_rules!` items empty. That is, identifiers in `macro_rules!` definitions do not inherit any semantics from the `macro_rules!`'s scope. Since `macro_rules!` macros are items, the scope of their identifiers "should" be the same as that of other items; in particular, the scope should contain only items. Since all items are unhygienic today, this would mean the scope should be empty. However, the scope of an identifier in a `macro_rules!` statement today is the scope that the identifier would have if it replaced the `macro_rules!` (excluding anything unhygienic, i.e. locals only). To continue to support this, this PR tracks the scope of each `macro_rules!` and uses it in `resolve` to ensure that an identifier expanded from a `macro_rules!` gets a chance to resolve to the locals in the `macro_rules!`'s scope. This PR is a pure refactoring. After this PR, - `syntax::ext::expand` is much simpler. - We can expand macros in any order without causing problems for hygiene (needed for macro modularization). - We can deprecate or remove today's `macro_rules!` scope easily. - Expansion performance improves by 25%, post-expansion memory usage decreases by ~5%. - Expanding a block is no longer quadratic in the number of `let` statements (fixes #10607). r? @nrc

bors · 2016-07-14T13:37:29Z

💔 Test failed - auto-win-msvc-64-cargotest

nrc · 2016-07-15T03:29:58Z

@bors: retry

@jseyfried failure looks like it might be genuine

jseyfried · 2016-07-15T04:28:24Z

@bors r-
Failure is legit -- macro-expanded labels should have a chance to resolve in the macro_rules!'s scope just like macro-expanded locals.
i.e. MacroDefinition rib should be added to label ribs as well as value ribs. Will fix soon.

jseyfried · 2016-07-15T05:27:38Z

@nrc Fixed and added a regression test.

eddyb · 2016-07-15T06:31:58Z

@bors r=nrc

bors · 2016-07-15T06:31:59Z

📌 Commit c1a6ff2 has been approved by nrc

bors · 2016-07-15T06:42:58Z

⌛ Testing commit c1a6ff2 with merge 1d99dec...

bors · 2016-07-15T10:06:57Z

💔 Test failed - auto-win-gnu-64-opt

jseyfried · 2016-07-15T10:15:14Z

@bors retry

bors · 2016-07-15T15:48:42Z

⌛ Testing commit c1a6ff2 with merge 4db1874...

@nrc

Simplify the macro hygiene algorithm This PR removes renaming from the hygiene algorithm and treats differently marked identifiers as unequal. This change makes the scope of identifiers in `macro_rules!` items empty. That is, identifiers in `macro_rules!` definitions do not inherit any semantics from the `macro_rules!`'s scope. Since `macro_rules!` macros are items, the scope of their identifiers "should" be the same as that of other items; in particular, the scope should contain only items. Since all items are unhygienic today, this would mean the scope should be empty. However, the scope of an identifier in a `macro_rules!` statement today is the scope that the identifier would have if it replaced the `macro_rules!` (excluding anything unhygienic, i.e. locals only). To continue to support this, this PR tracks the scope of each `macro_rules!` and uses it in `resolve` to ensure that an identifier expanded from a `macro_rules!` gets a chance to resolve to the locals in the `macro_rules!`'s scope. This PR is a pure refactoring. After this PR, - `syntax::ext::expand` is much simpler. - We can expand macros in any order without causing problems for hygiene (needed for macro modularization). - We can deprecate or remove today's `macro_rules!` scope easily. - Expansion performance improves by 25%, post-expansion memory usage decreases by ~5%. - Expanding a block is no longer quadratic in the number of `let` statements (fixes #10607). r? @nrc

bors · 2016-07-15T19:43:21Z

rust-highfive assigned nrc Jun 30, 2016

jseyfried mentioned this pull request Jun 30, 2016

Macro expansion is quadratic in the number of let statements. #10607

Closed

jseyfried force-pushed the no_rename branch 2 times, most recently from e683cf8 to 33afddf Compare June 30, 2016 09:09

jseyfried mentioned this pull request Jul 1, 2016

Fix macro hygiene regression #34374

Merged

petrochenkov reviewed Jul 3, 2016
View reviewed changes

jseyfried force-pushed the no_rename branch from 33afddf to 5db5316 Compare July 5, 2016 21:36

jseyfried force-pushed the no_rename branch from 5db5316 to 2877067 Compare July 8, 2016 05:20

nrc reviewed Jul 11, 2016
View reviewed changes

jseyfried added 5 commits July 14, 2016 01:30

Move node id assigning into resolve

195a27f

Remove IllegalCtxt

ca92404

Instead of renaming, treat differently marked identifiers as unequal

a15dfca

Implement macro_rules! placeholders and the macro scope map

0701571

Remove irrelevant tests

56c4ddf

jseyfried force-pushed the no_rename branch from 2877067 to 56c4ddf Compare July 14, 2016 01:34

jseyfried added 2 commits July 15, 2016 05:16

Include the MacroDefinition rib in the label ribs.

c83e31e

Add labels hygiene test

c1a6ff2

bors merged commit c1a6ff2 into rust-lang:master Jul 15, 2016

bors mentioned this pull request Jul 15, 2016

privacy: Substitute type aliases in private-in-public checker #34193

Merged

petrochenkov mentioned this pull request Jul 20, 2016

resolve: the relative order of items and statements in a block can be important #33118

Closed

jseyfried mentioned this pull request Aug 7, 2016

Extended expand.rs to support alternate expansion behaviours (eg. stepwise expansion) #34811

Merged

jseyfried deleted the no_rename branch October 16, 2016 09:31

TimNN mentioned this pull request Oct 22, 2016

ICE "break to unknown label" when while has a labeled break of itself in condition #37353

Closed

jseyfried mentioned this pull request Nov 4, 2016

Implement the loop_break_value feature. #37487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the macro hygiene algorithm #34570

Simplify the macro hygiene algorithm #34570

jseyfried commented Jun 30, 2016 •

edited

Loading

petrochenkov Jul 3, 2016

jseyfried Jul 3, 2016 •

edited

Loading

petrochenkov Jul 3, 2016

jseyfried Jul 3, 2016

petrochenkov commented Jul 3, 2016

petrochenkov commented Jul 3, 2016

jseyfried commented Jul 3, 2016 •

edited

Loading

nrc commented Jul 4, 2016

jseyfried commented Jul 5, 2016

durka commented Jul 6, 2016

petrochenkov commented Jul 7, 2016

jbclements commented Jul 7, 2016

bors commented Jul 10, 2016

nrc Jul 11, 2016

jseyfried Jul 11, 2016

jseyfried commented Jul 14, 2016

nrc commented Jul 14, 2016

bors commented Jul 14, 2016

bors commented Jul 14, 2016

bors commented Jul 14, 2016

nrc commented Jul 15, 2016

jseyfried commented Jul 15, 2016 •

edited

Loading

jseyfried commented Jul 15, 2016 •

edited

Loading

eddyb commented Jul 15, 2016

bors commented Jul 15, 2016

bors commented Jul 15, 2016

bors commented Jul 15, 2016

jseyfried commented Jul 15, 2016

bors commented Jul 15, 2016

bors commented Jul 15, 2016

Simplify the macro hygiene algorithm #34570

Simplify the macro hygiene algorithm #34570

Conversation

jseyfried commented Jun 30, 2016 • edited Loading

petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

jseyfried Jul 3, 2016 • edited Loading

Choose a reason for hiding this comment

petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

jseyfried Jul 3, 2016

Choose a reason for hiding this comment

petrochenkov commented Jul 3, 2016

petrochenkov commented Jul 3, 2016

jseyfried commented Jul 3, 2016 • edited Loading

nrc commented Jul 4, 2016

jseyfried commented Jul 5, 2016

durka commented Jul 6, 2016

petrochenkov commented Jul 7, 2016

jbclements commented Jul 7, 2016

bors commented Jul 10, 2016

nrc Jul 11, 2016

Choose a reason for hiding this comment

jseyfried Jul 11, 2016

Choose a reason for hiding this comment

jseyfried commented Jul 14, 2016

nrc commented Jul 14, 2016

bors commented Jul 14, 2016

bors commented Jul 14, 2016

bors commented Jul 14, 2016

nrc commented Jul 15, 2016

jseyfried commented Jul 15, 2016 • edited Loading

jseyfried commented Jul 15, 2016 • edited Loading

eddyb commented Jul 15, 2016

bors commented Jul 15, 2016

bors commented Jul 15, 2016

bors commented Jul 15, 2016

jseyfried commented Jul 15, 2016

bors commented Jul 15, 2016

bors commented Jul 15, 2016

jseyfried commented Jun 30, 2016 •

edited

Loading

jseyfried Jul 3, 2016 •

edited

Loading

jseyfried commented Jul 3, 2016 •

edited

Loading

jseyfried commented Jul 15, 2016 •

edited

Loading

jseyfried commented Jul 15, 2016 •

edited

Loading