[Rename] Also syntactically rename a macro’s definition #65399
Conversation
Previously we were skipping the macro’s definition (i.e. the part after `=` in a macro declaration if it wasn’t type checked. Since syntactic rename doesn’t type-check anything, it was thus skippin the macro definition. Add a flag to `ASTWalker` that allows `NameMatcher` to opt-out of this behavior.
ahoppen
requested review from
hborla,
slavapestov,
xedin and
rintaro
as code owners
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Linux |
| @@ -425,7 +425,8 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*, | |||
| if (auto def = MD->definition) { | |||
| // Don't walk into unchecked definitions. | |||
| if (auto expansion = dyn_cast<MacroExpansionExpr>(def)) { | |||
| if (!expansion->getType().isNull()) { | |||
| if (!expansion->getType().isNull() || | |||
| Walker.shouldWalkIntoUncheckedMacroDefinitions()) { | |||
There was a problem hiding this comment.
I suppose this is safer, but do you know what expansion->getType().isNull() was added for? We can walk over un-typechecked ASTs, so it seems like clients should be checking this instead.
There was a problem hiding this comment.
It was added in aac0406. @DougGregor Do you remember why you added it?
There was a problem hiding this comment.
@DougGregor and I just chatted about it and he added this check because of a verified crash and thinks that we can fix it in a more principled way than what he did.
I’m going to merge this as-is because it’s a low-risk change this way that we can cherry-pick to 5.9 and will provide a better fix in a follow-up PR.
There was a problem hiding this comment.
Tracking as rdar://108560477 for reference.
|
@swift-ci Please smoke test |
Previously we were skipping the macro’s definition (i.e. the part after
=in a macro declaration if it wasn’t type checked. Since syntactic rename doesn’t type-check anything, it was thus skippin the macro definition. Add a flag toASTWalkerthat allowsNameMatcherto opt-out of this behavior.rdar://108391854