[PATCH v3 5/9] Start adding the infrastructure for handling TypoExprs.

[Richard Smith]

On Mon, Jul 14, 2014 at 4:55 PM, Kaelyn Takata <rikka at google.com> wrote:

>
> The the typo count is propagated up the stack of
> ExpressionEvaluationContextRecords when one is popped off of to
> avoid accidentally dropping TypoExprs on the floor. For example,
> the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp
> happens with an ExpressionEvaluationContextRecord that is popped off
> the stack prior to ActOnFinishFullExpr being called and the tree
> transform for TypoExprs being run.
>

FWIW, that case sounds like a bug: finalizing the full-expression should
happen within the relevant ExpressionEvaluationContext.

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:

+    assert(NumTypos == 0 && "There are outstanding typos after popping the
"
+                            "last ExpressionEvaluationContextRecord");

... 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.


+  llvm::SmallVector<Expr *, 4> ExprStack;

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?


+      // If corrections for the first TypoExpr have been exhausted, retry
them
+      // against the next combination of substitutions for all of the other
+      // TypoExprs.
+      if (FirstTypoExpr->Consumer->finished()) {
+        unsigned numFinished = 1;
+        for (auto TE : TypoExprs) {
+          if (TE != FirstTypoExpr) {
+            TransformCache.erase(TE);
+            if (TE->Consumer->finished()) {
+              ++numFinished;
+              TE->Consumer->resetCorrectionStream();
+            } else {
+              break;
+            }
+          }
+        }
+        FirstTypoExpr->Consumer->resetCorrectionStream();
+        if (numFinished >= TypoExprs.size())
+          break;
+      }

It looks like the net result of this is that you'll try:

 * Typo 1 correction 1, typo 2 correction 1, typo 3 correction 1
 * Typo 1 correction 2, typo 2 correction 1, typo 3 correction 1
 * ...
 * Typo 1 correction 1, typo 2 correction 2, typo 3 correction 2
 * Typo 1 correction 2, typo 2 correction 2, typo 3 correction 2
 * ...

... 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).

I think we need to decide:
 * how many different combinations of typo corrections we're prepared to
try, and
 * which order to search them in

Due to the tree structure of the AST, we should be able to do better than
trying all combinations; for instance, given:

 f(typo1, g(typo2, typo3))

... we can figure out which combinations work for the call to g, and only
try those combinations in the call to f.


@@ -5964,6 +6063,16 @@ ExprResult Sema::ActOnFinishFullExpr(Expr *FE,
SourceLocation CC,
       return ExprError();
   }

+  if (ExprEvalContexts.back().NumTypos &&
+      (FullExpr.get()->isTypeDependent() ||
+       FullExpr.get()->isValueDependent() ||
+       FullExpr.get()->isInstantiationDependent())) {
+    FullExpr = TransformTypos(*this).Transform(FullExpr.get());
+    ExprEvalContexts.back().NumTypos = 0;
+    if (FullExpr.isInvalid())
+      return ExprError();
+  }

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.


+/// \returns a new \c TypoExpr that will later be replaced in the AST with
an
+/// Expr representing the result of performing typo correction.
+TypoExpr *Sema::CorrectTypoDelayed(

It would be useful to document that the caller still needs to produce a
diagnostic if this returns nullptr.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140725/3202d873/attachment.html>