swift-syntax icon indicating copy to clipboard operation
swift-syntax copied to clipboard

Copying a node breaks its source location

Open grynspan opened this issue 2 years ago • 2 comments

Description

When building a macro, it is commonplace to take a syntax node from the original tree and copy it into a new position, e.g. by constructing a TupleExprElementSyntax around an existing ExprSyntax to make a function call argument. Doing so causes the copy of the ExprSyntax to lose its source location information. Subsequently, using MacroExpansionContext.location(of:) will fail, and emitting instances of Diagnostic with a position value equal to the copied ExprSyntax's position (etc.) property will result in a source location for the diagnostic of ":0".

To be clear, I'm not saying I expect the constructed TupleExprElementSyntax to have valid source location information, but the copy of the ExprSyntax would need it. Without that information, we must actively maintain some sort of mapping back to the original ExprSyntax in order to correctly diagnose issues during macro expansion.

Steps to Reproduce

No response

grynspan avatar Jul 28 '23 15:07 grynspan

Tracked in Apple’s issue tracker as rdar://113036932

ahoppen avatar Jul 28 '23 16:07 ahoppen

For some context, a syntax node in SwiftSyntax doesn’t have a source location by itself. Its location is an emergent property of where it occurs in the tree. It is thus expected that, if en aExprSyntax gets copied into a TupleExprElementSyntax, it has a different source location (because it now occurs inside a different tree). The macro expansion detects this and realizes that the source location is anchored in a different tree than the original source, and thus produces the diagnostic at location :0 because that’s the best it can do in the absence of a source location within the original source code.


Maintaining a mapping from nodes in the edited tree to nodes in the original tree will be a tricky task and if we wanted to do it, we’d need to run thorough performance testing to make sure it doesn’t regress cases where we don’t need the mapping back to the original tree.

ahoppen avatar Jul 28 '23 20:07 ahoppen