[AST] Introduce explicit actions for ASTWalker #60955
Conversation
|
@bnbarham You may have inadvertently nerd sniped me into doing this 😄 |
|
@swift-ci please test source compatibility |
|
@swift-ci please test |
|
@swift-ci please SourceKit stress test |
bnbarham
left a comment
bnbarham
left a comment
There was a problem hiding this comment.
Thanks so much @hamishknight, this makes everything so much clearer!
I went through around half and stopped at all the Sema changes. Generally it seems there's a bunch of places we can now use stop rather than keeping a separate member, but given the size of the change I'd prefer those in another PR anyway.
lib/IDE/SwiftSourceDocInfo.cpp
Outdated
There was a problem hiding this comment.
Same as above I'd say (ie. yes).
There was a problem hiding this comment.
Yeah, I think I'd like to go through and fix these up in a follow-up
lib/IDE/Formatting.cpp
Outdated
There was a problem hiding this comment.
I'd hope tests would catch this if not, based on the rest of the class it does seem like we mean "stop" here rather than "skip". Worth trying, but also happy to do that in a separate PR given the size of this one.
lib/IDE/SwiftSourceDocInfo.cpp
Outdated
There was a problem hiding this comment.
shouldSkip doesn't touch isDone, so I wonder if isDone was supposed to be checked in the other branch as well 🤔. Probably doesn't really matter, I imagine we'll remove all the isDone after anyway.
There was a problem hiding this comment.
Yeah, for now I'd like to try and leave this patch as NFC as possible (modulo 26bd877), I can look into this in a follow-up (and hopefully eliminate isDone() altogether).
hamishknight
force-pushed
the
walk-this-way
branch
2 times, most recently
from
1a15b3d to
248460e
Compare
|
GitHub messed up the timestamps of the force push, https://github.com/apple/swift/compare/248460e3bc599a9d59bbe66d8da9873a4fc72a66..1f96321d81fc6612eaf4ecd580a05ea7ca9a4d32 is the right diff |
hamishknight
force-pushed
the
walk-this-way
branch
from
1f96321 to
fd2246b
Compare
`walkToTypeReprPost` should return `true` to keep walking, so we really want to check if the `offendingType` hasn't been found yet. Otherwise we'll bail as soon as we visit the first type.
Turns out we were getting away with dereferencing `nullptr` in a few cases as `walk` would use `nullptr` to indicate that the walk should be stopped, and luckily Clang didn't optimize it to something broken. This commit is fairly defensive and sprinkles some null checks for calls to `walk` directly on a body of a function or top-level code decl.
Replace the use of bool and pointer returns for `walkToXXXPre`/`walkToXXXPost`, and instead use explicit actions such as `Action::Continue(E)`, `Action::SkipChildren(E)`, and `Action::Stop()`. There are also conditional variants, e.g `Action::SkipChildrenIf`, `Action::VisitChildrenIf`, and `Action::StopIf`. There is still more work that can be done here, in particular: - SourceEntityWalker still needs to be migrated. - Some uses of `return false` in pre-visitation methods can likely now be replaced by `Action::Stop`. - We still use bool and pointer returns internally within the ASTWalker traversal, which could likely be improved. But I'm leaving those as future work for now as this patch is already large enough.
hamishknight
force-pushed
the
walk-this-way
branch
from
fd2246b to
4716f61
Compare
|
@swift-ci please test |
|
🚢 |
2 participants
Replace the use of bool and pointer returns for
walkToXXXPre/walkToXXXPost, and instead use explicit actions such asAction::Continue(E),Action::SkipChildren(E), andAction::Stop(). There are also conditional variants, e.gAction::SkipChildrenIf,Action::VisitChildrenIf, andAction::StopIf.There is still more work that can be done here, in particular:
return falsein pre-visitation methods can likely now be replaced byAction::Stop.But I'm leaving those as future work for now as this patch is already large enough.