Better diagnostics for iterators missing ++, !=, and * in range-based for loops.

[panzer]

Summary

This patch adds an additional note when ++begin, !=begin, *__begin, or the loop variable declaration is invalid, stating the high-level source of the error (e.g. "cannot use type T as an iterator; no operator ++ is defined"). Ideally, this would be the error message rather than a note, but this should be suffcient to clarify the existing errors.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

1427–1429

Perhaps this would be clearer if presented as a context-style note:

in implicit call to 'operator!=' for iterator of type %0

That'd also remove the issue that 'no viable operator' isn't necessarily the problem (it could be an ambiguous operator, or an inaccessible destructor, etc).

test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp

51

This note should presumably either say

loop variable 'a' has type 'char *'

or

loop variable 'a' is initialized by a value of type 'int'

Also, in these cases, it's not clear to me that this note is adding much value (since the primary diagnostic already mentions both relevant types). Are there other cases where the value is higher?

[panzer]

Changed wording according to rsmith's suggestions.

[panzer]

Changed the notes as per rsmith's suggestions.

include/clang/Basic/DiagnosticSemaKinds.td

1427–1429

Done. I feel it would be better still as the primary error message, if that becomes possible.

test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp

51

I wasn't sure about this note either, and I can't remember the case where it seemed necessary.

[dblaikie]

Looks like this was committed in r163350

include/clang/Basic/DiagnosticSemaKinds.td

   "when looking up '%select{begin|end}0' function for range expression "
   "of type %1">;
 def err_for_range_invalid: Error<
   "invalid range expression of type %0; no viable '%select{begin|end}1' "
   "function available">;
 def err_for_range_dereference : Error<
   "invalid range expression of type %0; did you mean to dereference it "
   "with '*'?">;
+def note_for_range_invalid_iterator : Note <
+  "cannot use type %0 as iterator in range-based for loop; "
+  "no viable operator %select{!=|*|++}1 is defined">;
+def note_for_range_loop_variable_type : Note <
+  "loop variable %0 has type %1">;
 def note_for_range_begin_end : Note<
   "selected '%select{begin|end}0' %select{function|template }1%2 with iterator type %3">;
 
 // C++11 constexpr
 def warn_cxx98_compat_constexpr : Warning<
   "'constexpr' specifier is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
 def err_invalid_constexpr : Error<

lib/Sema/SemaStmt.cpp

       return StmtError();
 
     // Build and check __begin != __end expression.
     NotEqExpr = ActOnBinOp(S, ColonLoc, tok::exclaimequal,
                            BeginRef.get(), EndRef.get());
     NotEqExpr = ActOnBooleanCondition(S, ColonLoc, NotEqExpr.get());
     NotEqExpr = ActOnFinishFullExpr(NotEqExpr.get());
     if (NotEqExpr.isInvalid()) {
+      Diag(RangeLoc, diag::note_for_range_invalid_iterator)
+        << RangeLoc << BeginRangeRef.get()->getType() << 0 ;
       NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
       if (!Context.hasSameType(BeginType, EndType))
         NoteForRangeBeginEndFunction(*this, EndExpr.get(), BEF_end);
       return StmtError();
     }
 
     // Build and check ++__begin expression.
     BeginRef = BuildDeclRefExpr(BeginVar, BeginRefNonRefType,
                                 VK_LValue, ColonLoc);
     if (BeginRef.isInvalid())
       return StmtError();
 
     IncrExpr = ActOnUnaryOp(S, ColonLoc, tok::plusplus, BeginRef.get());
     IncrExpr = ActOnFinishFullExpr(IncrExpr.get());
     if (IncrExpr.isInvalid()) {
+      Diag(RangeLoc, diag::note_for_range_invalid_iterator)
+        << RangeLoc << BeginRangeRef.get()->getType() << 2;
       NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
       return StmtError();
     }
 
     // Build and check *__begin  expression.
     BeginRef = BuildDeclRefExpr(BeginVar, BeginRefNonRefType,
                                 VK_LValue, ColonLoc);
     if (BeginRef.isInvalid())
       return StmtError();
 
     ExprResult DerefExpr = ActOnUnaryOp(S, ColonLoc, tok::star, BeginRef.get());
     if (DerefExpr.isInvalid()) {
+      Diag(RangeLoc, diag::note_for_range_invalid_iterator)
+        << RangeLoc << BeginRangeRef.get()->getType() << 1;
       NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
       return StmtError();
     }
 
     // Attach  *__begin  as initializer for VD.
     if (!LoopVar->isInvalidDecl()) {
       AddInitializerToDecl(LoopVar, DerefExpr.get(), /*DirectInit=*/false,
                            /*TypeMayContainAuto=*/true);
-      if (LoopVar->isInvalidDecl())
+      if (LoopVar->isInvalidDecl()) {
+        Diag(RangeLoc, diag::note_for_range_loop_variable_type)
+          << RangeLoc << LoopVar->getName() << DerefExpr.get()->getType();
         NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
+      }
     }
   } else {
     // The range is implicitly used as a placeholder when it is dependent.
     RangeVar->setUsed();
   }
 
   return Owned(new (Context) CXXForRangeStmt(RangeDS,
                                      cast_or_null<DeclStmt>(BeginEndDecl.get()),

test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp

 };
 
 void f();
 void f(int);
 
 void g() {
   for (int a : A())
     A __begin;
-  for (char *a : A()) { // expected-error {{cannot initialize a variable of type 'char *' with an lvalue of type 'int'}}
+  for (char *a : A()) { // expected-error {{cannot initialize a variable of type 'char *' with an lvalue of type 'int'}} \
+    expected-note {{loop variable a has type 'int'}}
   }
-  for (char *a : B()) { // expected-error {{cannot initialize a variable of type 'char *' with an lvalue of type 'int'}}
+  for (char *a : B()) { // expected-error {{cannot initialize a variable of type 'char *' with an lvalue of type 'int'}} \
+    expected-note {{loop variable a has type 'int'}}
+
   }
   // FIXME: Terrible diagnostic here. auto deduction should fail, but does not!
   for (double a : f) { // expected-error {{cannot use type '<overloaded function type>' as a range}}
   }
   for (auto a : A()) {
   }
   for (auto a : B()) {
   }
-  for (auto *a : A()) { // expected-error {{variable 'a' with type 'auto *' has incompatible initializer of type 'int'}}
+  for (auto *a : A()) { // expected-error {{variable 'a' with type 'auto *' has incompatible initializer of type 'int'}} \
+    expected-note {{loop variable a has type 'int'}}
+
   }
   // : is not a typo for :: here.
-  for (A NS:A()) { // expected-error {{no viable conversion from 'int' to 'A'}}
+  for (A NS:A()) { // expected-error {{no viable conversion from 'int' to 'A'}} \
+    expected-note {{loop variable NS has type 'int'}}
+
   }
   for (auto not_in_scope : not_in_scope) { // expected-error {{use of undeclared identifier 'not_in_scope'}}
   }
 
   for (auto a : A())
     for (auto b : A()) {
       __range.begin(); // expected-error {{use of undeclared identifier '__range'}}
       ++__begin; // expected-error {{use of undeclared identifier '__begin'}}
void g() {
   struct NoBegin {
     null_t end();
   };
   struct NoEnd {
     null_t begin();
   };
   for (auto u : NoBegin()) { // expected-error {{range type 'NoBegin' has 'end' member but no 'begin' member}}
   }
-  for (auto u : NoEnd()) { // expected-error {{range type 'NoEnd' has 'begin' member but no 'end' member}}
+  for (auto u : NoEnd()) { // expected-error {{range type 'NoEnd' has 'begin' member but no 'end' member}} 
   }
 
   struct NoIncr {
     void *begin(); // expected-note {{selected 'begin' function with iterator type 'void *'}}
     void *end();
   };
-  for (auto u : NoIncr()) { // expected-error {{arithmetic on a pointer to void}}
+  for (auto u : NoIncr()) { // expected-error {{arithmetic on a pointer to void}}\
+    expected-note {{cannot use type 'NoIncr' as iterator in range-based for loop; no viable operator ++ is defined}}
   }
 
   struct NoNotEq {
     NoNotEq begin(); // expected-note {{selected 'begin' function with iterator type 'NoNotEq'}}
     NoNotEq end();
     void operator++();
   };
-  for (auto u : NoNotEq()) { // expected-error {{invalid operands to binary expression}}
+  for (auto u : NoNotEq()) { // expected-error {{invalid operands to binary expression}}\
+    expected-note {{cannot use type 'NoNotEq' as iterator in range-based for loop; no viable operator != is defined}}
+  }
+
+  struct NoDeref {
+    NoDeref begin(); // expected-note {{selected 'begin' function}}
+    NoDeref end();
+    void operator++();
+    bool operator!=(NoDeref &);
+  };
+
+  for (auto u : NoDeref()) { // expected-error {{indirection requires pointer operand}} \
+    expected-note {{cannot use type 'NoDeref' as iterator in range-based for loop; no viable operator * is defined}}
   }
 
   struct NoCopy {
     NoCopy();
     NoCopy(const NoCopy &) = delete;
     int *begin();
     int *end();
   };
   for (int n : NoCopy()) { // ok
   }
 
   for (int n : 42) { // expected-error {{invalid range expression of type 'int'; no viable 'begin' function available}}
   }
 
   for (auto a : *also_incomplete) { // expected-error {{cannot use incomplete type 'struct Incomplete' as a range}}
   }
 }
 
 template<typename T, typename U>
 void h(T t) {
-  for (U u : t) { // expected-error {{no viable conversion from 'A' to 'int'}}
+  for (U u : t) { // expected-error {{no viable conversion from 'A' to 'int'}} \
+    expected-note {{loop variable u has type 'A'}}
   }
   for (auto u : t) {
   }
 }
 
 template void h<A, int>(A);
 template void h<A(&)[4], A &>(A(&)[4]);
 template void h<A(&)[13], A>(A(&)[13]);