<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 25, 2014 at 2:27 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div class="">On Mon, Jul 14, 2014 at 4:55 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
The the typo count is propagated up the stack of<br>
ExpressionEvaluationContextRecords when one is popped off of to<br>
avoid accidentally dropping TypoExprs on the floor. For example,<br>
the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp<br>
happens with an ExpressionEvaluationContextRecord that is popped off<br>
the stack prior to ActOnFinishFullExpr being called and the tree<br>
transform for TypoExprs being run.<br></blockquote><div><br></div></div><div>FWIW, that case sounds like a bug: finalizing the full-expression should happen within the relevant ExpressionEvaluationContext.</div></div></div>
</div></blockquote><div><br></div><div>I don't know enough about what the intended mechanics/interactions of these within clang is intended to be to say for sure, but my recent experiences with them suggests it isn't a bug. As I understand it, the declaration of b and its initialization based on the call to g() is the full expression within an expression evaluation context, but the the initializer (the call to g()) would not and should not include the declaration of b and so is given its own (sub-)expression evaluation context doesn't span the full expression or include the declaration of b.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>Is a simple count of TypoExprs sufficient here? If we leave the local expression without diagnosing a typo (or if our RecursiveASTVisitor fails to diagnose it), I think we still need to track it so that we can produce some kind of diagnostic for it. In particular:</div>
<div><br></div><div><div>+ assert(NumTypos == 0 && "There are outstanding typos after popping the "</div><div>+ "last ExpressionEvaluationContextRecord");</div></div>
<div><br></div><div>... I think there are probably cases where this assertion can fire, and the right thing to do seems to be to issue a basic 'unknown identifier' diagnostic for any remaining typos.</div></div></div>
</div></blockquote><div><br></div><div>IIRC, the assertion has actually been useful when hooking up the delayed typo correction to a primary caller of DiagnoseEmptyLookup in that it has helped me find cases where ActOnFinishFullExpr aren't ever called. However, now that Sema will be keeping track of all of the undiagnosed TypoExprs and their associated state to remove the layering violations in TypoExpr, this tracking of whether an Expr in a given evaluation context might need the tree transform run on it needs to be reworked.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>+ llvm::SmallVector<Expr *, 4> ExprStack;<br></div><div><br></div><div>This seems rather small; I expect we'd walk through more Exprs than this. But... you don't actually use this. Can you remove it?</div>
</div></div></div></blockquote><div><br></div><div>It is used in patch #8 for passing the parent Expr of the TypoExpr to the recovery callback, so that e.g. the number of arguments the TypoExpr is being called with can be known.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div><br></div><div><div>+ // If corrections for the first TypoExpr have been exhausted, retry them</div><div>+ // against the next combination of substitutions for all of the other</div><div>+ // TypoExprs.</div>
<div>+ if (FirstTypoExpr->Consumer->finished()) {</div><div>+ unsigned numFinished = 1;</div><div>+ for (auto TE : TypoExprs) {</div><div>+ if (TE != FirstTypoExpr) {</div><div>+ TransformCache.erase(TE);</div>
<div>+ if (TE->Consumer->finished()) {</div><div>+ ++numFinished;</div><div>+ TE->Consumer->resetCorrectionStream();</div><div>+ } else {</div><div>+ break;</div>
<div>+ }</div><div>+ }</div><div>+ }</div><div>+ FirstTypoExpr->Consumer->resetCorrectionStream();</div><div>+ if (numFinished >= TypoExprs.size())</div><div>+ break;</div>
<div>+ }</div></div><div><br></div><div>It looks like the net result of this is that you'll try:</div><div><br></div><div> * Typo 1 correction 1, typo 2 correction 1, typo 3 correction 1</div><div> * Typo 1 correction 2, typo 2 correction 1, typo 3 correction 1</div>
<div> * ...</div><div> * Typo 1 correction 1, typo 2 correction 2, typo 3 correction 2</div><div> * Typo 1 correction 2, typo 2 correction 2, typo 3 correction 2<br></div><div> * ...</div><div><br></div><div>... and you'll never try typo 2 correction 2 with typo 3 correction 1. (Also, if typo 2 has 2 corrections and typo 3 has 3, you'll never consider the third correction for typo 3).</div>
</div></div></div></blockquote><div><br></div><div>I believe you are forgetting to account for the TransformCache which keeps the cached TypoExprs from advancing Unless I screwed up the logic, the net result should be e.g. (assuming typo 1 has 3 candidates, typo 2 has 2, and typo 3 has 4):</div>
<div><br></div><div> * Typo 1 correction 1, typo 2 correction 1, typo 3 correction 1</div><div> * Typo 1 correction 2, typo 2 correction 1, typo 3 correction 1</div><div> * Typo 1 correction 3, typo 2 correction 1, typo 3 correction 1</div>
<div> * Typo 1 correction 1, typo 2 correction 2, typo 3 correction 1</div><div> * Typo 1 correction 2, typo 2 correction 2, typo 3 correction 1<br></div><div><div> * Typo 1 correction 3, typo 2 correction 2, typo 3 correction 1</div>
</div><div><div> * Typo 1 correction 1, typo 2 correction 1, typo 3 correction 2</div><div> * Typo 1 correction 2, typo 2 correction 1, typo 3 correction 2</div><div> * Typo 1 correction 3, typo 2 correction 1, typo 3 correction 2</div>
<div> * Typo 1 correction 1, typo 2 correction 2, typo 3 correction 2</div><div> * Typo 1 correction 2, typo 2 correction 2, typo 3 correction 2<br></div><div> * Typo 1 correction 3, typo 2 correction 2, typo 3 correction 2</div>
</div><div><div> * Typo 1 correction 1, typo 2 correction 1, typo 3 correction 3</div><div> * Typo 1 correction 2, typo 2 correction 1, typo 3 correction 3</div><div> * Typo 1 correction 3, typo 2 correction 1, typo 3 correction 3</div>
<div> * Typo 1 correction 1, typo 2 correction 2, typo 3 correction 3</div><div> * Typo 1 correction 2, typo 2 correction 2, typo 3 correction 3<br></div><div> * Typo 1 correction 3, typo 2 correction 2, typo 3 correction 3</div>
<div><div> * Typo 1 correction 1, typo 2 correction 1, typo 3 correction 4</div><div> * Typo 1 correction 2, typo 2 correction 1, typo 3 correction 4</div><div> * Typo 1 correction 3, typo 2 correction 1, typo 3 correction 4</div>
<div> * Typo 1 correction 1, typo 2 correction 2, typo 3 correction 4</div><div> * Typo 1 correction 2, typo 2 correction 2, typo 3 correction 4<br></div><div> * Typo 1 correction 3, typo 2 correction 2, typo 3 correction 4</div>
</div></div><div><br></div><div>The code:</div><div> 1) notes the first TypoExpr encountered</div><div> 2) for each TypoExpr encountered, if is the first TypoExpr or it isn't in the cache, goes through the standard logic for choosing a new candidate and caches the result, otherwise returns the cached result</div>
<div> 3) then once the first TypoExpr is finished, resets it and removes the next TypoExpr from the cache so a new result will be generated for that TypoExpr, and if that TypoExpr is also finished resets it and removes the following TypoExpr from the cache, repeating until a TypoExpr that wasn't finished is removed from the cache or all TypoExprs were seen to be finished.</div>
<div><br></div><div>Since this seems to not be as clear/obvious as I thought, shall I add a brief explanation about how the retrying interacts with the caching to the comment on the loop?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>I think we need to decide:</div><div> * how many different combinations of typo corrections we're prepared to try, and</div><div> * which order to search them in</div></div></div></div></blockquote>
<div><br></div><div>Right now the code tries all possible combinations that passed validation, in a depth-first manner.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Due to the tree structure of the AST, we should be able to do better than trying all combinations; for instance, given:</div>
<div><br></div><div> f(typo1, g(typo2, typo3))</div><div><br></div><div>... we can figure out which combinations work for the call to g, and only try those combinations in the call to f.</div></div></div></div></blockquote>
<div><br></div><div>Unless the tree transform code doesn't short-circuit the transform once an ExprError is encountered, the existing code already does this to a large degree (and only for the subset of possible corrections that CorrectTypo would have returned anyway). Given that having more than one or two typos in a single full-expression seems to be quite uncommon, I suspect that trying to be any more clever about which sub-combinations do or don't work to avoid trying all combinations in this error path will not yield sufficient gains to warrant the additional design effort and code complexity.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div>
<div><div>@@ -5964,6 +6063,16 @@ ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC,</div><div> return ExprError();</div><div> }</div><div> </div><div>+ if (ExprEvalContexts.back().NumTypos &&</div>
<div>+ (FullExpr.get()->isTypeDependent() ||</div><div>+ FullExpr.get()->isValueDependent() ||</div><div>+ FullExpr.get()->isInstantiationDependent())) {</div><div>+ FullExpr = TransformTypos(*this).Transform(FullExpr.get());</div>
<div>+ ExprEvalContexts.back().NumTypos = 0;</div><div>+ if (FullExpr.isInvalid())</div><div>+ return ExprError();</div><div>+ }</div></div><div><br></div><div>This is assuming that any call to TransformTypos::Transform will remove all TypoExprs within the entire ExpressionEvaluationContext. I am not sure that this will necessarily be true in general. It'd be more robust for TransformTypos to specify which TypoExprs it transformed, and to remove those from the ExpressionEvaluationContext or from a list of currently undiagnosed TypoExprs.</div>
</div></div></div></blockquote><div><br></div><div>As mentioned, the count tracking needs to be reworked now that Sema will be maintaining the set of undiagnosed TypoExprs anyway, and what I have in mind for fixing it will also solve this problem (I knew the current count tracking was quite fragile, but it was sufficient for hooking the delayed typo correction to member lookup and I didn't want to put a lot of effort into wiring up precise tracking of the count when large parts of the code may be changing anyway--and with Sema now maintaining a map of undiagnosed TypoExprs to their associated Sema-specific state, I'm glad I hadn't tried to add a bunch of fancy typo count tracking).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div><br></div><div><div>+/// \returns a new \c TypoExpr that will later be replaced in the AST with an</div><div>+/// Expr representing the result of performing typo correction.</div><div>+TypoExpr *Sema::CorrectTypoDelayed(</div>
<div><br></div></div><div>It would be useful to document that the caller still needs to produce a diagnostic if this returns nullptr.</div></div></div></div>
</blockquote></div><br></div><div class="gmail_extra">Actually, I think it may make more sense to invoke the TypoDiagnosticGenerator with an empty correction before returning nullptr instead of foisting that responsibility on the caller.</div>
</div>