[Sema] Disallow assigning record lvalues with nested const-qualified fields.

[bevinh]

Summary

According to C99 6.3.2.1p1, structs and unions with nested
const-qualified fields (that is, const-qualified fields
declared at some recursive level of the aggregate) are not
modifiable lvalues. However, Clang permits assignments of
these lvalues.

With this patch, we both prohibit the assignment of records
with const-qualified fields and emit a best-effort diagnostic.
This fixes https://bugs.llvm.org/show_bug.cgi?id=31796 .

[bjope]

include/clang/Basic/DiagnosticSemaKinds.td

5888 ↗

(On Diff #113038)

Shouldn't there be one more entry here as well?
(see comment about updating both err_typecheck_assign_const and note_typecheck_assign_const when adding things to the enum at line 10181).

If I understand it correctly you never print notes for the new NestedConstMember, so there isn't really a need for a fifth entry in the select today.
But if someone adds new enum values later (putting them after NestedContMember) then it might be nice if the size of this select matches with the old size of the enum.

lib/Sema/SemaExpr.cpp

10329 ↗

(On Diff #113038)

If adding a new entry to note_typecheck_assign_const (identical to the ConstMember entry) then you can use NestedConstMember here. It would make it possible to change the string for NestedConstMember notes without also changing all ConstMember notes.

Now it kind of looks like an error that the error diagnostic is for a NestedConstMember, but the note is about a ConstMember. That might confuse someone, since it looks like a typo unless you know what is going on here.

[bevinh]

Added a diag note for NestedConstMember.

[bjope]

This patch looks good to me now.
But I'm currently working in the same out-of-tree project as the author, so it would be nice if someone else could have a look as well and accept this.

[bjope]

Nobody else seems to bother. At least no objections, so I think we can land this now.

cfe/trunk/include/clang/AST/Expr.h

enum isModifiableLvalueResult {
     MLV_Valid,
     MLV_NotObjectType,
     MLV_IncompleteVoidType,
     MLV_DuplicateVectorComponents,
     MLV_InvalidExpression,
     MLV_LValueCast,           // Specialized form of MLV_InvalidExpression.
     MLV_IncompleteType,
     MLV_ConstQualified,
+    MLV_ConstQualifiedField,
     MLV_ConstAddrSpace,
     MLV_ArrayType,
     MLV_NoSetterProperty,
     MLV_MemberFunction,
     MLV_SubObjCPropertySetting,
     MLV_InvalidMessageExpression,
     MLV_ClassTemporary,
     MLV_ArrayTemporary
public:
     enum ModifiableType {
       CM_Untested, // testModifiable was false.
       CM_Modifiable,
       CM_RValue, // Not modifiable because it's an rvalue
       CM_Function, // Not modifiable because it's a function; C++ only
       CM_LValueCast, // Same as CM_RValue, but indicates GCC cast-as-lvalue ext
       CM_NoSetterProperty,// Implicit assignment to ObjC property without setter
       CM_ConstQualified,
+      CM_ConstQualifiedField,
       CM_ConstAddrSpace,
       CM_ArrayType,
       CM_IncompleteType
     };
 
   private:
     friend class Expr;
 

cfe/trunk/include/clang/AST/Type.h

explicit RecordType(TypeClass TC, RecordDecl *D)
     : TagType(TC, reinterpret_cast<const TagDecl*>(D), QualType()) { }
   friend class ASTContext;   // ASTContext creates these.
 public:
 
   RecordDecl *getDecl() const {
     return reinterpret_cast<RecordDecl*>(TagType::getDecl());
   }
 
-  // FIXME: This predicate is a helper to QualType/Type. It needs to
-  // recursively check all fields for const-ness. If any field is declared
-  // const, it needs to return false.
-  bool hasConstFields() const { return false; }
+  /// Recursively check all fields in the record for const-ness. If any field
+  /// is declared const, return true. Otherwise, return false.
+  bool hasConstFields() const;
 
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 
   static bool classof(const Type *T) { return T->getTypeClass() == Record; }
 };
 
 /// A helper class that allows the use of isa/cast/dyncast

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 
 def err_typecheck_assign_const : Error<
   "%select{"
   "cannot assign to return value because function %1 returns a const value|"
   "cannot assign to variable %1 with const-qualified type %2|"
   "cannot assign to %select{non-|}1static data member %2 "
   "with const-qualified type %3|"
   "cannot assign to non-static data member within const member function %1|"
+  "cannot assign to %select{variable %2|non-static data member %2|lvalue}1 "
+  "with %select{|nested }3const-qualified data member %4|"
   "read-only variable is not assignable}0">;
 
 def note_typecheck_assign_const : Note<
   "%select{"
   "function %1 which returns const-qualified type %2 declared here|"
   "variable %1 declared const here|"
   "%select{non-|}1static data member %2 declared const here|"
-  "member function %q1 is declared const here}0">;
+  "member function %q1 is declared const here|"
+  "%select{|nested }1data member %2 declared const here}0">;
 
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,
   InGroup<SignCompare>, DefaultIgnore;
 def warn_lunsigned_always_true_comparison : Warning<
   "comparison of unsigned%select{| enum}2 expression %0 is always %1">,
   InGroup<TautologicalUnsignedZeroCompare>;
 def warn_out_of_range_compare : Warning<

cfe/trunk/lib/AST/ExprClassification.cpp

if (CT->isArrayType())
     return Cl::CM_ArrayType;
   // Incomplete types are not modifiable.
   if (CT->isIncompleteType())
     return Cl::CM_IncompleteType;
 
   // Records with any const fields (recursively) are not modifiable.
   if (const RecordType *R = CT->getAs<RecordType>())
     if (R->hasConstFields())
-      return Cl::CM_ConstQualified;
+      return Cl::CM_ConstQualifiedField;
 
   return Cl::CM_Modifiable;
 }
 
 Expr::LValueClassification Expr::ClassifyLValue(ASTContext &Ctx) const {
   Classification VC = Classify(Ctx);
   switch (VC.getKind()) {
   case Cl::CL_LValue: return LV_Valid;
Expr::isModifiableLvalue(ASTContext &Ctx, SourceLocation *Loc) const {
   case Cl::CM_Untested: llvm_unreachable("Did not test modifiability");
   case Cl::CM_Modifiable: return MLV_Valid;
   case Cl::CM_RValue: llvm_unreachable("CM_RValue and CL_LValue don't match");
   case Cl::CM_Function: return MLV_NotObjectType;
   case Cl::CM_LValueCast:
     llvm_unreachable("CM_LValueCast and CL_LValue don't match");
   case Cl::CM_NoSetterProperty: return MLV_NoSetterProperty;
   case Cl::CM_ConstQualified: return MLV_ConstQualified;
+  case Cl::CM_ConstQualifiedField: return MLV_ConstQualifiedField;
   case Cl::CM_ConstAddrSpace: return MLV_ConstAddrSpace;
   case Cl::CM_ArrayType: return MLV_ArrayType;
   case Cl::CM_IncompleteType: return MLV_IncompleteType;
   }
   llvm_unreachable("Unhandled modifiable type");
 }

cfe/trunk/lib/AST/Type.cpp

 TagDecl *TagType::getDecl() const {
   return getInterestingTagDecl(decl);
 }
 
 bool TagType::isBeingDefined() const {
   return getDecl()->isBeingDefined();
 }
 
+bool RecordType::hasConstFields() const {
+  for (FieldDecl *FD : getDecl()->fields()) {
+    QualType FieldTy = FD->getType();
+    if (FieldTy.isConstQualified())
+      return true;
+    FieldTy = FieldTy.getCanonicalType();
+    if (const RecordType *FieldRecTy = FieldTy->getAs<RecordType>())
+      if (FieldRecTy->hasConstFields())
+        return true;
+  }
+  return false;
+}
+
 bool AttributedType::isQualifier() const {
   switch (getAttrKind()) {
   // These are type qualifiers in the traditional C sense: they annotate
   // something about a specific value/variable of a type.  (They aren't
   // always part of the canonical type, though.)
   case AttributedType::attr_address_space:
   case AttributedType::attr_objc_gc:
   case AttributedType::attr_objc_ownership:

cfe/trunk/lib/Sema/SemaExpr.cpp

 
 static bool IsTypeModifiable(QualType Ty, bool IsDereference) {
   Ty = Ty.getNonReferenceType();
   if (IsDereference && Ty->isPointerType())
     Ty = Ty->getPointeeType();
   return !Ty.isConstQualified();
 }
 
-/// 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 function is returning a const reference.
-static void DiagnoseConstAssignment(Sema &S, const Expr *E,
-                                    SourceLocation Loc) {
-  // Update err_typecheck_assign_const and note_typecheck_assign_const
-  // when this enum is changed.
-  enum {
-    ConstFunction,
-    ConstVariable,
-    ConstMember,
-    ConstMethod,
-    ConstUnknown,  // Keep as last element
-  };
+  NestedConstMember,
 
+/// 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 function is returning a const reference.
+static void DiagnoseConstAssignment(Sema &S, const Expr *E,
+                                    SourceLocation Loc) {
   SourceRange ExprRange = E->getSourceRange();
 
   // Only emit one error on the first const found.  All other consts will emit
   // a note to the error.
   bool DiagnosticEmitted = false;
 
   // Track if the current expression is the result of a dereference, and if the
   // next checked expression is the result of a dereference.
static void DiagnoseConstAssignment(Sema &S, const Expr *E,
 
   if (DiagnosticEmitted)
     return;
 
   // Can't determine a more specific message, so display the generic error.
   S.Diag(Loc, diag::err_typecheck_assign_const) << ExprRange << ConstUnknown;
 }
 
+enum OriginalExprKind {
+  OEK_Variable,
+  OEK_Member,
+  OEK_LValue
+};
+
+static void DiagnoseRecursiveConstFields(Sema &S, const ValueDecl *VD,
+                                         const RecordType *Ty,
+                                         SourceLocation Loc, SourceRange Range,
+                                         OriginalExprKind OEK,
+                                         bool &DiagnosticEmitted,
+                                         bool IsNested = false) {
+  // We walk the record hierarchy breadth-first to ensure that we print
+  // diagnostics in field nesting order.
+  // First, check every field for constness.
+  for (const FieldDecl *Field : Ty->getDecl()->fields()) {
+    if (Field->getType().isConstQualified()) {
+      if (!DiagnosticEmitted) {
+        S.Diag(Loc, diag::err_typecheck_assign_const)
+            << Range << NestedConstMember << OEK << VD
+            << IsNested << Field;
+        DiagnosticEmitted = true;
+      }
+      S.Diag(Field->getLocation(), diag::note_typecheck_assign_const)
+          << NestedConstMember << IsNested << Field
+          << Field->getType() << Field->getSourceRange();
+    }
+  }
+  // Then, recurse.
+  for (const FieldDecl *Field : Ty->getDecl()->fields()) {
+    QualType FTy = Field->getType();
+    if (const RecordType *FieldRecTy = FTy->getAs<RecordType>())
+      DiagnoseRecursiveConstFields(S, VD, FieldRecTy, Loc, Range,
+                                   OEK, DiagnosticEmitted, true);
+  }
+}
+
+/// Emit an error for the case where a record we are trying to assign to has a
+/// const-qualified field somewhere in its hierarchy.
+static void DiagnoseRecursiveConstFields(Sema &S, const Expr *E,
+                                         SourceLocation Loc) {
+  QualType Ty = E->getType();
+  assert(Ty->isRecordType() && "lvalue was not record?");
+  SourceRange Range = E->getSourceRange();
+  const RecordType *RTy = Ty.getCanonicalType()->getAs<RecordType>();
+  bool DiagEmitted = false;
+
+  if (const MemberExpr *ME = dyn_cast<MemberExpr>(E))
+    DiagnoseRecursiveConstFields(S, ME->getMemberDecl(), RTy, Loc,
+            Range, OEK_Member, DiagEmitted);
+  else if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
+    DiagnoseRecursiveConstFields(S, DRE->getDecl(), RTy, Loc,
+            Range, OEK_Variable, DiagEmitted);
+  else
+    DiagnoseRecursiveConstFields(S, nullptr, RTy, Loc,
+            Range, OEK_LValue, DiagEmitted);
+  if (!DiagEmitted)
+    DiagnoseConstAssignment(S, E, Loc);
+}
+
 /// CheckForModifiableLvalue - Verify that E is a modifiable lvalue.  If not,
 /// emit an error and return true.  If so, return false.
 static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) {
   assert(!E->hasPlaceholderType(BuiltinType::PseudoObject));
 
   S.CheckShadowingDeclModification(E, Loc);
 
   SourceLocation OrigLoc = Loc;
if (DiagID == 0) {
       DiagnoseConstAssignment(S, E, Loc);
       return true;
     }
 
     break;
   case Expr::MLV_ConstAddrSpace:
     DiagnoseConstAssignment(S, E, Loc);
     return true;
+  case Expr::MLV_ConstQualifiedField:
+    DiagnoseRecursiveConstFields(S, E, Loc);
+    return true;
   case Expr::MLV_ArrayType:
   case Expr::MLV_ArrayTemporary:
     DiagID = diag::err_typecheck_array_not_modifiable_lvalue;
     NeedType = true;
     break;
   case Expr::MLV_NotObjectType:
     DiagID = diag::err_typecheck_non_object_not_modifiable_lvalue;
     NeedType = true;

cfe/trunk/test/Sema/assign.c

 
 typedef int arr[10];
 void test3() {
   const arr b;
   const int b2[10]; 
   b[4] = 1; // expected-error {{read-only variable is not assignable}}
   b2[4] = 1; // expected-error {{read-only variable is not assignable}}
 }
+
+typedef struct I {
+  const int a; // expected-note 4{{nested data member 'a' declared const here}} \
+                  expected-note 6{{data member 'a' declared const here}}
+} I;
+typedef struct J {
+  struct I i;
+} J;
+typedef struct K {
+  struct J *j;
+} K;
+
+void testI(struct I i1, struct I i2) {
+  i1 = i2; // expected-error {{cannot assign to variable 'i1' with const-qualified data member 'a'}}
+}
+void testJ1(struct J j1, struct J j2) {
+  j1 = j2; // expected-error {{cannot assign to variable 'j1' with nested const-qualified data member 'a'}}
+}
+void testJ2(struct J j, struct I i) {
+  j.i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}}
+}
+void testK1(struct K k, struct J j) {
+  *(k.j) = j; // expected-error {{cannot assign to lvalue with nested const-qualified data member 'a'}}
+}
+void testK2(struct K k, struct I i) {
+  k.j->i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}}
+}
+
+void testI_(I i1, I i2) {
+  i1 = i2; // expected-error {{cannot assign to variable 'i1' with const-qualified data member 'a'}}
+}
+void testJ1_(J j1, J j2) {
+  j1 = j2; // expected-error {{cannot assign to variable 'j1' with nested const-qualified data member 'a'}}
+}
+void testJ2_(J j, I i) {
+  j.i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}}
+}
+void testK1_(K k, J j) {
+  *(k.j) = j; // expected-error {{cannot assign to lvalue with nested const-qualified data member 'a'}}
+}
+void testK2_(K k, I i) {
+  k.j->i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}}
+}