Macro expansion often produces invalid Span values

[michaelwoerister]

The span of many AST node types is pieced together from the spans of the node's children. Here is an example from Parser::parse_more_binops:

let lhs_span = lhs.span;
let rhs_span = rhs.span;
let binary = self.mk_binary(codemap::respan(cur_op_span, cur_op), lhs, rhs);
let bin = self.mk_expr(lhs_span.lo, rhs_span.hi, binary);

Unfortunately, in the macro expansion phase, this can lead to undesirable results. Consider the following example:

add1!(3);
macro_rules! add1 { ($val:expr) => ( { 1 + $val } ) }

Here lhs and rhs come from completely different contexts. lhs_span comes from the macro-definition-site (lo: 50, hi: 51) while rhs_span comes from the expansion-site (lo: 6, hi:7). Taking lo from lhs and hi from rhs, as the above code does, we end up with a span of lo: 50, hi: 7. This value can't be interpreted in any meaningful way.

A similar error can occur for any kind of AST node where a sub-node is located directly at the border of a parent:

// binary operators
$expr + $expr

// unary operators
! $expr

// closures
|x| $expr

// parameters
fn foo($expr: $ty) {}

// type parameters
fn bar<$ident: $ty>

// type ascription
$expr : $type

// C-style enums
enum Foo {
    $ident = $expr,
}

// derived types
& &mut $ty

// where clauses
fn baz<T>() where T: $ty {}

// call expressions (?)
$e()

// path expressions (?)
$x::foo()

// ... probably others

This invalid Span values may lead to strange error messages and caused at least one ICE (#23115).

[kmcallister]

We should have a library of common span operations that ensures the syntax contexts match. These operations are necessarily partial; there's no sensible way to union spans from two different contexts. But we could introduce a "span set" or "compound span" to handle that.

[michaelwoerister]

My proposal would be to use the context that the defining operator/token for the given AST node is in. For example, for a binary operation AST node, use the context the operator token originally occurs. For the above example, that would mean:

macro_rules! add1 { ($val:expr) => ( { 1 + $val } ) }
                                       ^^^^^^^^
// the span of the (+) AST node lies within the macro-definition,
// regardless where $val comes from

[steveklabnik]

Triage: there's been a lot of work on spans with the new error format, but I'm not close enough to it to know if it fixes this issue.

[jseyfried]

cc #39450, which is on the declarative macros 2.0 roadmap.

@kmcallister @michaelwoerister
The current plan here is to add more span information to "interpolated tokens" (NoDelim token trees in the macros 2.0 framework) and make the parser smart enough to know when to use e.g. the span of the meta-variable $val versus the span where $val came from.

[estebank]

I believe this is minimized (if not out right eliminated) after #47942.

[Mark-Simulacrum]

I'm going to close this in favor of opening more targeted bugs/PRs as we come across them, in particular in light of @estebank's previous comment about this being most likely fixed today.