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

resolve: Relax shadowing restrictions on macro-expanded macros #53778

Merged
merged 11 commits into from Sep 9, 2018

Conversation

Projects
None yet
8 participants
@petrochenkov
Contributor

petrochenkov commented Aug 29, 2018

Previously any macro-expanded macros weren't allowed to shadow macros from outer scopes.
Now only "more macro-expanded" macros cannot shadow "less macro-expanded" macros.
See comments to fn may_appear_after and added tests for more details and examples.

The functional changes are a21f6f5 and 46dd365, other commits are refactorings.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Aug 29, 2018

Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Aug 29, 2018

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov
Contributor

petrochenkov commented Aug 29, 2018

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Aug 29, 2018

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Aug 29, 2018

Contributor

cc @nikomatsakis @nrc, you may be interested in this as well.
IIRC, there was a long discussion about what shadowing restrictions are necessary to make macro resolution/expansion predictable during the macro modularization RFC.

The resulting implemented rule was conservative - for macros 2.0 any macro-expanded macro could not shadow any other macro (legacy macros used more elaborate rules for backward compatibility).
This conservative rule prohibits totally reasonable code in which everything lives inside a single macro expansion (e.g. include!(...)) - see the example in #53205 (comment).

This PR refines that rule. The new rule is still simple enough (see the comments for fn may_appear_after) and is directly derived from requirements imposed on macro resolutions by any fixed-point expansion algorithm that's more or less similar to ours.
(It also pretty much fits what was previously implemented for legacy macros.)

Contributor

petrochenkov commented Aug 29, 2018

cc @nikomatsakis @nrc, you may be interested in this as well.
IIRC, there was a long discussion about what shadowing restrictions are necessary to make macro resolution/expansion predictable during the macro modularization RFC.

The resulting implemented rule was conservative - for macros 2.0 any macro-expanded macro could not shadow any other macro (legacy macros used more elaborate rules for backward compatibility).
This conservative rule prohibits totally reasonable code in which everything lives inside a single macro expansion (e.g. include!(...)) - see the example in #53205 (comment).

This PR refines that rule. The new rule is still simple enough (see the comments for fn may_appear_after) and is directly derived from requirements imposed on macro resolutions by any fixed-point expansion algorithm that's more or less similar to ours.
(It also pretty much fits what was previously implemented for legacy macros.)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 29, 2018

Member

The code changes seems reasonable here, but I'm no expert on the intricacies of macro resolution nor what the long-term future plans are. In that sense I'll defer to @nrc and others for the review of the purpose of the PR

Member

alexcrichton commented Aug 29, 2018

The code changes seems reasonable here, but I'm no expert on the intricacies of macro resolution nor what the long-term future plans are. In that sense I'll defer to @nrc and others for the review of the purpose of the PR

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov
Contributor

petrochenkov commented Aug 29, 2018

@rust-highfive rust-highfive assigned nrc and unassigned alexcrichton Aug 29, 2018

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Aug 30, 2018

Member

The code looks fine and the relaxed rules sound fine too, but I've really paged out name resolution, so I'm not 100% sure. Hopefully Niko can remember more than me.

Member

nrc commented Aug 30, 2018

The code looks fine and the relaxed rules sound fine too, but I've really paged out name resolution, so I'm not 100% sure. Hopefully Niko can remember more than me.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Aug 31, 2018

Contributor

Updated with tests demonstrating possible expansion/shadowing configurations.
r? @nikomatsakis

Contributor

petrochenkov commented Aug 31, 2018

Updated with tests demonstrating possible expansion/shadowing configurations.
r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned nrc Aug 31, 2018

bors added a commit that referenced this pull request Sep 3, 2018

Auto merge of #53913 - petrochenkov:biattr4, r=<try>
resolve: Future proof resolutions for potentially built-in attributes

Based on #53778

This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.
TODO: Explain what complications arise with the full solution.

bors added a commit that referenced this pull request Sep 3, 2018

Auto merge of #53913 - petrochenkov:biattr4, r=<try>
resolve: Future proof resolutions for potentially built-in attributes

Based on #53778

This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.
TODO: Explain what complications arise with the full solution.

cc #50911 (comment)
Closes #53531
@nikomatsakis

I've not really read the code too deply, mostly the test cases. I'm not sure I really understand the rule yet! @petrochenkov maybe you can clarify?

Show outdated Hide outdated src/librustc_resolve/macros.rs
// `+?` - configuration possible only with legacy scoping
// N | Outer ~ Invoc | Invoc ~ Inner | Outer ~ Inner | Possible |
// 1 | < | < | < | + |

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

I don't really understand this chart. I would assume that this means that "Inner takes precedence" -- e.g., because Outer < Invoc. But check_1 below seems to report an error?

And what is N here?

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

I don't really understand this chart. I would assume that this means that "Inner takes precedence" -- e.g., because Outer < Invoc. But check_1 below seems to report an error?

And what is N here?

This comment has been minimized.

@petrochenkov

petrochenkov Sep 4, 2018

Contributor

Outer < Invoc means expansion producing Outer is less than expansion producing Invoc.
Expansions form a tree (code produced by a macro expansion may contains multiple nested expansions and so on), and < means strictly closer to the root of that tree.

N is just the number of combination, from 1 to 4 * 4 * 4 = 64.

@petrochenkov

petrochenkov Sep 4, 2018

Contributor

Outer < Invoc means expansion producing Outer is less than expansion producing Invoc.
Expansions form a tree (code produced by a macro expansion may contains multiple nested expansions and so on), and < means strictly closer to the root of that tree.

N is just the number of combination, from 1 to 4 * 4 * 4 = 64.

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

Ah, ok, so this is like "can we construct a secnario where the outer is less than the invoc"... I see.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

Ah, ok, so this is like "can we construct a secnario where the outer is less than the invoc"... I see.

Show outdated Hide outdated src/test/ui/macros/restricted-shadowing-legacy.rs
Show outdated Hide outdated src/test/ui/macros/restricted-shadowing-legacy.rs
// Suppose that we resolved macro invocation with `invoc_id` to binding `binding` at some
// expansion round `max(invoc_id, binding)` when they both emerged from macros.
// Then this function returns `true` if `self` may emerge from a macro *after* that
// in some later round and screw up our previously found resolution.

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

Hmm, can we improve this comment a bit?

For example, I am not clear on how you get an error -- the comment makes it sounds like returning true means that the result "may screw up our previously found resolution" -- so is that an error? Or does "may" here mean "is allowed to".

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

Hmm, can we improve this comment a bit?

For example, I am not clear on how you get an error -- the comment makes it sounds like returning true means that the result "may screw up our previously found resolution" -- so is that an error? Or does "may" here mean "is allowed to".

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

From inspecting the rest of the code, it seems like true means error. If so, may I suggest not_shadowed_by as a possible name? Alternatively, invert the sense: name it is_shadowed_by.

I'd also like some comments that explain the logic here. Here is my attempt to figure it out. =) I think some examples are helpful, too, so I'll try to supply some.


This function defines the shadowing rules across macro invocations. The idea here is that if a macro definition defines a macro and then invokes it, we should resolve to the macro defined locally. So, for example, the following macro resolves just fine:

            macro gen_inner_invoc() {
                macro m() {}  // B
                m!(); // OK -- resolves to B
            }

This is true even if that macro is defined (or invoked) in a context that already contains a macro m:

macro m() { } // macro def'n A, shadowed by B above
maco gen_inner_invoc() { /* .. as above .. */ }
gen_inner_invoc!();

The general rule is that a macro B will shadow some "earlier" macro A if:

  • B is defined in a "subinvocation" of the one that defined A (as above)
  • B is XX (see my questions below)
@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

From inspecting the rest of the code, it seems like true means error. If so, may I suggest not_shadowed_by as a possible name? Alternatively, invert the sense: name it is_shadowed_by.

I'd also like some comments that explain the logic here. Here is my attempt to figure it out. =) I think some examples are helpful, too, so I'll try to supply some.


This function defines the shadowing rules across macro invocations. The idea here is that if a macro definition defines a macro and then invokes it, we should resolve to the macro defined locally. So, for example, the following macro resolves just fine:

            macro gen_inner_invoc() {
                macro m() {}  // B
                m!(); // OK -- resolves to B
            }

This is true even if that macro is defined (or invoked) in a context that already contains a macro m:

macro m() { } // macro def'n A, shadowed by B above
maco gen_inner_invoc() { /* .. as above .. */ }
gen_inner_invoc!();

The general rule is that a macro B will shadow some "earlier" macro A if:

  • B is defined in a "subinvocation" of the one that defined A (as above)
  • B is XX (see my questions below)

This comment has been minimized.

@petrochenkov

petrochenkov Sep 4, 2018

Contributor

OK, I think "shadowed by"/"not shadowed by" is not the right way to think about it, that also leads to confusion here and in #53778 (comment).

Macro from inner scope always shadows a macro from outer scope:

// Fully expanded
macro m() {} // Outer
{
    macro m(); // Inner, always shadows outer
}

the question is whether an instance of shadowing is "good" or "bad", which is determined entirely by their relative expansion order (earlier/later relations on expansions).
I hoped to convey that in the comments to may_appear_after, but probably not as successfully as I thought.

I'll try to reread this comment and #53778 (comment) later this evening and say something more specific.

@petrochenkov

petrochenkov Sep 4, 2018

Contributor

OK, I think "shadowed by"/"not shadowed by" is not the right way to think about it, that also leads to confusion here and in #53778 (comment).

Macro from inner scope always shadows a macro from outer scope:

// Fully expanded
macro m() {} // Outer
{
    macro m(); // Inner, always shadows outer
}

the question is whether an instance of shadowing is "good" or "bad", which is determined entirely by their relative expansion order (earlier/later relations on expansions).
I hoped to convey that in the comments to may_appear_after, but probably not as successfully as I thought.

I'll try to reread this comment and #53778 (comment) later this evening and say something more specific.

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

Yes, I.. well I'm not sure I see but I may be starting to see. =) I sort of had the sense I was thinking about it wrong. I will try to revisit this with this paragraph in mind.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

Yes, I.. well I'm not sure I see but I may be starting to see. =) I sort of had the sense I was thinking about it wrong. I will try to revisit this with this paragraph in mind.

let certainly_before_other_or_simultaneously =
other_parent_expansion.is_descendant_of(self_parent_expansion);
let certainly_before_invoc_or_simultaneously =
invoc_parent_expansion.is_descendant_of(self_parent_expansion);

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

I'm not sure I understand this. I think this is saying that the self would legally be shadowed by any resolution that happens in "a child" macro.. but is that right? I'm probably missing some bit of context here. I guess I'm imagining that this would apply in a case like:

macro m { .. }
macro foo {
    () => m!()
}
foo!() { }

Here, the invocation foo! is a "sub-expansion" of the one that defined m -- but this seems to suggest that there hence m the decision to use m cannot be shadowed. I guess that means that (e.g.) we have "definition 1" here?

macro m { .. } // definition 1

macro gen_m { () => macro m { ... } }
gen_m!(); // generates definition 2, possibly later

macro foo {
    () => m!() // resolves to definition 1
}
foo!() { }
@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

I'm not sure I understand this. I think this is saying that the self would legally be shadowed by any resolution that happens in "a child" macro.. but is that right? I'm probably missing some bit of context here. I guess I'm imagining that this would apply in a case like:

macro m { .. }
macro foo {
    () => m!()
}
foo!() { }

Here, the invocation foo! is a "sub-expansion" of the one that defined m -- but this seems to suggest that there hence m the decision to use m cannot be shadowed. I guess that means that (e.g.) we have "definition 1" here?

macro m { .. } // definition 1

macro gen_m { () => macro m { ... } }
gen_m!(); // generates definition 2, possibly later

macro foo {
    () => m!() // resolves to definition 1
}
foo!() { }
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 4, 2018

Contributor

I see that we are time sensitive here. If needed, I think we could go ahead and land this PR, but I would really like it if we can start to write down in a clearer way (apart from the source, ideally) our "name resolution model".

Contributor

nikomatsakis commented Sep 4, 2018

I see that we are time sensitive here. If needed, I think we could go ahead and land this PR, but I would really like it if we can start to write down in a clearer way (apart from the source, ideally) our "name resolution model".

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 5, 2018

Contributor

@nikomatsakis
So, I tried to lay out all the reasoning step by step from scratch and now I'm questioning all I know.

I'm still sure what this PR does is no worse than what the compiler was doing before it, and is also better in some sense.
However, I'm no longer sure why we are doing this at all, and what strict guarantees restricted shadowing gives us compared to simply verifying that the initial resolution of a macro (actually used during expansion) and its second validating resolution (performed after all expansion is done) are the same.

I need one more evening.

Contributor

petrochenkov commented Sep 5, 2018

@nikomatsakis
So, I tried to lay out all the reasoning step by step from scratch and now I'm questioning all I know.

I'm still sure what this PR does is no worse than what the compiler was doing before it, and is also better in some sense.
However, I'm no longer sure why we are doing this at all, and what strict guarantees restricted shadowing gives us compared to simply verifying that the initial resolution of a macro (actually used during expansion) and its second validating resolution (performed after all expansion is done) are the same.

I need one more evening.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 5, 2018

Contributor

First part of my notes:

Preface 1, The end result of macro expansion.

// Everything was expanded
macro m() {} // An Outer One, incorrect solution
{
	macro m() {} // An Outer One, incorrect solution
	{
		macro m() {} // An Outer One, incorrect solution
		{
			macro m() {} // The Inner One, correct solution
			
			m!(); // Or rather "trace" of it, since it was expanded too
		}
	}
}

Preface 2, Two stages.

Each macro call m!() is resolved twice - 1) initial resolution and 2) validating resolution.

Initial resolution

Happens when the crate is incomplete, some items are missing (not produced by expansions yet).
We find some solution and proceed with expanding it, this decision cannot be reverted later on, expansion does not backtrack.
The found solution may be incorrect (one of the Outer Ones), because the crate is incomplete.

Validating resolution

Happens when everything is expanded, the crate is complete and all items are in place.
Perfomed on a "trace" recorded during the initial resolution and containing all the data necessary for resolution - name and location of the original macro invocation.
Certainly founds the correct solution (the Inner One).

Looking at the initial resolution in more detail

So, we have just resolved some macro invocation to some macro definition (maybe even incorrect), what does that mean?
It means a lot actually!
It means that the macro invocation was already produced.
It also mean that the macro definition was already produced too.
It means that all parents of the invocation (expansion <= parent_expansion(invoc)) or the definition (expansion <= parent_expansion(initial_resolution)) were already expanded too.
It means that this is the best solution from those existing at this point in time (multiple solutions could be produced by the same expansion and appear simultaneously).
It means that if our initial resolution is incorrect (an Outer One), then the correct resolution (the Inner One) cannot be produced neither by ancestor(invoc) nor by ancestor(initial_resolution).
"Produced neither by ancestor(invoc) nor by ancestor(initial_resolution)" is exactly what fn may_appear_after returns.

...

Contributor

petrochenkov commented Sep 5, 2018

First part of my notes:

Preface 1, The end result of macro expansion.

// Everything was expanded
macro m() {} // An Outer One, incorrect solution
{
	macro m() {} // An Outer One, incorrect solution
	{
		macro m() {} // An Outer One, incorrect solution
		{
			macro m() {} // The Inner One, correct solution
			
			m!(); // Or rather "trace" of it, since it was expanded too
		}
	}
}

Preface 2, Two stages.

Each macro call m!() is resolved twice - 1) initial resolution and 2) validating resolution.

Initial resolution

Happens when the crate is incomplete, some items are missing (not produced by expansions yet).
We find some solution and proceed with expanding it, this decision cannot be reverted later on, expansion does not backtrack.
The found solution may be incorrect (one of the Outer Ones), because the crate is incomplete.

Validating resolution

Happens when everything is expanded, the crate is complete and all items are in place.
Perfomed on a "trace" recorded during the initial resolution and containing all the data necessary for resolution - name and location of the original macro invocation.
Certainly founds the correct solution (the Inner One).

Looking at the initial resolution in more detail

So, we have just resolved some macro invocation to some macro definition (maybe even incorrect), what does that mean?
It means a lot actually!
It means that the macro invocation was already produced.
It also mean that the macro definition was already produced too.
It means that all parents of the invocation (expansion <= parent_expansion(invoc)) or the definition (expansion <= parent_expansion(initial_resolution)) were already expanded too.
It means that this is the best solution from those existing at this point in time (multiple solutions could be produced by the same expansion and appear simultaneously).
It means that if our initial resolution is incorrect (an Outer One), then the correct resolution (the Inner One) cannot be produced neither by ancestor(invoc) nor by ancestor(initial_resolution).
"Produced neither by ancestor(invoc) nor by ancestor(initial_resolution)" is exactly what fn may_appear_after returns.

...

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 5, 2018

Contributor

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

Contributor

bors commented Sep 5, 2018

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

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 5, 2018

Contributor

@petrochenkov

I am a bit surprised to here that shadowing is not the right way to think about it. I think that this problem arises, perhaps, because we are not considering a "rich enough" view of the expanded source. Perhaps if we included the "trace" of the macros that were expanded in our result, we could define shadowing rules?

I basically expect that we ought to be able to show a fully expanded source and say that the error occurs because some macro invocation has two candidates and neither of them shadows one another. Then we can define the rule that lets a macro invocation shadow another as some specific invocation site.

It seems like the answer is:

  • At the invocation site X, a macro definition Inner shadows a macro definition Outer if:
    • The definitions of both Inner and Outer are ancestors of the invocation site X
    • The definition of Outer is a strict ancestor of the definition of Inner
      • this is overly simplified; I'm ignoring block scoping here, which has to be taken into account

I say that something is an "ancestor" if, in the tree of invocations, it is an ancestor -- in other words, if some text A was expanded to include B, then A is an ancestor of B. Also, B is an ancestor of itself. A "strict ancestor" is not.

Applying that to this test you wrote:

    fn check1() {
        macro m() {} // candidate A
        {
            #[rustc_transparent_macro]
            macro gen_gen_inner_invoc() {
                gen_inner!(); // generates candidate B
                m!(); //~ ERROR `m` is ambiguous
            }
            gen_gen_inner_invoc!();
        }
    }

I believe that this fully expands to:

macro m { } // candidate A
{
  macro gen_gen_inner_invoc() { /* definition not relevant */ }
  gen_gen_inner_invoc!() as {
    gen_inner!() as {
      macro m { .. } // candidate B
    }

    m!() // ERROR
  }
}

Here we have an error because candidate B is not an ancestor of the invocation.


In contrast, in check5:

    fn check5() {
        macro m() {}
        {
            #[rustc_transparent_macro]
            macro gen_inner_invoc() {
                macro m() {}
                m!(); // OK
            }
            gen_inner_invoc!();
        }
    }

we fully expand to

macro m { } // candidate A
{
  macro gen_gen_inner_invoc() { /* definition not relevant */ }
  gen_inner_invoc!() as {
    macro m { .. } // candidate B

    m!() // ERROR
  }
}

Here it works out because both A and B are ancestors of the invocation site, and A is a strict ancestor of B.


In this example, my rule doesn't work, because I didn't account for blocking scoping:

    fn check10() {
        macro m() {}
        {
            macro m() {}
            gen_invoc!(); // OK
        }
    }

but I think you can extend my "tree" notion to include not just macro invocations but also block scoping, and then it works fine here.


This example check13

    fn check13() {
        macro m() {}
        {
            gen_inner!();
            #[rustc_transparent_macro]
            macro gen_invoc() { m!() } //~ ERROR `m` is ambiguous
            gen_invoc!();
        }
    }

is basically the same as check1 from the point of view of my check. The gen_inner! call would create a macro candidate B that is visible to the invocation but not an ancestor of it, so shadowing results in an error.

(I realize now I didn't define how a macro becomes visible, but that is basically the same as any other amount of lexical scoping, except that we traverse through macro invocations.)


Does this description of the rule make sense to you @petrochenkov ?

Contributor

nikomatsakis commented Sep 5, 2018

@petrochenkov

I am a bit surprised to here that shadowing is not the right way to think about it. I think that this problem arises, perhaps, because we are not considering a "rich enough" view of the expanded source. Perhaps if we included the "trace" of the macros that were expanded in our result, we could define shadowing rules?

I basically expect that we ought to be able to show a fully expanded source and say that the error occurs because some macro invocation has two candidates and neither of them shadows one another. Then we can define the rule that lets a macro invocation shadow another as some specific invocation site.

It seems like the answer is:

  • At the invocation site X, a macro definition Inner shadows a macro definition Outer if:
    • The definitions of both Inner and Outer are ancestors of the invocation site X
    • The definition of Outer is a strict ancestor of the definition of Inner
      • this is overly simplified; I'm ignoring block scoping here, which has to be taken into account

I say that something is an "ancestor" if, in the tree of invocations, it is an ancestor -- in other words, if some text A was expanded to include B, then A is an ancestor of B. Also, B is an ancestor of itself. A "strict ancestor" is not.

Applying that to this test you wrote:

    fn check1() {
        macro m() {} // candidate A
        {
            #[rustc_transparent_macro]
            macro gen_gen_inner_invoc() {
                gen_inner!(); // generates candidate B
                m!(); //~ ERROR `m` is ambiguous
            }
            gen_gen_inner_invoc!();
        }
    }

I believe that this fully expands to:

macro m { } // candidate A
{
  macro gen_gen_inner_invoc() { /* definition not relevant */ }
  gen_gen_inner_invoc!() as {
    gen_inner!() as {
      macro m { .. } // candidate B
    }

    m!() // ERROR
  }
}

Here we have an error because candidate B is not an ancestor of the invocation.


In contrast, in check5:

    fn check5() {
        macro m() {}
        {
            #[rustc_transparent_macro]
            macro gen_inner_invoc() {
                macro m() {}
                m!(); // OK
            }
            gen_inner_invoc!();
        }
    }

we fully expand to

macro m { } // candidate A
{
  macro gen_gen_inner_invoc() { /* definition not relevant */ }
  gen_inner_invoc!() as {
    macro m { .. } // candidate B

    m!() // ERROR
  }
}

Here it works out because both A and B are ancestors of the invocation site, and A is a strict ancestor of B.


In this example, my rule doesn't work, because I didn't account for blocking scoping:

    fn check10() {
        macro m() {}
        {
            macro m() {}
            gen_invoc!(); // OK
        }
    }

but I think you can extend my "tree" notion to include not just macro invocations but also block scoping, and then it works fine here.


This example check13

    fn check13() {
        macro m() {}
        {
            gen_inner!();
            #[rustc_transparent_macro]
            macro gen_invoc() { m!() } //~ ERROR `m` is ambiguous
            gen_invoc!();
        }
    }

is basically the same as check1 from the point of view of my check. The gen_inner! call would create a macro candidate B that is visible to the invocation but not an ancestor of it, so shadowing results in an error.

(I realize now I didn't define how a macro becomes visible, but that is basically the same as any other amount of lexical scoping, except that we traverse through macro invocations.)


Does this description of the rule make sense to you @petrochenkov ?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 6, 2018

Contributor

@nikomatsakis
So, I came up with a from-scratch explanation of restricted shadowing.
(But this is not an answer to your posts yet, sorry.)

The intent

What is the intent of restricted shadowing (even if it's not entirely fulfilled)?
The intent is pretty clear - making the expansion result independent from expansion order of individual macro invocations, resolution order for individual imports, and ultimately item order in modules.

How expansion algorithm should work to ensure this order-independence?

Expansion algorithm

It should work in rounds in a somewhat transactional way:

  1. Collect unresolved paths from imports and macro invocations.
  2. Resolve collected paths independently and speculatively.
  3. Commit: writeback resolved import resolutions into modules, expand resolved macros.
  4. IF PROGRESS GOTO 1

Or, alternatively, with imports and macros separated:

  1. Collect (imports) unresolved paths from imports.
  2. Resolve (imports) collected paths independently and speculatively.
  3. Commit (imports): writeback resolved import resolutions into modules.
  4. IF PROGRESS GOTO 1
  5. Collect (macros) unresolved paths from macro invocations.
  6. Resolve (macros) collected paths independently and speculatively.
  7. Commit (macros): expand resolved macros.
  8. IF PROGRESS GOTO 1

In this case we can number all expansion rounds from 0 to N and have our expansions totally ordered.
Let's define R(X) as a round that produced macro definition or macro invocation X.

If we found some resolution A for invocation I in round R(A, I) = max(R(A), R(I)), and then later in round R(A') > R(A, I) we found another resolution A' for I that's better than A (aka "shadows A", i.e. closer to I in scopes), then we are in self-inconsistent state and must report an error.
This is the precise version of restricted shadowing.

However, this precise version is tied to specific round numbers, which may depend on algorithm specifics (e.g. whether we are splitting imports and macros), so we may want something more conservative.

More conservative rule

"More conservative" means "R(A') > R(A, I) IMPLIES REPORT_ERROR", i.e. a superset of the precise rule errors is reported.

Now, note that if Parent Expansion of X is a (non-strict) ancestor of parent expansion of Y, then R(X) is certainly less-or-equal than R(Y): PE(X) <= PE(Y) IMPLIES R(X) <= R(Y)), or, after inverting, R(X) > R(Y) IMPLIES !(PE(X) <= PE(Y)).

Now let's apply some algebra to get rid of specific round numbers.
R(A') > R(A, I) == R(A') > max(R(A), R(I)) == R(A') > R(A) && R(A') > R(I) IMPLIES !(PE(A') <= PE(A)) && !(PE(A') <= PE(I)) == !(PE(A') <= PE(A) || PE(A') <= PE(I)).

So we can take REPORT_ERROR == !(PE(A') <= PE(A) || PE(A') <= PE(I)), that's exactly what this PR does :)

Very conservative rule

!(PE(A') <= PE(A) || PE(A') <= PE(I)) IMPLIES !(PE(A') <= PE(I)), so we can take REPORT_ERROR == !(PE(A') <= PE(I)) as well.

This is what macro_rules used all the time before this PR.
(Non-macro_rules macros used even dumber REPORT_ERROR == !(PE(A') <= ROOT) == PE(A') != ROOT.)

Real life

Ok, now let's return to how compiler actually performs expansion:

  1. Collect unresolved imports and macro invocations.
  2. Resolve a single collected import.
  3. Commit: writeback the resolved import into modules.
  4. IF PROGRESS GOTO 1
  5. Resolve a single collected macro.
  6. Commit: expand the resolved macro.
  7. IF PROGRESS GOTO 4
  8. GOTO 1

That's... pretty order-dependent.
So all our efforts with restricted shadowing are shattered by the bad expansion algorithm, that's disappointing!

However...

Conclusion

... I still have some degree of confidence that the expansion algorithm can be reimplemented in order-independent manner with minimal practical breakage, so we should keep the appropriate shadowing restriction rules described above until then.

Contributor

petrochenkov commented Sep 6, 2018

@nikomatsakis
So, I came up with a from-scratch explanation of restricted shadowing.
(But this is not an answer to your posts yet, sorry.)

The intent

What is the intent of restricted shadowing (even if it's not entirely fulfilled)?
The intent is pretty clear - making the expansion result independent from expansion order of individual macro invocations, resolution order for individual imports, and ultimately item order in modules.

How expansion algorithm should work to ensure this order-independence?

Expansion algorithm

It should work in rounds in a somewhat transactional way:

  1. Collect unresolved paths from imports and macro invocations.
  2. Resolve collected paths independently and speculatively.
  3. Commit: writeback resolved import resolutions into modules, expand resolved macros.
  4. IF PROGRESS GOTO 1

Or, alternatively, with imports and macros separated:

  1. Collect (imports) unresolved paths from imports.
  2. Resolve (imports) collected paths independently and speculatively.
  3. Commit (imports): writeback resolved import resolutions into modules.
  4. IF PROGRESS GOTO 1
  5. Collect (macros) unresolved paths from macro invocations.
  6. Resolve (macros) collected paths independently and speculatively.
  7. Commit (macros): expand resolved macros.
  8. IF PROGRESS GOTO 1

In this case we can number all expansion rounds from 0 to N and have our expansions totally ordered.
Let's define R(X) as a round that produced macro definition or macro invocation X.

If we found some resolution A for invocation I in round R(A, I) = max(R(A), R(I)), and then later in round R(A') > R(A, I) we found another resolution A' for I that's better than A (aka "shadows A", i.e. closer to I in scopes), then we are in self-inconsistent state and must report an error.
This is the precise version of restricted shadowing.

However, this precise version is tied to specific round numbers, which may depend on algorithm specifics (e.g. whether we are splitting imports and macros), so we may want something more conservative.

More conservative rule

"More conservative" means "R(A') > R(A, I) IMPLIES REPORT_ERROR", i.e. a superset of the precise rule errors is reported.

Now, note that if Parent Expansion of X is a (non-strict) ancestor of parent expansion of Y, then R(X) is certainly less-or-equal than R(Y): PE(X) <= PE(Y) IMPLIES R(X) <= R(Y)), or, after inverting, R(X) > R(Y) IMPLIES !(PE(X) <= PE(Y)).

Now let's apply some algebra to get rid of specific round numbers.
R(A') > R(A, I) == R(A') > max(R(A), R(I)) == R(A') > R(A) && R(A') > R(I) IMPLIES !(PE(A') <= PE(A)) && !(PE(A') <= PE(I)) == !(PE(A') <= PE(A) || PE(A') <= PE(I)).

So we can take REPORT_ERROR == !(PE(A') <= PE(A) || PE(A') <= PE(I)), that's exactly what this PR does :)

Very conservative rule

!(PE(A') <= PE(A) || PE(A') <= PE(I)) IMPLIES !(PE(A') <= PE(I)), so we can take REPORT_ERROR == !(PE(A') <= PE(I)) as well.

This is what macro_rules used all the time before this PR.
(Non-macro_rules macros used even dumber REPORT_ERROR == !(PE(A') <= ROOT) == PE(A') != ROOT.)

Real life

Ok, now let's return to how compiler actually performs expansion:

  1. Collect unresolved imports and macro invocations.
  2. Resolve a single collected import.
  3. Commit: writeback the resolved import into modules.
  4. IF PROGRESS GOTO 1
  5. Resolve a single collected macro.
  6. Commit: expand the resolved macro.
  7. IF PROGRESS GOTO 4
  8. GOTO 1

That's... pretty order-dependent.
So all our efforts with restricted shadowing are shattered by the bad expansion algorithm, that's disappointing!

However...

Conclusion

... I still have some degree of confidence that the expansion algorithm can be reimplemented in order-independent manner with minimal practical breakage, so we should keep the appropriate shadowing restriction rules described above until then.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 6, 2018

Contributor

I think that this problem arises, perhaps, because we are not considering a "rich enough" view of the expanded source. Perhaps if we included the "trace" of the macros that were expanded in our result, we could define shadowing rules?

Traces of macros are already recorded, we are performing the validating resolution (#53778 (comment)) on them.

I basically expect that we ought to be able to show a fully expanded source and say that the error occurs because some macro invocation has two candidates ...

Yes, that's what validating resolution does (fn resolve_legacy_scope/fn resolve_lexical_macro_path_segment when record_used is true).

... and neither of them shadows one another. Then we can define the rule that lets a macro invocation shadow another as some specific invocation site.

Ok, in my formulation the inner candidate always shadows the outer candidate during the validating resolution, but the shadowing may be illegal (self-inconsistent) because the inner candidate arrived too late, so the invocation already left with another candidate.

It seems like the answer is: ...

Something is mixed up in details, because at least "both Inner and Outer are ancestors of the invocation site X" is not correct, but I think it's possible to define it in this way yes.
I'm just not sure two orthogonal aspects - shadowing (block scoping, inner/outer, closer/farther) and expansion ordering (earlier/later) need to be conflated in this way.

Applying that to this test [fn check1] you wrote:
Here we have an error because candidate B is not an ancestor of the invocation.

In my model it's an error because B may appear "later" (or rather "not-earlier-or-simultaneously" due to partial ordering) than both A and m!(), i.e. it's not an ancestor of either of them.

In contrast, in check5:
Here it works out because both A and B are ancestors of the invocation site, and A is a strict ancestor of B.

In this case it's ok because B always appears earlier-or-simultaneously with m!(), in this case it's irrelevant when A arrives.
In other words, when m!() is available for resolving, B is always available to be used as resolution as well.

In this example [fn check10], my rule doesn't work, because I didn't account for blocking scoping:

This case is equivalent to the previous one - when m!() becomes available for resolving, B is always already there.
(B appears earlier-or-simultaneously with m!(), when A appears is irrelevant.)

This example check13
is basically the same as check1 from the point of view of my check. The gen_inner! call would create a macro candidate B that is visible to the invocation but not an ancestor of it, so shadowing results in an error.

This case is similar to 1, yes. B may appear "later" than both A and m!().
(Not an ancestor of A AND not an ancestor of m!().)

(I realize now I didn't define how a macro becomes visible, but that is basically the same as any other amount of lexical scoping, except that we traverse through macro invocations.)

This may be the culprit. It seems misleading to me to think of macro expansions as some kind of scopes, like blocks. Tree of macro expansions is some entirely independent dimension from tree of scopes (even more so for macro_rules when we are less bound by block syntax), so we better move along them independently ("early/later" for one and "inner/outer" for another, as I mentioned previously).

So yeah, this is largely a matter of terminology, but I hope that maybe #53778 (comment) clarifies something.

Contributor

petrochenkov commented Sep 6, 2018

I think that this problem arises, perhaps, because we are not considering a "rich enough" view of the expanded source. Perhaps if we included the "trace" of the macros that were expanded in our result, we could define shadowing rules?

Traces of macros are already recorded, we are performing the validating resolution (#53778 (comment)) on them.

I basically expect that we ought to be able to show a fully expanded source and say that the error occurs because some macro invocation has two candidates ...

Yes, that's what validating resolution does (fn resolve_legacy_scope/fn resolve_lexical_macro_path_segment when record_used is true).

... and neither of them shadows one another. Then we can define the rule that lets a macro invocation shadow another as some specific invocation site.

Ok, in my formulation the inner candidate always shadows the outer candidate during the validating resolution, but the shadowing may be illegal (self-inconsistent) because the inner candidate arrived too late, so the invocation already left with another candidate.

It seems like the answer is: ...

Something is mixed up in details, because at least "both Inner and Outer are ancestors of the invocation site X" is not correct, but I think it's possible to define it in this way yes.
I'm just not sure two orthogonal aspects - shadowing (block scoping, inner/outer, closer/farther) and expansion ordering (earlier/later) need to be conflated in this way.

Applying that to this test [fn check1] you wrote:
Here we have an error because candidate B is not an ancestor of the invocation.

In my model it's an error because B may appear "later" (or rather "not-earlier-or-simultaneously" due to partial ordering) than both A and m!(), i.e. it's not an ancestor of either of them.

In contrast, in check5:
Here it works out because both A and B are ancestors of the invocation site, and A is a strict ancestor of B.

In this case it's ok because B always appears earlier-or-simultaneously with m!(), in this case it's irrelevant when A arrives.
In other words, when m!() is available for resolving, B is always available to be used as resolution as well.

In this example [fn check10], my rule doesn't work, because I didn't account for blocking scoping:

This case is equivalent to the previous one - when m!() becomes available for resolving, B is always already there.
(B appears earlier-or-simultaneously with m!(), when A appears is irrelevant.)

This example check13
is basically the same as check1 from the point of view of my check. The gen_inner! call would create a macro candidate B that is visible to the invocation but not an ancestor of it, so shadowing results in an error.

This case is similar to 1, yes. B may appear "later" than both A and m!().
(Not an ancestor of A AND not an ancestor of m!().)

(I realize now I didn't define how a macro becomes visible, but that is basically the same as any other amount of lexical scoping, except that we traverse through macro invocations.)

This may be the culprit. It seems misleading to me to think of macro expansions as some kind of scopes, like blocks. Tree of macro expansions is some entirely independent dimension from tree of scopes (even more so for macro_rules when we are less bound by block syntax), so we better move along them independently ("early/later" for one and "inner/outer" for another, as I mentioned previously).

So yeah, this is largely a matter of terminology, but I hope that maybe #53778 (comment) clarifies something.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 7, 2018

Contributor

@nikomatsakis
This, for example:

macro_rules! gen_outer_inner {() => {
    macro_rules! m {() => ()} // Outer
    macro_rules! m {() => ()} // Inner
}}

fn main() {
    gen_outer_inner!();
    m!(); // Invoc
}

UPDATE: Test cases 34, 35, 62 and 63 from restricted-shadowing-legacy.rs.

Contributor

petrochenkov commented Sep 7, 2018

@nikomatsakis
This, for example:

macro_rules! gen_outer_inner {() => {
    macro_rules! m {() => ()} // Outer
    macro_rules! m {() => ()} // Inner
}}

fn main() {
    gen_outer_inner!();
    m!(); // Invoc
}

UPDATE: Test cases 34, 35, 62 and 63 from restricted-shadowing-legacy.rs.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 8, 2018

Contributor

@nikomatsakis
Updated.

Contributor

petrochenkov commented Sep 8, 2018

@nikomatsakis
Updated.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 9, 2018

Contributor

@bors r=nikomatsakis p=1
I'm going to interpret the semi-r+ in #53778 (comment) as r+, otherwise we'll have too much code to backport after beta (which is in a few days).

Contributor

petrochenkov commented Sep 9, 2018

@bors r=nikomatsakis p=1
I'm going to interpret the semi-r+ in #53778 (comment) as r+, otherwise we'll have too much code to backport after beta (which is in a few days).

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 9, 2018

Contributor

📌 Commit 2dce377 has been approved by nikomatsakis

Contributor

bors commented Sep 9, 2018

📌 Commit 2dce377 has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 9, 2018

Contributor

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #54011
Contributor

bors commented Sep 9, 2018

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #54011
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 9, 2018

Contributor

📌 Commit 2dce377 has been approved by nikomatsakis

Contributor

bors commented Sep 9, 2018

📌 Commit 2dce377 has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 9, 2018

Contributor

⌛️ Testing commit 2dce377 with merge 2d4e34c...

Contributor

bors commented Sep 9, 2018

⌛️ Testing commit 2dce377 with merge 2d4e34c...

bors added a commit that referenced this pull request Sep 9, 2018

Auto merge of #53778 - petrochenkov:shadrelax2, r=nikomatsakis
resolve: Relax shadowing restrictions on macro-expanded macros

Previously any macro-expanded macros weren't allowed to shadow macros from outer scopes.
Now only "more macro-expanded" macros cannot shadow "less macro-expanded" macros.
See comments to `fn may_appear_after` and added tests for more details and examples.

The functional changes are a21f6f5 and 46dd365, other commits are refactorings.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 9, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2d4e34c to master...

Contributor

bors commented Sep 9, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2d4e34c to master...

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