<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 15, 2014 at 9:19 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">The attached patch fixes PR21925. I think the proximate cause of the crash is this code in CorrectDelayedTyposInExpr:<div><br></div><div><div>    auto TyposResolved = DelayedTypos.size();</div><div>    auto Result = TransformTypos(*this, Filter).Transform(E);</div><div>    TyposResolved -= DelayedTypos.size();</div><div>    if (TyposResolved) {</div><div>      ExprEvalContexts.back().NumTypos -= TyposResolved;</div><div>      return Result;</div><div>    }</div></div><div><br></div><div>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.</div><div><br></div><div>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).</div></div></blockquote><div><br></div><div>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.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>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.</div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class="HOEnZb"><font color="#888888"><div><br></div><div>Nick</div><div><br></div></font></span></div>
</blockquote></div><br></div></div>