refactor: preserve spans for rewrites and add empty-print fallback to avoid sink_lets and remove_redundant_let_types #1469
Conversation
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Mostly LGTM, but I'm not super familiar with the code, so probably wait for Andrei to review, too.
Transforms that create new statement wrappers need to preserve source spans for the rewrite system to extract text during recovery. Changes: - Set stmt.span explicitly in transforms when wrapping locals - Extend Splice for Stmt to include attributes from inner nodes - Add fallback in rewrite_at_impl for macro-expanded code Fixes sink_lets and remove_redundant_let_types on libxml2.
| // | ||
| // If the printed text is empty, try extracting the source directly using | ||
| // splice_span() (which includes attributes). If source is available, use that | ||
| // for reparsing instead of the empty pretty-printed text. |
There was a problem hiding this comment.
Could this reinsert back code that the transforms decided to delete?
| self.span | ||
| // Extend statement span to include attributes on the inner node. | ||
| // | ||
| // Transforms like sink_lets set stmt.span = local.span, but attributes are |
There was a problem hiding this comment.
You're fixing sink_lets below, is this also needed?
There was a problem hiding this comment.
My understanding is that the attributes need to sink as well, without this the extracted source code span would just be the let statement but with this we cover the attribute as well and so we'll get a match to the pretty printed code (which includes the attribute)
There was a problem hiding this comment.
We create a bunch of new mk()...local_stmt(...) below, couldn't we copy the attributes there?
There was a problem hiding this comment.
I'm not exactly sure how that would be done, can you put attributes inside a statement? I thought that was contained at a higher level.
There was a problem hiding this comment.
You can put attributes on a statement, wouldn't that work here?
oinoom
changed the title
oinoom
changed the title
pprust can emit
""for statements containingDUMMY_NODE_IDlocals. When the rewrite pipeline reparses that text, it yields zero statements and trips thelone()assertion, crashing sink_lets and remove_redundant_let_types.DUMMY_SPrewrite_at_impladds a fallback: if pprust pretty-printing returns an empty string (seen forDUMMY_NODE_IDlocals), it pulls the original source viaspan_to_snippetbefore reparsing