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

[Douglas Gregor]

Looks great, thanks! I've committed this as r125722.

	- Doug

On Feb 6, 2011, at 6:05 AM, Richard Smith wrote:

> Hi again,
> 
> On Thu, February 3, 2011 21:05, Douglas Gregor wrote:
>> On Jan 25, 2011, at 10:18 AM, Richard Smith wrote:
>>> 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 a lot for your comments. After some further experimentation, I
> think it would be tricky to get the insert-semicolon fixits reliable
> enough. In particular, cases like this are tricky:
> 
>  for (int n = 0; n < a ++n)
> 
> because clang will have parsed the ++ as postfix. The 'insert =' fixit
> where the LHS is a DeclRefExpr is unreliable since, as you note, the
> initializer (and following semicolon) could simply be missing.
> 
> The attached, revised patch has the fixits and the "expected '='"
> diagnostic removed. It still resolves the problem where an error within a
> for-statement would lead to cascading errors beyond the end of the
> for-statement.
> 
> Thanks,
> Richard<clang-for-2.diff>