[IDE] Ensure every walkToPre call is matched with a walkToPost call in SemaAnnotator #36628
Conversation
| return true; | ||
| return false; |
There was a problem hiding this comment.
Heh. Nathan and I were bamboozled by this one as well. This is actually in a lambda where the true/false is reversed. I'm happy for that to be changed/something else, but the false here actually means "no error". Ie. the check below is:
if (ReportParamList(ParamList)) return false;
So this should be true as is, not false.
There was a problem hiding this comment.
But… why? Before anyone gets hit by this again, I’m inverting the return value of ReportParamList.
There was a problem hiding this comment.
Why indeed 😆. Sounds good to me!
| /// If \c walkTo*Pre returns \c true, the children are visited and \c | ||
| /// walkTo*Post is called after all children have been visited. If \c walkTo*Pre | ||
| /// returns \c false, the corresponding \c walkTo*Post call will not be issued. | ||
| /// Once the traversal is terminated, no more \c walk* calls are issued. Nodes | ||
| /// that have already received a \c walkTo*Pre call will *not* receive a \c | ||
| /// walkTo*Post call. |
There was a problem hiding this comment.
Thanks for adding this comment! I really should have when I did my change in this class since all the true/falses/post being called/not really isn't obvious and took me a while to understand. You could also mention that if walkTo*Post returns false then the walk is cancelled.
There was a problem hiding this comment.
Good call. It’s now included in the comment.
…n SemaAnnotator We had some unbalanced calls of `walkTo*Pre` and `walkTo*Post` in `SemaAnnotator`. The main fix was to set `Cancelled` to `true` if traversal is being stopped in `walkToExprPre`. While I was at it, I also - Added some more checks, ensuring that no more `walkTo*` calls are issued after `Cancelled` has been set to `true`. - Added some comments, describing the intended traversal behaviour. - Inverted the return value of the `ReportParamList` lambda to be in line with the return value of the enclosing `walkToDeclPre` - Moved `walkToExprPost` to be place right after `walkToExprPre` Resolves rdar://64139829 [SR-12957]
ahoppen
force-pushed
the
pr/balanced-pre-post
branch
from
bb93209 to
981b663
Compare
|
@swift-ci Please smoke test Linux |
We had some unbalanced calls of
walkTo*PreandwalkTo*PostinSemaAnnotator, causing the new test case to crash in assert builds.The main fix was to set
Cancelledtotrueif traversal is being stopped inwalkToExprPre.While I was at it, I also
walkTo*calls are issued afterCancelledhas been set totrue.true(i.e. visit children) although we were settingCancelledto falsewalkToExprPostto be place right afterwalkToExprPreResolves rdar://64139829 [SR-12957]