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

[Richard Smith]

================
Comment at: lib/Sema/SemaExpr.cpp:8488
@@ +8487,3 @@
+/// 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,
----------------
Typo 'funciton'.

================
Comment at: lib/Sema/SemaExpr.cpp:8508
@@ +8507,3 @@
+
+  while (true) {
+    if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
----------------
Each iteration of this loop should `IgnoreParenImpCasts` or similar.

================
Comment at: lib/Sema/SemaExpr.cpp:8514
@@ +8513,3 @@
+        if (Field->isMutable()) {
+          break;
+        }
----------------
Can you `assert(DiagnosticEmitted)` here?

================
Comment at: lib/Sema/SemaExpr.cpp:8528
@@ +8527,3 @@
+        }
+        E = ME->getBase();
+        continue;
----------------
Do we need to track whether `.` or `->` was used here? If we have:

  struct A { const int n; };
  extern A *p const;
  p->n = 0;

... we shouldn't produce a note about `p`; its constness is not relevant. And conversely if we have:

  struct B { int n; };
  const B *q;
  q->n = 0;

... then we should produce a note about `q`.

================
Comment at: lib/Sema/SemaExpr.cpp:8549-8570
@@ +8548,24 @@
+    // Member calls
+    if (const CXXMemberCallExpr *Call = dyn_cast<CXXMemberCallExpr>(E)) {
+      if (const MemberExpr *ME = dyn_cast<MemberExpr>(Call->getCallee())) {
+        if (const CXXMethodDecl *MD =
+                dyn_cast<CXXMethodDecl>(ME->getMemberDecl())) {
+          QualType ReturnType = MD->getReturnType();
+          if (ReturnType.getNonReferenceType().isConstQualified()) {
+            if (!DiagnosticEmitted) {
+              S.Diag(Loc, diag::err_typecheck_assign_const)
+                  << ExprRange << ConstFunction << MD;
+              DiagnosticEmitted = true;
+            }
+            S.Diag(MD->getReturnTypeSourceRange().getBegin(),
+                   diag::note_typecheck_assign_const)
+                << ConstFunction << MD << ReturnType
+                << MD->getReturnTypeSourceRange();
+          }
+          E = Call->getCallee();
+          continue;
+        }
+      }
+      break;
+    } // End CXXMemberCallExpr
+
----------------
I'm not sure this special case makes sense as it stands. If I have:

  struct A {};
  struct B {
    const A f();
  } const b;
  void g() {
    b.f() = A();
  }

... then it's not at all relevant that `b` is `const`, but I think I'll get a note telling me that it is. I think the appropriate check here is somewhat more sophisticated; if:

 * we have a member call on a non-static member function, and
 * that function is `const`, and
 * there is a "similar" function with the same name that is not `const`, and
 * the similar function has a non-`const` (or reference-to-non-`const`) return type

... then it matters that the object is `const` (and arguably we don't need to produce a note pointing at the member function).

This seems like a good enhancement to the basic mechanism here, but my preference would be to drop the member call part of this patch and add that enhancement as a separate patch.

================
Comment at: lib/Sema/SemaExpr.cpp:8575
@@ +8574,3 @@
+      const FunctionDecl *FD = CE->getDirectCallee();
+      if (FD->getReturnType().getNonReferenceType().isConstQualified()) {
+        if (!DiagnosticEmitted) {
----------------
Use `FD->getCallResultType().isConstQualified()` here. (I don't think there are any functionality changes from that, but if there are, they would be bugfixes.)

================
Comment at: lib/Sema/SemaExpr.cpp:8587
@@ +8586,3 @@
+      }
+      E = CE->getCallee()->IgnoreImpCasts();
+      continue;
----------------
The constness of the callee doesn't seem relevant here; should probably just `break` at this point.

================
Comment at: lib/Sema/SemaExpr.cpp:8594
@@ +8593,3 @@
+      if (const ValueDecl *VD = DRE->getDecl()) {
+        if (VD->getType().isConstQualified()) {
+          if (!DiagnosticEmitted) {
----------------
It'd be nice to also handle reference-to-const here.

================
Comment at: lib/Sema/SemaExpr.cpp:8611-8615
@@ +8610,7 @@
+          if (MD->isConst()) {
+            if (!DiagnosticEmitted) {
+              S.Diag(Loc, diag::err_typecheck_assign_const)
+                  << ExprRange << ConstMethod << MD;
+              DiagnosticEmitted = true;
+            }
+            S.Diag(MD->getLocation(), diag::note_typecheck_assign_const)
----------------
I worry that we'll diagnose "cannot assign to non-static data member within const member function" in response to:

    struct X { void f() { this = new X; } };

As noted above, perhaps we should track whether we're assigning to the result of `E` or the result of dereferencing the result of `E`.

http://reviews.llvm.org/D4479