[PATCH] Add more information when displaying a "read-only variable is not assignable" error

[Richard Trieu]

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4817
@@ +4816,3 @@
+  "%select{read-only variable is not assignable|"
+  "cannot assign to return value since function %1 returns a const value|"
+  "cannot assign to variable %1 with const qualified type %2|"
----------------
Richard Smith wrote:
> Please use 'because' rather than 'since'.
Done.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4819-4820
@@ +4818,4 @@
+  "cannot assign to variable %1 with const qualified type %2|"
+  "cannot assign to data member %1 with const qualified type %2|"
+  "cannot assign to data member within const member function %1}0">;
+
----------------
Richard Smith wrote:
> You have "data member" here but "field" below. This should be consistently "field" (or "non-static data member") in both places.
Went with "static data member" and "non-static data member".

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4823
@@ +4822,3 @@
+def note_typecheck_assign_const : Note<
+  "%select{|"
+  "function %1 which returns const qualified type %2 declared here|"
----------------
Richard Smith wrote:
> Can you put something like `ERROR` in here so that we don't just get "note: " if this is somehow produced? (We do this in a few other diagnostics which have %select's with missing options.)
Switched the order of the enum so that ConstUnknown is last.  The note has one less text in the select than the error, which will assert in the diagnostic printer if ConstUnknown is used as the note index.

================
Comment at: lib/Sema/SemaExpr.cpp:8485-8488
@@ -8484,1 +8484,6 @@
 
+// Emit the "read-only variable not assignable" error and print notes to give
+// more information about why the variable is not assignable, such as pointing
+// to the declaration of a const variable, showing that a method is const, or
+// that the funciton is returning a const reference.
+static void DiagnoseConstAssignment(Sema &S, const Expr *E,
----------------
Richard Smith wrote:
> `///` for this comment, please.
Done.

================
Comment at: lib/Sema/SemaExpr.cpp:8526-8533
@@ +8525,10 @@
+    // Class method is const.
+    if (isa<CXXThisExpr>(E)) {
+      if (const DeclContext *DC = S.getFunctionLevelDeclContext()) {
+        MD = dyn_cast<CXXMethodDecl>(DC);
+        if (MD && !MD->isConst()) {
+          MD = nullptr;
+        }
+      }
+    }
+  }
----------------
Richard Smith wrote:
> I don't think this does the right thing for a member reference within a lambda-expression (I think you'll pick up the wrong DC).
Do you have an example of the lambda expressions you think this would not produce the correct diagnostic?  

================
Comment at: lib/Sema/SemaExpr.cpp:8548-8553
@@ +8547,8 @@
+
+  if (FD) {
+    PD << ConstFunction << FD;
+  } else if (VD) {
+    PD << ConstVariable << VD << VD->getType();
+  } else if (ME) {
+    PD << ConstField << ME->getMemberDecl() << ME->getType();
+  } else if (MD) {
----------------
Richard Smith wrote:
> I think `ME` should be first here (notionally, I think that the constness of the field is a more immediate problem than the fact that the function return type or the variable is declared `const`). This will also fix a bug in the current implementation with `mutable` members:
> 
>   struct A { const int n; };
>   struct B { mutable A a; };
>   extern const B b;
>   void f() { b.a.n = 23; }
> 
> This should diagnose that `A::n` is `const`, but I think it'll incorrectly diagnose that `b` is `const` (which is irrelevant because `B::a` is `mutable`).
Notes are now produced in the order of AST node reached.  Which means that:
  get().x.y.z = 5;
will produce notes (if applicable) in order of z, y, x, then finally get().

For the mutable case, it should produce the correct diagnostic now, error on A::n and ignore the const b.

================
Comment at: lib/Sema/SemaExpr.cpp:8562-8588
@@ +8561,29 @@
+
+  if (FD) {
+    S.Diag(FD->getReturnTypeSourceRange().getBegin(),
+           diag::note_typecheck_assign_const)
+        << ConstFunction << FD << FD->getReturnType()
+        << FD->getReturnTypeSourceRange();
+  }
+
+  if (VD) {
+    S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
+        << ConstVariable << VD << VD->getType() << VD->getSourceRange();
+  }
+
+  if (ME) {
+    while (ME) {
+      ValueDecl *VD = ME->getMemberDecl();
+      if (VD->getType().isConstQualified()) {
+        S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
+            << ConstField << VD << VD->getType() << VD->getSourceRange();
+      }
+      ME = dyn_cast<MemberExpr>(ME->getBase());
+    }
+  }
+
+  if (MD) {
+    S.Diag(MD->getLocation(), diag::note_typecheck_assign_const)
+        << ConstMethod << MD << MD->getSourceRange();
+  }
+}
----------------
Richard Smith wrote:
> As noted above, these may be incorrect if we hit a `mutable` on the way.
Done.  No longer error on mutables.

http://reviews.llvm.org/D4479