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

[Richard Trieu]

================
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,
----------------
rsmith wrote:
> Typo 'funciton'.
Fixed.

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

================
Comment at: lib/Sema/SemaExpr.cpp:8514
@@ +8513,3 @@
+        if (Field->isMutable()) {
+          break;
+        }
----------------
rsmith wrote:
> Can you `assert(DiagnosticEmitted)` here?
Oops, will get this on next patch.

================
Comment at: lib/Sema/SemaExpr.cpp:8528
@@ +8527,3 @@
+        }
+        E = ME->getBase();
+        continue;
----------------
rsmith wrote:
> 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`.
Dereference is now tracked by `IsDereference` and `NextIsDereference`.  Those examples have been added to the tests.

================
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
+
----------------
rsmith wrote:
> 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.
Testing:

```
struct A {};
struct B {
  const A f() const;
} const b = {};
void g() {
  b.f() = A();
}
```
The error message is no viable overload since the implicit move and copy assignment operators for A are not marked const, which is a different code path.

Testing:

```
struct B {
    const int f() const;
} const b = {};
void g() {
    b.f() = 5;
}
```
Gives: expression is not assignable, also a different code path.

Testing:

```
struct B {
    const int& f() const;
} const b = {};
void g() {
    b.f() = 5;
}
```
Gives:
```
const.cc:65:11: error: cannot assign to return value because function 'f'
      returns a const value
    b.f() = 5;
    ~~~~~ ^
const.cc:62:11: note: function 'f' which returns const-qualified type
      'const int &' declared here
    const int& f() const;
          ^~~~
1 error generated.
```
Which does not show a message for `b` at all.

================
Comment at: lib/Sema/SemaExpr.cpp:8575
@@ +8574,3 @@
+      const FunctionDecl *FD = CE->getDirectCallee();
+      if (FD->getReturnType().getNonReferenceType().isConstQualified()) {
+        if (!DiagnosticEmitted) {
----------------
rsmith wrote:
> Use `FD->getCallResultType().isConstQualified()` here. (I don't think there are any functionality changes from that, but if there are, they would be bugfixes.)
Call to `getNonReferenceType()` is needed otherwise
```
const int& foo();
foo() = 5;
```
would fall through to the default error message below.

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

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

================
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)
----------------
rsmith wrote:
> 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`.
Dereferences are checked now.  In this cases, assigning to `this` pointer causes a different error to be emitted:
```
const.cc:73:28: error: expression is not assignable
struct X { void f() { this = new X; } };
                      ~~~~ ^
```

http://reviews.llvm.org/D4479

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/