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

[Richard Smith]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-for-2.diff
Type: text/x-patch
Size: 2655 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110206/e6e9b433/attachment.bin>