Suggest fix-it ':' when '=' used in for-range-declaration while initializing an auto

[ismailp]

Summary

I patched this during an evening at ACCU 2014 last week, but seems like I forgot to send it.

This is fix for PR19176:

int arr[] = {1, 2, 3, 4};
for (auto i = arr)
  (void)i;

possibly meant:

for (auto i : arr)

This is going to be issued iff for range declaration begins with auto keyword in C++11 mode.

[rsmith]

lib/Parse/ParseStmt.cpp

1479–1484

It's not really OK to use a tentative parse every time you see for (auto. Tentative parses aren't free, and that's a really common pattern.

An alternative approach would be to track (possibly on Declarator) whether we're parsing the first variable declared in a for statement. In that case, when we get to the end of the initializer, if the next token is ), then recover by correcting the = to a :.

That'll do the wrong thing for cases like int x[] = {1, 2, 3}; for (auto x = x) (name lookup will find the wrong x), but it's close enough.

[ismailp]

TentativeParsingAction has been removed. Diagnostics has been moved into ParseDeclarationAfterDeclaratorAndAttributes. The note has been converted into a warning. We are trying to identify a single variable declaration in a for statement where type of variable is auto.

[ismailp]

Ping!

[rsmith]

include/clang/Basic/DiagnosticParseKinds.td

207

This should be an ExtWarn or (strongly preferably) just an Error. I don't see any good reason to support this as an extension.

include/clang/Parse/Parser.h

1731

You don't need this new flag. Use Declarator::isFirstDeclarator instead.

lib/Parse/ParseDecl.cpp

1837

Why check TypeContainsAuto? I don't think that's a good indicator either way.

1839–1840

Please try to avoid follow-on errors here. After giving your error message, perhaps mark Init as invalid, and make sure that the rest of the parsing code handles this without producing spurious diagnostics.

[ismailp]

Richard,

thank you for feedback. Briefly, changes since last revision:

• Replaced warning with an error. This is going to be the only diagnostics emitted for this case.
• TypeContainsAuto is removed from check; I thought auto (strongly) suggests that this is a range-based for, while an int, for example, is not that strong.
• Pass ForRangeInit down to ParseDeclarationAfterDeclaratorAndAttributes to set ForRangeInit::ColonLoc; this tells parser to treat a for statement as a "range-based for".
• Fix-it uses spelling location for replacement to handle macro expansions

[rsmith]

Looks good with some minor tweaks.

lib/Parse/ParseDecl.cpp

1835–1836

Instead of checking for C++11 and ForContext here, just check FRI.

1839–1841

Please don't deviate from the usual convention here -- if we choose to apply typo-corrections within macros, that should be a centralized decision -- just pass in EqualLoc.

1845–1846

Also FRI->RangeExpr = Init; ?

1855–1856

I think the right thing to do here is to add tok::r_paren to the stop set if the context is ForContext.

[ismailp]

Thank you, Richard. Changes:

• Don't use spelling location.
• Add tok::r_paren to stop set.
• Check FRI instead of ForContext and CPlusPlus11.
• Invalidate FRI->RangeExpr

Committed in r208299.

[bkramer]

Silly phab

include/clang/Basic/DiagnosticParseKinds.td

def err_namespace_nonnamespace_scope : Error<
   "namespaces can only be defined in global or namespace scope">;
 def err_nested_namespaces_with_double_colon : Error<
   "nested namespace definition must define each namespace separately">;
 def err_expected_semi_after_attribute_list : Error<
   "expected ';' after attribute list">;
 def err_expected_semi_after_static_assert : Error<
   "expected ';' after static_assert">;
 def err_expected_semi_for : Error<"expected ';' in 'for' statement specifier">;
+def err_single_decl_assign_in_for_range : Error<
+  "range based for statement requires ':' after range declaration">;
 def warn_missing_selector_name : Warning<
   "%0 used as the name of the previous parameter rather than as part "
   "of the selector">,
   InGroup<DiagGroup<"missing-selector-name">>;
 def note_missing_selector_name : Note<
   "introduce a parameter name to make %0 part of the selector">;
 def note_force_empty_selector_name : Note<
   "or insert whitespace before ':' to use %0 as parameter name "

include/clang/Parse/Parser.h

private:
   bool MightBeDeclarator(unsigned Context);
   DeclGroupPtrTy ParseDeclGroup(ParsingDeclSpec &DS, unsigned Context,
                                 bool AllowFunctionDefinitions,
                                 SourceLocation *DeclEnd = nullptr,
                                 ForRangeInit *FRI = nullptr);
   Decl *ParseDeclarationAfterDeclarator(Declarator &D,
                const ParsedTemplateInfo &TemplateInfo = ParsedTemplateInfo());
   bool ParseAsmAttributesAfterDeclarator(Declarator &D);
-  Decl *ParseDeclarationAfterDeclaratorAndAttributes(Declarator &D,
-               const ParsedTemplateInfo &TemplateInfo = ParsedTemplateInfo());
+  Decl *ParseDeclarationAfterDeclaratorAndAttributes(
+      Declarator &D,
+      const ParsedTemplateInfo &TemplateInfo = ParsedTemplateInfo(),
+      ForRangeInit *FRI = nullptr);
   Decl *ParseFunctionStatementBody(Decl *Decl, ParseScope &BodyScope);
   Decl *ParseFunctionTryBlock(Decl *Decl, ParseScope &BodyScope);
 
   /// \brief When in code-completion, skip parsing of the function/method body
   /// unless the body contains the code-completion point.
   ///
   /// \returns true if the function body was skipped.
   bool trySkippingFunctionBody();

lib/Parse/ParseDecl.cpp

if (FRI && (Tok.is(tok::colon) || isTokIdentifier_in())) {
     if (IsForRangeLoop)
       Actions.ActOnCXXForRangeDecl(ThisDecl);
     Actions.FinalizeDeclaration(ThisDecl);
     D.complete(ThisDecl);
     return Actions.FinalizeDeclaratorGroup(getCurScope(), DS, ThisDecl);
   }
 
   SmallVector<Decl *, 8> DeclsInGroup;
-  Decl *FirstDecl = ParseDeclarationAfterDeclaratorAndAttributes(D);
+  Decl *FirstDecl = ParseDeclarationAfterDeclaratorAndAttributes(
+      D, ParsedTemplateInfo(), FRI);
   if (LateParsedAttrs.size() > 0)
     ParseLexedAttributeList(LateParsedAttrs, FirstDecl, true, false);
   D.complete(FirstDecl);
   if (FirstDecl)
     DeclsInGroup.push_back(FirstDecl);
 
   bool ExpectSemi = Context != Declarator::ForContext;
   

 /// [C++]   '(' expression-list ')'
 /// [C++0x] '=' 'default'                                                [TODO]
 /// [C++0x] '=' 'delete'
 /// [C++0x] braced-init-list
 ///
 /// According to the standard grammar, =default and =delete are function
 /// definitions, but that definitely doesn't fit with the parser here.
 ///
-Decl *Parser::ParseDeclarationAfterDeclarator(Declarator &D,
-                                     const ParsedTemplateInfo &TemplateInfo) {
+Decl *Parser::ParseDeclarationAfterDeclarator(
+    Declarator &D, const ParsedTemplateInfo &TemplateInfo) {
   if (ParseAsmAttributesAfterDeclarator(D))
     return 0;
 
   return ParseDeclarationAfterDeclaratorAndAttributes(D, TemplateInfo);
 }
 
-Decl *Parser::ParseDeclarationAfterDeclaratorAndAttributes(Declarator &D,
-                                     const ParsedTemplateInfo &TemplateInfo) {
+Decl *Parser::ParseDeclarationAfterDeclaratorAndAttributes(
+    Declarator &D, const ParsedTemplateInfo &TemplateInfo, ForRangeInit *FRI) {
   // Inform the current actions module that we just parsed this declarator.
   Decl *ThisDecl = 0;
   switch (TemplateInfo.Kind) {
   case ParsedTemplateInfo::NonTemplate:
     ThisDecl = Actions.ActOnDeclarator(getCurScope(), D);
     break;
 
   case ParsedTemplateInfo::Template:
case ParsedTemplateInfo::ExplicitInstantiation: {
     }
   }
 
   bool TypeContainsAuto = D.getDeclSpec().containsPlaceholderType();
 
   // Parse declarator '=' initializer.
   // If a '==' or '+=' is found, suggest a fixit to '='.
   if (isTokenEqualOrEqualTypo()) {
-    ConsumeToken();
+    SourceLocation EqualLoc = ConsumeToken();
 
     if (Tok.is(tok::kw_delete)) {
       if (D.isFunctionDeclarator())
         Diag(ConsumeToken(), diag::err_default_delete_in_multiple_declaration)
           << 1 /* delete */;
       else
         Diag(ConsumeToken(), diag::err_deleted_non_function);
     } else if (Tok.is(tok::kw_default)) {
if (Tok.is(tok::kw_delete)) {
         Actions.CodeCompleteInitializer(getCurScope(), ThisDecl);
         Actions.FinalizeDeclaration(ThisDecl);
         cutOffParsing();
         return 0;
       }
 
       ExprResult Init(ParseInitializer());
 
+      // If this is the only decl in (possibly) range based for statement,
+      // our best guess is that the user meant ':' instead of '='.
+      if (Tok.is(tok::r_paren) && getLangOpts().CPlusPlus11 &&
+          D.getContext() == Declarator::ForContext && D.isFirstDeclarator()) {
+        Diag(EqualLoc, diag::err_single_decl_assign_in_for_range)
+            << FixItHint::CreateReplacement(
+                EqualLoc.isMacroID()
+                    ? PP.getSourceManager().getSpellingLoc(EqualLoc)
+                    : EqualLoc,
+                ":");
+        // We are trying to stop parser from looking for ';' in this for
+        // statement, therefore preventing spurious errors to be issued.
+        FRI->ColonLoc = EqualLoc;
+        Init = ExprError();
+      }
+
       if (getLangOpts().CPlusPlus && D.getCXXScopeSpec().isSet()) {
         Actions.ActOnCXXExitDeclInitializer(getCurScope(), ThisDecl);
         ExitScope();
       }
 
       if (Init.isInvalid()) {
+        if (!FRI || FRI->ColonLoc.isInvalid() || Tok.isNot(tok::r_paren))
           SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
         Actions.ActOnInitializerError(ThisDecl);
       } else
         Actions.AddInitializerToDecl(ThisDecl, Init.take(),
                                      /*DirectInit=*/false, TypeContainsAuto);
     }
   } else if (Tok.is(tok::l_paren)) {
     // Parse C++ direct initializer: '(' expression-list ')'
     BalancedDelimiterTracker T(*this, tok::l_paren);

lib/Parse/ParseStmt.cpp

StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
   } else if (isForInitDeclaration()) {  // for (int X = 4;
     // Parse declaration, which eats the ';'.
     if (!C99orCXXorObjC)   // Use of C99-style for loops in C90 mode?
       Diag(Tok, diag::ext_c99_variable_decl_in_for_loop);
 
     // In C++0x, "for (T NS:a" might not be a typo for ::
     bool MightBeForRangeStmt = getLangOpts().CPlusPlus;
     ColonProtectionRAIIObject ColonProtection(*this, MightBeForRangeStmt);
 
     SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
     StmtVector Stmts;
-    DeclGroupPtrTy DG = ParseSimpleDeclaration(Stmts, Declarator::ForContext,
-                                               DeclEnd, attrs, false,
-                                               MightBeForRangeStmt ?
-                                                 &ForRangeInit : 0);
+    DeclGroupPtrTy DG = ParseSimpleDeclaration(
+        Stmts, Declarator::ForContext, DeclEnd, attrs, false,
+        MightBeForRangeStmt ? &ForRangeInit : nullptr);
     FirstPart = Actions.ActOnDeclStmt(DG, DeclStart, Tok.getLocation());
-
     if (ForRangeInit.ParsedForRangeDecl()) {
       Diag(ForRangeInit.ColonLoc, getLangOpts().CPlusPlus11 ?
            diag::warn_cxx98_compat_for_range : diag::ext_for_range);
 
       ForRange = true;
     } else if (Tok.is(tok::semi)) {  // for (int x = 4;
       ConsumeToken();
     } else if ((ForEach = isTokIdentifier_in())) {

test/Parser/cxx0x-for-range.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -std=c++11 %s 2>&1 | FileCheck %s
 
 template<typename T, typename U>
 struct pair {};
 
 template<typename T, typename U>
 struct map {
   typedef pair<T,U> *iterator;
   iterator begin();
int foo(map<char*,int> &m) {
 
   for (tie(p, n) : m) { // expected-error {{for range declaration must declare a variable}}
     (void)p;
     (void)n;
   }
 
   return n;
 }
+
+namespace PR19176 {
+struct Vector {
+  struct iterator {
+    int &operator*();
+    iterator &operator++();
+    iterator &operator++(int);
+    bool operator==(const iterator &) const;
+  };
+  iterator begin();
+  iterator end();
+};
+
+void f() {
+  Vector v;
+  int a[] = {1, 2, 3, 4};
+  for (auto foo   =     a) // expected-error {{range based for statement requires ':' after range declaration}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:20}:":"
+    (void)foo;
+  for (auto i
+      =
+      v) // expected-error@-1 {{range based for statement requires ':' after range declaration}}
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:8}:":"
+    (void)i;
+#define FORRANGE(v, a) for (DECLVARWITHINIT(v) a)  // expected-note {{expanded from macro}}
+#define DECLAUTOVAR(v) auto v
+#define DECLVARWITHINIT(v) DECLAUTOVAR(v) =  // expected-note {{expanded from macro}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:43-[[@LINE-1]]:44}:":"
+  FORRANGE(i, a) {  // expected-error {{range based for statement requires ':' after range declaration}}
+
+  }
+}
+}