Skip to content
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

Assign node ids during macro expansion #36438

Merged
merged 9 commits into from
Sep 15, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Sep 13, 2016

After this PR,

  • The ExtCtxt can access resolve's Resolver through the trait object ext::base::Resolver.
    • The Resolver trait object can load macros and replaces today's MacroLoader trait object.
    • The macro expander uses the Resolver trait object to resolve macro invocations.
  • The macro expander assigns node ids and builds the Resolver's macros_at_scope map.
  • Performance of expansion together with node id assignment improves by ~5%.

EDIT: Since Github is reordering the commits, here is git log:

  • b54e1e3: Differentiate between monotonic and non-monotonic expansion and only assign node ids during monotonic expansion.
  • 78c0039: Expand generated test harnesses and macro registries.
  • f3c2dca: Remove scope placeholders from the crate root.
  • c86c8d4: Perform node id assignment and macros_at_scope construction during the InvocationCollector and PlaceholderExpander folds.
  • 72a6369: Move macro resolution into librustc_resolve.
  • 20b43b2: Rewrite the unit tests in ext/expand.rs as a compile-fail test.
  • a9821e1: Refactor ExtCtxt to use a Resolver instead of a MacroLoader.
  • 60440b2: Refactor noop_fold_stmt_kind out of noop_fold_stmt.
  • 50f94f6: Avoid needless reexpansions.

r? @nrc


// create a really evil test case where a $x appears inside a binding of $x
// but *shouldn't* bind because it was inserted by a different macro....
// can't write this test case until we have macro-generating macros.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have a test like this -- c.f. #35453.

@jseyfried
Copy link
Contributor Author

cc @eddyb

@@ -488,6 +489,9 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {
fn metadata_encoding_version(&self) -> &[u8] { bug!("metadata_encoding_version") }
}

pub trait MacroLoader {
fn load_crate(&mut self, _: &ast::Item, _: bool) -> Vec<LoadedMacro>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be useful to specify argument names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

@nrc
Copy link
Member

nrc commented Sep 14, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 14, 2016

📌 Commit b54e1e3 has been approved by nrc

@bors
Copy link
Contributor

bors commented Sep 14, 2016

⌛ Testing commit b54e1e3 with merge bcf6bd2...

@bors
Copy link
Contributor

bors commented Sep 14, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Sep 13, 2016 at 9:42 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/2504


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#36438 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95Ai7ZBb7A_lcI1k0I3kn7aSEJIUmks5qp3scgaJpZM4J7Q2c
.

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 15, 2016
Assign node ids during macro expansion

After this PR,
 - The `ExtCtxt` can access `resolve`'s `Resolver` through the trait object `ext::base::Resolver`.
  - The `Resolver` trait object can load macros and replaces today's `MacroLoader` trait object.
  - The macro expander uses the `Resolver` trait object to resolve macro invocations.
 - The macro expander assigns node ids and builds the `Resolver`'s `macros_at_scope` map.
   - This is groundwork for merging import resolution and expansion.
 - Performance of expansion together with node id assignment improves by ~5%.

**EDIT:** Since Github is reordering the commits, here is `git log`:
 - b54e1e3: Differentiate between monotonic and non-monotonic expansion and only assign node ids during monotonic expansion.
 - 78c0039: Expand generated test harnesses and macro registries.
 - f3c2dca: Remove scope placeholders from the crate root.
 - c86c8d4: Perform node id assignment and `macros_at_scope` construction during the `InvocationCollector` and `PlaceholderExpander` folds.
 - 72a6369: Move macro resolution into `librustc_resolve`.
 - 20b43b2: Rewrite the unit tests in `ext/expand.rs` as a `compile-fail` test.
 - a9821e1: Refactor `ExtCtxt` to use a `Resolver` instead of a `MacroLoader`.
 - 60440b2: Refactor `noop_fold_stmt_kind` out of `noop_fold_stmt`.
 - 50f94f6: Avoid needless reexpansions.

r? @nrc
bors added a commit that referenced this pull request Sep 15, 2016
Rollup of 9 pull requests

- Successful merges: #36384, #36405, #36425, #36429, #36438, #36454, #36459, #36461, #36463
- Failed merges: #36444
@bors bors merged commit b54e1e3 into rust-lang:master Sep 15, 2016
@jseyfried jseyfried deleted the node_ids_in_expansion branch September 16, 2016 20:16
est31 added a commit to est31/rust that referenced this pull request Nov 6, 2020
Originally, there has been a dedicated pass for renumbering
AST NodeIds to have actual values. This pass had been added by
commit a5ad4c3.

Then, later, this step was moved to where it resides now,
macro expansion. See commit c86c8d4
or PR rust-lang#36438.

The comment snippet, added by the original commit, has
survived the times without any change, becoming outdated
at removal of the dedicated pass.

Nowadays, grepping for the next_node_id function will show up
multiple places in the compiler that call it, but the main
rewriting that the comment talks about is still done in the
expansion step, inside an innocious looking visit_id function
that's called during macro invocation collection.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 6, 2020
…=oli-obk

The renumber pass is long gone

Originally, there has been a dedicated pass for renumbering
AST NodeIds to have actual values. This pass had been added by
commit a5ad4c3.

Then, later, this step was moved to where it resides now,
macro expansion. See commit c86c8d4
or PR rust-lang#36438.

The comment snippet, added by the original commit, has
survived the times without any change, becoming outdated
at removal of the dedicated pass.

Nowadays, grepping for the next_node_id function will show up
multiple places in the compiler that call it, but the main
rewriting that the comment talks about is still done in the
expansion step, inside an innocious looking visit_id function
that's called during macro invocation collection.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 6, 2020
…=oli-obk

The renumber pass is long gone

Originally, there has been a dedicated pass for renumbering
AST NodeIds to have actual values. This pass had been added by
commit a5ad4c3.

Then, later, this step was moved to where it resides now,
macro expansion. See commit c86c8d4
or PR rust-lang#36438.

The comment snippet, added by the original commit, has
survived the times without any change, becoming outdated
at removal of the dedicated pass.

Nowadays, grepping for the next_node_id function will show up
multiple places in the compiler that call it, but the main
rewriting that the comment talks about is still done in the
expansion step, inside an innocious looking visit_id function
that's called during macro invocation collection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants