Page MenuHomePhabricator

Add more information when displaying a "read-only variable is not assignable" error
ClosedPublic

Authored by rtrieu on Jul 11 2014, 6:57 PM.

Details

Summary

http://llvm.org/bugs/show_bug.cgi?id=19139

When a "read-only variable is not assignable" is emitted, give additional notes to show where the const is location.

$ cat test.cc
const int k = 1;
void f() { k = 0; }
class X {
  int i;
  const int j = 1;
  void f() const { i = 0; }
  void g() { j = 0; }
};
$ old_clang test.cc -std=c++11
test.cc:2:14: error: read-only variable is not assignable
void f() { k = 0; }
           ~ ^
test.cc:6:22: error: read-only variable is not assignable
  void f() const { i = 0; }
                   ~ ^
test.cc:7:16: error: read-only variable is not assignable
  void g() { j = 0; }
             ~ ^
3 errors generated.
$ new_clang test.cc -std=c++11
test.cc:2:14: error: read-only variable is not assignable
void f() { k = 0; }
           ~ ^
test.cc:1:11: note: variable 'k' is declared here with type 'const int' which is const
const int k = 1;
~~~~~~~~~~^~~~~
test.cc:6:22: error: read-only variable is not assignable
  void f() const { i = 0; }
                   ~ ^
test.cc:6:8: note: method 'f' is declared const here
  void f() const { i = 0; }
  ~~~~~^~~~~~~~~
test.cc:7:16: error: read-only variable is not assignable
  void g() { j = 0; }
             ~ ^
test.cc:5:13: note: field 'j' is declared here with type 'const int' which is const
  const int j = 1;
  ~~~~~~~~~~^~~~~
3 errors generated.

Diff Detail

Repository
rL LLVM

Event Timeline

rtrieu updated this revision to Diff 11334.Jul 11 2014, 6:57 PM
rtrieu retitled this revision from to Add more information when displaying a "read-only variable is not assignable" error.
rtrieu updated this object.
rtrieu edited the test plan for this revision. (Show Details)
rtrieu updated this object.
rtrieu added subscribers: Unknown Object (MLST), nlewycky, echristo.

An out-of-line definition might be interesting:

struct foo {
  int i;
  void f() const;
};
void foo::f() cont {
  i = 3;
}

does the diagnostic point to the definition of 'f' or the declaration?

Also, would it be better for the diagnostic to point to the "const" in the function rather than the "f"?

"of type 'const T' which is const" sounds a bit strange - any better way we could phrase this? Could we leverage/improve type diffing to allow a silent (silent in the sense that the non-const type is not printed in the diagnostic) comparison to the non-const type? ("X is declared with type '<const> int' here"?) Not sure how it would/should look when typedefs are involved.

$ cat c.cc
struct foo {
  int i;
  void f() const;
};
void foo::f() cont {
  i = 3;
}
$ clang c.cc
c.cc:6:5: error: read-only variable is not assignable
  i = 3;
  ~ ^
c.cc:5:11: note: method 'f' is declared const here
void foo::f() const {
~~~~~~~~~~^~~~~~~~~
1 error generated.

Note will point to function definition.

$ cat d.cc
typedef const int X;
void test(X x) {
  x = 5;
}
$ clang d.cc
d.cc:3:5: error: read-only variable is not assignable
  x = 5;
  ~ ^
d.cc:2:13: note: variable 'x' is declared here with type 'X' (aka 'const int') which is const
void test(X x) {
          ~~^
1 error generated.

No problems with typedefs.

I think the note should explicitly point out the const qualifier, otherwise the user will see "note: variable 'x' has type 'y'" which isn't very helpful. Perhaps "note: variable "x" is declared here with type 'const int'; const qualifier prevents assignment" would be better?

Also, would it be better for the diagnostic to point to the "const" in the function rather than the "f"?

Probably, but the location of the const on a function does not appear to be available.

In D4479#7, @rtrieu wrote:

Also, would it be better for the diagnostic to point to the "const" in the function rather than the "f"?

Probably, but the location of the const on a function does not appear to be available.

Mr. Smith - any thoughts on whether this should be available? Or is it just not feasible due to AST size costs? In any case, if it's not available, that's OK for this commit & maybe leave a FIXME in the test and in the source suggesting that it could be improved if we get that source fidelity in the AST some day.

In D4479#6, @rtrieu wrote:
$ cat c.cc
struct foo {
  int i;
  void f() const;
};
void foo::f() cont {
  i = 3;
}
$ clang c.cc
c.cc:6:5: error: read-only variable is not assignable
  i = 3;
  ~ ^
c.cc:5:11: note: method 'f' is declared const here
void foo::f() const {
~~~~~~~~~~^~~~~~~~~
1 error generated.

Note will point to function definition.

Great (not sure about the "declared const here" phrasing, though - technically true (a definition is a declaration, though most people think of the first declaration as "the declaration") and I can't think of much better, though)

$ cat d.cc
typedef const int X;
void test(X x) {
  x = 5;
}
$ clang d.cc
d.cc:3:5: error: read-only variable is not assignable
  x = 5;
  ~ ^
d.cc:2:13: note: variable 'x' is declared here with type 'X' (aka 'const int') which is const
void test(X x) {
          ~~^
1 error generated.

No problems with typedefs.

I think the note should explicitly point out the const qualifier, otherwise the user will see "note: variable 'x' has type 'y'" which isn't very helpful.

Depends if the 'y' has the word 'const' in it, in which case it's fairly helpful. Though that's why I was suggesting possibly repurposing the type diffing code to highlight the "const"-ness.

Perhaps "note: variable "x" is declared here with type 'const int'; const qualifier prevents assignment" would be better?

That sounds pretty good to me. Would have to see it in a few more examples (like the const this pointer case, etc) to see how the phrasing generalizes.

Open to other people's ideas too, hopefully there's some other opinions - mine are by no means "golden".

rtrieu updated this revision to Diff 12132.Aug 1 2014, 5:43 PM

Improve the wording of the error message. Make the error more informative by taking the information from the notes and moving it to the error message.

+ "cannot assign to const data member %1 with const qualified type %2|"

Replace the "const data member" with "data member" here; we don't need to
point out the constness twice. (Likewise below)

+ "cannot assign to data member within const member function %1|"

"data member" is not the right term here. Use either "non-static data
member" or "field" (we tend to use the latter in diagnostics that might
appear in C, and the former in exclusively C++ diagnostics).

+ "cannot assign to const data member %1 with const qualified type %2 in a
"
+ "const member function %3}0">;

I don't think this last variant is useful: I think we should either point
out that the field's type is const, or that we're in a const member
function, not both. The way the diagnostic is phrased now might lead to
people thinking that removing *either* const would solve the problem.
Diagnosing the constness of the field seems better to me, since seems in
some sense more fundamental.

+def note_function_returns_const_ref : Note<
+ "function %0 which returns const qualified type %1 declared here">;
+def note_const_variable : Note<
+ "variable %0 declared const here">;
+def note_const_field : Note<
+ "field %0 declared const here">;
+def note_const_method : Note<
+ "member function %q0 is declared const here">;

Could you use a single diagnostic with a %select here?

+enum {
+ ConstUnknown,
+ ConstFunction,
+ ConstVariable,
+ ConstField,
+ ConstMethod,
+ ConstFieldAndMethod
+};

Move this inside the function.

+ PartialDiagnostic PD = S.PDiag(diag::err_typecheck_assign_const)
+ << E->getSourceRange();

Seems like weird indentation for the <<.

+ Handle class fields and methods.
+ if (const MemberExpr *Member = dyn_cast<MemberExpr>(E)) {
+ const Expr *Base = E;
+ while (Member) {
+ ValueDecl *VD = Member->getMemberDecl();
+
Class field is const.
+ if (VD->getType().isConstQualified()) {
+ ME = Member;
+ break;
+ }
+ Base = Member->getBase();
+ Member = dyn_cast<MemberExpr>(Base);
+ }

This loop should go before all the other checks, so that we can diagnose

const X x;
x.y.z = 3;

const X &f();
f().y.z = 3;

properly.

rtrieu updated this revision to Diff 12471.Aug 13 2014, 2:42 PM
- + "cannot assign to const data member %1 with const qualified type %2|"
- 
- Replace the "const data member" with "data member" here; we don't need to
- point out the constness twice. (Likewise below)

Fixed

+ "cannot assign to data member within const member function %1|"

"data member" is not the right term here. Use either "non-static data
member" or "field" (we tend to use the latter in diagnostics that might
appear in C, and the former in exclusively C++ diagnostics).

removed const;

+ "cannot assign to const data member %1 with const qualified type %2 in a
"
+ "const member function %3}0">;

I don't think this last variant is useful: I think we should either point
out that the field's type is const, or that we're in a const member
function, not both. The way the diagnostic is phrased now might lead to
people thinking that removing *either* const would solve the problem.
Diagnosing the constness of the field seems better to me, since seems in
some sense more fundamental.

improved code

+def note_function_returns_const_ref : Note<
+ "function %0 which returns const qualified type %1 declared here">;
+def note_const_variable : Note<
+ "variable %0 declared const here">;
+def note_const_field : Note<
+ "field %0 declared const here">;
+def note_const_method : Note<
+ "member function %q0 is declared const here">;

Could you use a single diagnostic with a %select here?

Done

+enum {
+ ConstUnknown,
+ ConstFunction,
+ ConstVariable,
+ ConstField,
+ ConstMethod,
+ ConstFieldAndMethod
+};

Move this inside the function.

Done

+ PartialDiagnostic PD = S.PDiag(diag::err_typecheck_assign_const)
+ << E->getSourceRange();

Seems like weird indentation for the <<.

Added two more spances.

+ Handle class fields and methods.
+ if (const MemberExpr *Member = dyn_cast<MemberExpr>(E)) {
+ const Expr *Base = E;
+ while (Member) {
+ ValueDecl *VD = Member->getMemberDecl();
+ Class field is const.
+ if (VD->getType().isConstQualified()) {
+ ME = Member;
+ break;
+ }
+ Base = Member->getBase();
+ Member = dyn_cast<MemberExpr>(Base);
+ }

This loop should go before all the other checks, so that we can diagnose

const X x;
x.y.z = 3;

const X &f();
f().y.z = 3;
properly.

Added test cases and makes sure test runs proberl

rsmith added inline comments.Aug 14 2014, 4:04 PM
include/clang/Basic/DiagnosticSemaKinds.td
4817 ↗(On Diff #12471)

Please use 'because' rather than 'since'.

4818–4819 ↗(On Diff #12471)

There's a missing hyphen between "const" and "qualified" in both of these.

4819–4820 ↗(On Diff #12471)

You have "data member" here but "field" below. This should be consistently "field" (or "non-static data member") in both places.

4823 ↗(On Diff #12471)

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

lib/Sema/SemaExpr.cpp
8485–8488 ↗(On Diff #12471)

/// for this comment, please.

8526–8533 ↗(On Diff #12471)

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

8543–8545 ↗(On Diff #12471)

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?

8548–8553 ↗(On Diff #12471)

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

8562–8588 ↗(On Diff #12471)

As noted above, these may be incorrect if we hit a mutable on the way.

rtrieu updated this revision to Diff 12643.Aug 18 2014, 8:39 PM

Add additional test
Rewrite the loop to display notes and stop when no new notes are needed
Fix some wording with the diagnostic

rtrieu added inline comments.Aug 18 2014, 9:01 PM
include/clang/Basic/DiagnosticSemaKinds.td
4817 ↗(On Diff #12471)

Done.

4819–4820 ↗(On Diff #12471)

Went with "static data member" and "non-static data member".

4823 ↗(On Diff #12471)

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.

lib/Sema/SemaExpr.cpp
8485–8488 ↗(On Diff #12471)

Done.

8526–8533 ↗(On Diff #12471)

Do you have an example of the lambda expressions you think this would not produce the correct diagnostic?

8548–8553 ↗(On Diff #12471)

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.

8562–8588 ↗(On Diff #12471)

Done. No longer error on mutables.

rtrieu updated this revision to Diff 12644.Aug 18 2014, 9:15 PM

"const qualified" -> "const-qualified"

rsmith added inline comments.Sep 24 2014, 6:09 PM
lib/Sema/SemaExpr.cpp
8488 ↗(On Diff #12644)

Typo 'funciton'.

8508 ↗(On Diff #12644)

Each iteration of this loop should IgnoreParenImpCasts or similar.

8514 ↗(On Diff #12644)

Can you assert(DiagnosticEmitted) here?

8528 ↗(On Diff #12644)

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.

8549–8570 ↗(On Diff #12644)

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.

8575 ↗(On Diff #12644)

Use FD->getCallResultType().isConstQualified() here. (I don't think there are any functionality changes from that, but if there are, they would be bugfixes.)

8587 ↗(On Diff #12644)

The constness of the callee doesn't seem relevant here; should probably just break at this point.

8594 ↗(On Diff #12644)

It'd be nice to also handle reference-to-const here.

8611–8615 ↗(On Diff #12644)

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.

rtrieu updated this revision to Diff 20440.Feb 20 2015, 2:59 PM

Extend the better error messages to more cases.
Centralize part of the const checking.
Update even more old test cases.

rtrieu added inline comments.Feb 20 2015, 3:43 PM
lib/Sema/SemaExpr.cpp
8488 ↗(On Diff #12644)

Fixed.

8508 ↗(On Diff #12644)

Call to IgnoreParenImpsCasts added.

8514 ↗(On Diff #12644)

Oops, will get this on next patch.

8528 ↗(On Diff #12644)

Dereference is now tracked by IsDereference and NextIsDereference. Those examples have been added to the tests.

8549–8570 ↗(On Diff #12644)

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.

8575 ↗(On Diff #12644)

Call to getNonReferenceType() is needed otherwise

const int& foo();
foo() = 5;

would fall through to the default error message below.

8587 ↗(On Diff #12644)

break added.

8594 ↗(On Diff #12644)

References are handled in IsTypeReadable

8611–8615 ↗(On Diff #12644)

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; } };
                      ~~~~ ^
rsmith added inline comments.Mar 13 2015, 1:37 PM
lib/Sema/SemaExpr.cpp
8780 ↗(On Diff #20440)

Should this be called something more like isTypeModifiable?

8860–8882 ↗(On Diff #20440)

Can you combine this with the general CallExpr check below? The only difference seems to be that you keep going after hitting a CXXMemberCallExpr, in case it's a MemberExpr with a const base object.

8877–8878 ↗(On Diff #20440)

It'd be nice to condition this on the existence of another overload of the callee that is not const and has a non-const-qualified return type.

8887 ↗(On Diff #20440)

Should you be using IsTypeReadable here?

test/Sema/assign.c
9 ↗(On Diff #20440)

Please remove the path here :-)

rtrieu updated this revision to Diff 23624.Apr 10 2015, 2:37 PM
rtrieu added inline comments.Apr 10 2015, 2:58 PM
lib/Sema/SemaExpr.cpp
8780 ↗(On Diff #20440)

Done.

8860–8882 ↗(On Diff #20440)

Removed handling for CXXMemberCallExpr. CallExpr below will handle the generic case. Will try to incorporate the overloaded member detection in a future change.

8887 ↗(On Diff #20440)

Done.

test/Sema/assign.c
9 ↗(On Diff #20440)

Path removed.

rsmith accepted this revision.Apr 10 2015, 3:37 PM
rsmith added a reviewer: rsmith.

LGTM, but I think the loop can be simplified slightly:

lib/Sema/SemaExpr.cpp
8859 ↗(On Diff #23624)

Every code path below this point reaches a break; -- maybe end the loop here?

8874 ↗(On Diff #23624)

The function will return anyway, because DiagnosticEmitted is now true. Remove this return for consistency with the other cases?

8876 ↗(On Diff #23624)

This looks like a dead store. Remove?

This revision is now accepted and ready to land.Apr 10 2015, 3:37 PM
This revision was automatically updated to reflect the committed changes.