[cfe-commits] [PATCH] Improved error recovery in for-statement

[Douglas Gregor]

Hi Richard,

On Jan 25, 2011, at 10:18 AM, Richard Smith wrote:

> Hi,
> 
> The attached patch improves diagnostics error recovery in for-statements.
> Examples:
> 
> [snip examples]

> The patch can be viewed online here: http://codereview.appspot.com/3992046/
> 
> Please let me know what I need to fix in order to get this committed.


Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp	(revision 124070)
+++ lib/Parse/ParseStmt.cpp	(working copy)
@@ -14,6 +14,8 @@
 
 #include "clang/Parse/Parser.h"
 #include "RAIIObjectsForParser.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Decl.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/PrettyDeclStackTrace.h"
 #include "clang/Sema/Scope.h"
@@ -1038,8 +1040,22 @@
       }
       Collection = ParseExpression();
     } else {
-      Diag(Tok, diag::err_expected_semi_for);
-      SkipUntil(tok::semi);
+      // suggest 'expected "="' if the declaration's last declarator lacks
+      // an initializer. eg. for (int n 0; n < 10; ++n)
+      VarDecl *VD = DG.get().isNull() ? 0 : dyn_cast<VarDecl>(*(DG.get().end() - 1));
+      if (VD && !VD->hasInit()) {
+        Diag(Tok, diag::err_expected_equals_for)
+          << FixItHint::CreateInsertion(Tok.getLocation(), "=");
+        ExprResult Init = ParseExpression();
+        VD->setInit(Init.take());
+      }

There are a couple problems with this… first of all, we shouldn't suggest a fix-it unless we're fairly certain that it's correct. The '=' here is a bit too much of a guess, since we have no idea that we're going to get two expressions in a row. The user might have forgotten the initializer completely, rather than forgetting the semicolon. Perhaps a bit more lookahead would make it possible to distinguish these cases, so that the Fix-It can be right almost all of the time (which is our criteria for this feature).

Second, just setting the initializer with VD->setInit() is going to cause huge problems down the line, because the initializer needs to be checked and converted by Sema. At the very least, we'd need a call to Sema::AddInitializerToDecl. However, this happens too late in semantic analysis (after Sema::ActOnUnitializedDecl is called) for this to actually work. For example, we will already have complained if the variable has reference type or has no valid default constructor.

+      if (Tok.is(tok::semi)) {
+        ConsumeToken();
+      } else {
+        // eg. for (int n = 0l n < 10; ++n)
+        Diag(Tok, diag::err_expected_semi_for)
+          << FixItHint::CreateInsertion(Tok.getLocation(), "; ");
+      }
     }

Again, we need a higher degree of confidence that inserting the semicolon is actually the right thing to do.

@@ -1065,8 +1081,30 @@
       }
       Collection = ParseExpression();
     } else {
-      if (!Value.isInvalid()) Diag(Tok, diag::err_expected_semi_for);
-      SkipUntil(tok::semi);
+      if (!Value.isInvalid()) {
+        // 'expected "="' if the expression is a DeclRefExpr: for (n 0; n < 10; ++n)
+        if (isa<DeclRefExpr>(Value.get())) {
+          SourceLocation TokLocation = Tok.getLocation();
+          Diag(Tok, diag::err_expected_equals_for)
+            << FixItHint::CreateInsertion(TokLocation, "=");
+          ExprResult Init = ParseExpression();
+          if (!Init.isInvalid()) {
+            Value = Actions.ActOnBinOp(getCurScope(), TokLocation, tok::equal,
+                                       Value.take(), Init.take());
+            FirstPart = Actions.ActOnExprStmt(Actions.MakeFullExpr(Value.get()));
+          }
+        }

Same general comment; we need more lookahead before we can determine that the user actually meant to write the assignment in here. The use of ActOnBinOp/ActOnExprStmt is great here, though, because it's doing semantic analysis to recovery as if the user had written the '='.

Thanks for working on this!

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110203/00edbe6b/attachment.html>