jseyfried
commented
8 years ago Macro calls shouldn't ever have "normal" node ids that could conflict with placeholder ids (i.e. Marks) -- that is, a macro call's id should be either DUMMY_NODE_ID or a placeholder id that represents a valid Mark.
Macro calls are being erroneously assigned "normal" (non-placeholder) node ids when they are noop_folded by the InvocationCollector here (and here, etc.). Avoiding the noop_folds (e.g. replacing return P(noop_fold_expr(expr, self)); with return P(expr);) should fix the collision problem.
In fact, Syntex shouldn't need to assign normal node ids at all, so you could replace next_node_id with a dummy implementation that always returns DUMMY_NODE_ID.
This PR hacks around an odd bug in syntex due to the new way macros are expanded. The new algorithm works (as far as I can tell) by doing a pass through the AST and pulling out all the macro calls into a series of macro invocations, with the original AST being replaced with a placeholder AST. These invocations then are recursively expanded and substituted back into the AST.
The bug stems from how we know where to splice in the new AST. This is done by building a mapping of node ids of the AST to expand to the actual macro. It seems we're somehow the node ids generated by the resolver are colliding with the hygiene system's
Mark::fresh(), which is being used as the placeholder's node id. When we're trying to substitute the newly expanded macros, we end up generating bad code by substituting in the wrong place.This hack makes sure that the node ids are globally unique, but given that rustc doesn't seem to have a problem, I feel this is just obscuring the real bug.
@nrc, @jseyfried, @cgswords: any of you know how rustc avoids this problem?
Also, this also adds the ability to ignore unknown type macros, which was accidentally left out of 0.45.0.
cc @korczis