patch: fix crash in typo correction when TransformTypo creates a new delayed TypoExpr

[Kaelyn Takata]

On Mon, Dec 15, 2014 at 9:19 PM, Nick Lewycky <nlewycky at google.com> wrote:

> The attached patch fixes PR21925. I think the proximate cause of the crash
> is this code in CorrectDelayedTyposInExpr:
>
>     auto TyposResolved = DelayedTypos.size();
>     auto Result = TransformTypos(*this, Filter).Transform(E);
>     TyposResolved -= DelayedTypos.size();
>     if (TyposResolved) {
>       ExprEvalContexts.back().NumTypos -= TyposResolved;
>       return Result;
>     }
>
> When TransformTypos visits the member expression, it converts
> "typoA.typoB()" into "notypo.typoB()" and produces a new TypoExpr for
> typoB. I'm not certain whether TransformTypos ought to be creating new
> TypoExprs at all, but if it does then we see "DelayedTypos.size()" is 1
> before and 1 after, so we end up returning the original expression instead
> of the transformed one. We'll then try to visit it again later and the
> assertion fires as we try to type-correct "typoA" again.
>
> My proposed fix is to check whether the resulting expr is the same after
> transformation. There's might be two problems here. Is it possible that a
> subexpr was modified in place instead of being replaced? If so we'll
> accidentally throw away the correction. Also, what should we do if the new
> ExprResult is invalid? Currently we ignore the "invalid bit" and just look
> at the pointer it contains (in my experiments I see a 0x1 on an invalid
> return from TransformTypos which doesn't equal E so we propagate that
> invalid along).
>

The patch and the approach look good to me. As long as the comparison does
the right thing, checking whether the ExprResult is invalid shouldn't be
necessary (we'd want to always return the invalid result as it means that
the typo correction failed). And as I understand the TreeTransform, it
doesn't destructively modify the original Expr tree so having a
subexpression change but the outer expression remain the same wouldn't be
possible.

>
> But my higher-level question is why we only built one delayed TypoExpr and
> then created another one part way through transforming it? Is it just that
> delaying resolution of member-expr typos hasn't been implemented yet? I
> would expected to see some form of dependent member expr with two typo
> exprs on either side, then work through the possibilities with the
> streaming consumer interface.
>

I would have thought it worked the way you expected too... I'm guessing
it's a side-effect of how member resolution works that is causing this
(i.e. the second TypoExpr wasn't created yet because until the first is
resolved, there is no way to know if the member even needs typo
correction--particularly in the case of an -> instead of a . and the
possibility of overloaded operator-> resolution needing to be done since .
and -> are mostly handled by the same code path). That this went unnoticed
is another unfortunate example of a recovery path not covered by any
existing unit tests.

Now the tricky part would be getting it to dtrt in the case where the
member reference itself could be used to choose a better correction (e.g.
in "var1.foo()" correcting it to var3 instead of var2 because "var3.foo()"
is a better match than "var2.Foo()", or even correcting "fizbin.bar()" to
"FizBin.bar()" because "fizBin.bar()" isn't viable even after trying to
typo correct "bar"). ;) But obviously that's out of the scope of your patch.

>
> Nick
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141216/42c9b843/attachment.html>