Skip to content

Macro Exterminator #15601

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

Merged
merged 4 commits into from
Jul 12, 2014
Merged

Conversation

jbclements
Copy link
Contributor

Our AST definition can include macro invocations, which can expand into all kinds of things. Macro invocations are expanded away during expansion time, and the rest of the compiler doesn't have to deal with them. However, we have no way of enforcing this.

This patch adds two protective mechanisms.

First, it adds a (quick) explicit check that ensures there are no macro invocations remaining in the AST after expansion. Second, it updates the visit and fold mechanisms so that by default, they will not traverse macro invocations. It's easy enough to add this, if desired (it's documented in the source, and examples appear, e.g. in the IdentFinder.

Along the way, I also consulted with @sfackler to refactor the macro export mechanism so that it stores macro text spans in a side table, rather than leaving them in the AST.

@sfackler
Copy link
Member

One interesting implication of this that I didn't think about before is that --pretty expanded source will not export macros that the unexpanded source would. I don't know how much we care about the semantics of the expanded source, though, as there are already other issues with e.g. hygiene.

@jbclements
Copy link
Contributor Author

On Jul 10, 2014, at 11:30 PM, Steven Fackler [email protected] wrote:

One interesting implication of this that I didn't think about before is that --pretty expanded source will not export macros that the unexpanded source would. I don't know how much we care about the semantics of the expanded source, though, as there are already other issues with e.g. hygiene.

I thought about that a bit. As you point out, though, there are much larger problems with pretty —expanded.

plus, all the tests pass!


Reply to this email directly or view it on GitHub.

@lilyball
Copy link
Contributor

@sfackler Personally I would love to see the --pretty expanded source work for as many programs as possible (ideally 100%). Any change that explicitly prevents that from being possible makes me sad.

FWIW, the only feature I know of offhand that should be impossible to implement in --pretty expanded is the #![!resolve_unexported] attribute that the auto-generated test module uses, and that's because that attribute very intentionally is invalid to parse. That bugs me, but at least there I can delete the test module and pass the --test flag again and it should work. Aside from that one intentional breakage, everything else theoretically should work. On the matter of hygiene, that could perhaps be solved with a hygiene-preserving pass done to the expanded source that renames any identifiers that would be unhygienic if left alone (since they're always created by macros it's not a big deal if their names are mangled anyway).

@jbclements
Copy link
Contributor Author

@kballard your point is well taken, but I'm reluctant to spend time on fixing pretty --expanded for two reasons:

  1. I believe that @pcwalton is hoping to replace pretty --expanded entirely, and
  2. The current system simply copies source text, which doesn't work when e.g. you have a macro
    that expands into another macro.

fn visit_mac(&mut self, macro: &ast::Mac, _:()) {
self.sess.span_diagnostic.span_err(macro.span,
"macro exterminator: expected AST \
with no macro invocations");
Copy link
Member

Choose a reason for hiding this comment

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

Should this be span_bug instead of span_err?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! right you are.

There were two things named MacroExpander, which was confusing. I renamed
one of them TTMacroExpander.

[breaking change]
Per discussion with @sfackler, refactored the expander to
change the way that exported macros are collected. Specifically,
a crate now contains a side table of spans that exported macros
go into.

This has two benefits. First, the encoder doesn't need to scan through
the expanded crate in order to discover exported macros. Second, the
expander can drop all expanded macros from the crate, with the pleasant
result that a fully expanded crate contains no macro invocations (which
include macro definitions).
macros can expand into arbitrary items, exprs, etc. This
means that using a default walker or folder on an AST before
macro expansion is complete will miss things (the things that
the macros expand into). As a partial fence against this, this
commit moves the default traversal of macros into a separate
procedure, and makes the default trait implementation signal
an error. This means that Folders and Visitors can traverse
macros if they want to, but they need to explicitly add an
impl that calls the walk_mac or fold_mac procedure

This should prevent problems down the road.
the Macro Exterminator ensures that there are no macro invocations in
an AST. This should help make later passes confident that there aren't
hidden items, methods, expressions, etc.
bors added a commit that referenced this pull request Jul 12, 2014
…, r=alexcrichton

Our AST definition can include macro invocations, which can expand into all kinds of things. Macro invocations are expanded away during expansion time, and the rest of the compiler doesn't have to deal with them. However, we have no way of enforcing this.

This patch adds two protective mechanisms.

First, it adds a (quick) explicit check that ensures there are no macro invocations remaining in the AST after expansion. Second, it updates the visit and fold mechanisms so that by default, they will not traverse macro invocations. It's easy enough to add this, if desired (it's documented in the source, and examples appear, e.g. in the IdentFinder.

Along the way, I also consulted with @sfackler to refactor the macro export mechanism so that it stores macro text spans in a side table, rather than leaving them in the AST.
@bors bors closed this Jul 12, 2014
@bors bors merged commit c253b36 into rust-lang:master Jul 12, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
fix: Temporarily skip decl check in derive expansions

"Fixes rust-lang/rust-analyzer#15344"
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