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

[Richard Smith]

================
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|"
----------------
Please use 'because' rather than 'since'.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4818-4819
@@ +4817,4 @@
+  "cannot assign to return value since function %1 returns a const value|"
+  "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">;
----------------
There's a missing hyphen between "const" and "qualified" in both of these.

================
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">;
+
----------------
You have "data member" here but "field" below. This should be consistently "field" (or "non-static data member") in both places.

================
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|"
----------------
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.)

================
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,
----------------
`///` for this comment, please.

================
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;
+        }
+      }
+    }
+  }
----------------
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).

================
Comment at: lib/Sema/SemaExpr.cpp:8543-8545
@@ +8542,5 @@
+  if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E)) {
+    VD = DRE->getDecl();
+    if (VD && !VD->getType().isConstQualified())
+      VD = nullptr;
+  }
----------------
If we're in a lambda-expression and the variable got constified by lambda-capture, it'd be nice to point to the lambda and suggest that maybe it should be `mutable`. For simple cases, we seem to do that in `CheckForModifiableLvalue`, but not for member access cases. Maybe we should move more of the code from there into here?

================
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) {
----------------
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`).

================
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();
+  }
+}
----------------
As noted above, these may be incorrect if we hit a `mutable` on the way.

http://reviews.llvm.org/D4479