[WIP] Fix member access of anonymous struct/union fields in C

[aaron.ballman]

Summary

We were accepting invalid code where the qualifiers of the anonymous structure were not taken into account when forming a member access to one of the indirect fields, as in:

struct S {
  const struct {
    int i;
  };
} s;

void foo(void) {
  s.i = 12; // previously accepted, now rejected
}

We now expose the anonymous structure's field as having a qualified type instead of an unqualified type so that access checking notes that an intermediate object in the expression is qualified. We also adjusted the diagnostic so that it's slightly more user friendly despite this being a rather rare case.

Note, this only impacts C; in C++ and with the Microsoft C anonymous structure extension, the qualifiers are stripped.

Fixes #48099

[aaron.ballman]

I'm marking this as a WIP -- the content is ready to go, but I'm no longer convinced we want to implement this for GCC compatibility as GCC is the only C compiler I can find that cares about the qualifiers: https://godbolt.org/z/hTqY8zMb5

[tahonermann]

clang/docs/ReleaseNotes.rst

152–157

Perhaps the first sentence should clarify that it applies only to C modes? Though the code changes don't appear to be specific to C.

I'm confused by "Note that qualifiers are ignored in C++ and for Microsoft's extension of anonymous objects". I find it very surprising that cv-qualifiers would be ignored in C++. Unless there is a relevant core issue?

clang/test/Sema/anonymous-struct-union.c

137–140

Top-level cv-qualifiers are ignored in generic selection expressions, so I'm not sure what these assertions are intended to ensure.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

152–157

I can definitely clarify if we decide to go this route.

The C++ behavior is intentional (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L5352) but perhaps emergent as the C++ standard is silent on qualifiers on the anonymous union object, but all C++ implementations agree to ignore the qualifiers: https://godbolt.org/z/7Wd8Kn5Mf.

clang/test/Sema/anonymous-struct-union.c

137–140

I think this is the second time I've been caught by that. I keep expecting that the "no two associations can use compatible types" to make that situation an error.

I almost wonder if we should give a warning in cases where an association type is qualified (or is an array/function type rather than a decayed pointer)?

[tahonermann]

clang/test/Sema/anonymous-struct-union.c

137–140

I was surprised when I first learned of this behavior too; it hobbles the utility of generic selection expressions in some cases.

A warning like you suggest does seem appropriate. I'm not aware of any way in which a cv-qualified or non-decayed association type can be used in any useful manner (other than to show that a compiler is not behaving properly). If there is such a use, I would be interested in knowing what it is!

[nickdesaulniers]

Consider adding an AST test; that was something that came up in my patch https://reviews.llvm.org/D95408.

I'm pushing back on the WG14 lists to see if this is a good opportunity to clarify the other direction and be compatible with C++ in the process.

It would really be good to have some clarification from WG14 added to the spec; as is, code is being written in the Linux kernel that uses this functionality to avoid marking all members of an anonymous aggregate as const. This is resulting in code being submitted where clang doesn't error on modification of such members, while GCC errors, so such patches are rejected. So developers using clang are getting caught off guard for a code base that needs to be compiler portable.

Kernel developers were of the opinion that it is nice syntactic sugar. For that reason, I agree with them. But I'm also fine leaving this out of clang until the spec is amended to clarify. That said, if WG14 doesn't pursue this (but doesn't reject or accept), I'd like to have this in clang sooner for better compatibility with GCC. It is too bad though if WG14 chooses to further diverge C from C++ over this...

clang/docs/ReleaseNotes.rst

 - Fixed a false positive diagnostic about an unevaluated expression having no
   side effects when the expression is of VLA type and is an operand of the
   ``sizeof`` operator. Fixes `Issue 48010 <https://github.com/llvm/llvm-project/issues/48010>`_.
 - Fixed a false positive diagnostic about scoped enumerations being a C++11
   extension in C mode. A scoped enumeration's enumerators cannot be named in C
   because there is no way to fully qualify the enumerator name, so this
   "extension" was unintentional and useless. This fixes
   `Issue 42372 <https://github.com/llvm/llvm-project/issues/42372>`_.
+- When forming a member expression, now consider any qualifiers written on an
+  anonymous structure or union as also applying to the field being referenced.
+  This fixes an issue where qualifiers were being ignored, allowing you to
+  assign to a ``const`` field. Note that qualifiers are ignored in C++ and for
+  Microsoft's extension of anonymous objects. This fixes
+  `Issues 48099 <https://github.com/llvm/llvm-project/issues/48099>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "%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|"
+  "|" // Unused for anonymous objects
   "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|"
-  "%select{|nested }1data member %2 declared const here}0">;
+  "%select{|nested }1data member %2 declared const here|"
+  "anonymous %select{union|struct}1 declared const here}0">;
 
 def warn_unsigned_always_true_comparison : Warning<
   "result of comparison of %select{%3|unsigned expression}0 %2 "
   "%select{unsigned expression|%3}0 is always %4">,
   InGroup<TautologicalUnsignedZeroCompare>, DefaultIgnore;
 def warn_unsigned_char_always_true_comparison : Warning<
   "result of comparison of %select{%3|char expression}0 %2 "
   "%select{char expression|%3}0 is always %4, since char is interpreted as "

clang/lib/Sema/SemaDecl.cpp

   TypeSourceInfo *TInfo = GetTypeForDeclarator(Dc, S);
   assert(TInfo && "couldn't build declarator info for anonymous struct/union");
 
   // Create a declaration for this anonymous struct/union.
   NamedDecl *Anon = nullptr;
   if (RecordDecl *OwningClass = dyn_cast<RecordDecl>(Owner)) {
     Anon = FieldDecl::Create(
         Context, OwningClass, DS.getBeginLoc(), Record->getLocation(),
-        /*IdentifierInfo=*/nullptr, Context.getTypeDeclType(Record), TInfo,
+                             /*IdentifierInfo=*/nullptr,
+                             Context.getTypeDeclType(Record).withFastQualifiers(
+                                 DS.getTypeQualifiers()),
+                             TInfo,
         /*BitWidth=*/nullptr, /*Mutable=*/false,
         /*InitStyle=*/ICIS_NoInit);
     Anon->setAccess(AS);
     ProcessDeclAttributes(S, Anon, Dc);
 
     if (getLangOpts().CPlusPlus)
       FieldCollector->Add(cast<FieldDecl>(Anon));
   } else {
     DeclSpec::SCS SCSpec = DS.getStorageClassSpec();
     StorageClass SC = StorageClassSpecToVarDeclStorageClass(DS);
     if (SCSpec == DeclSpec::SCS_mutable) {
       // mutable can only appear on non-static class members, so it's always
       // an error here
       Diag(Record->getLocation(), diag::err_mutable_nonmember);
       Invalid = true;
       SC = SC_None;
     }
 
     assert(DS.getAttributes().empty() && "No attribute expected");
     Anon = VarDecl::Create(Context, Owner, DS.getBeginLoc(),
                            Record->getLocation(), /*IdentifierInfo=*/nullptr,
-                           Context.getTypeDeclType(Record), TInfo, SC);
+                           Context.getTypeDeclType(Record).withFastQualifiers(
+                               DS.getTypeQualifiers()),
+                           TInfo, SC);
 
     // Default-initialize the implicit variable. This initialization will be
     // trivial in almost all cases, except if a union member has an in-class
     // initializer:
     //   union { int n = 0; };
     ActOnUninitializedDecl(Anon);
   }
   Anon->setImplicit();

clang/lib/Sema/SemaExpr.cpp

 // Update err_typecheck_assign_const and note_typecheck_assign_const
 // when this enum is changed.
 enum {
   ConstFunction,
   ConstVariable,
   ConstMember,
   ConstMethod,
   NestedConstMember,
+  ConstAnonymousObject, // Only used as a note
   ConstUnknown,  // Keep as last element
 };
 
 /// 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.
   bool IsDereference = false;
   bool NextIsDereference = false;
+  // Used when diagnosing indirect fields that are const because the anonymous
+  // object is const rather than the field. This allows us to diagnose the
+  // field rather than the anonymous object.
+  const FieldDecl *LastNamedField = nullptr;
 
   // Loop to process MemberExpr chains.
   while (true) {
     IsDereference = NextIsDereference;
 
     E = E->IgnoreImplicit()->IgnoreParenImpCasts();
     if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
       NextIsDereference = ME->isArrow();
       const ValueDecl *VD = ME->getMemberDecl();
       if (const FieldDecl *Field = dyn_cast<FieldDecl>(VD)) {
         // Mutable fields can be modified even if the class is const.
         if (Field->isMutable()) {
           assert(DiagnosticEmitted && "Expected diagnostic not emitted.");
           break;
         }
 
         if (!IsTypeModifiable(Field->getType(), IsDereference)) {
           if (!DiagnosticEmitted) {
-            S.Diag(Loc, diag::err_typecheck_assign_const)
-                << ExprRange << ConstMember << false /*static*/ << Field
-                << Field->getType();
+            // If the field is an anonymous union or structure, we want to
+            // diagnose the last named field rather than the unnamed object.
+            // This requires us to combine the qualifiers from the anonymous
+            // object with those of the field in order for the diagnostic to
+            // make sense.
+            if (Field->isAnonymousStructOrUnion()) {
+              assert(LastNamedField &&
+                     "we should have seen a named field by this point");
+              S.Diag(Loc, diag::err_typecheck_assign_const)
+                  << ExprRange << ConstMember
+                  << false /*static*/ << LastNamedField
+                  << LastNamedField->getType().withFastQualifiers(
+                         Field->getType().getLocalFastQualifiers());
+            } else {
+              S.Diag(Loc, diag::err_typecheck_assign_const)
+                  << ExprRange << ConstMember << false /*static*/ << Field
+                  << Field->getType();
+            }
             DiagnosticEmitted = true;
           }
-          S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
-              << ConstMember << false /*static*/ << Field << Field->getType()
-              << Field->getSourceRange();
+          // If the field is an anonymous union or structure, we want the note
+          // to be about the anonymous object being const rather than the
+          // indirect field.
+          if (Field->isAnonymousStructOrUnion()) {
+            S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
+                << ConstAnonymousObject
+                << Field->getType()
+                       ->castAs<RecordType>()
+                       ->getDecl()
+                       ->isStruct();
+          } else {
+            S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
+                << ConstMember << false /*static*/ << Field << Field->getType()
+                << Field->getSourceRange();
+          }
         }
         E = ME->getBase();
+        LastNamedField = Field;
         continue;
       } else if (const VarDecl *VDecl = dyn_cast<VarDecl>(VD)) {
         if (VDecl->getType().isConstQualified()) {
           if (!DiagnosticEmitted) {
             S.Diag(Loc, diag::err_typecheck_assign_const)
                 << ExprRange << ConstMember << true /*static*/ << VDecl
                 << VDecl->getType();
             DiagnosticEmitted = true;

clang/test/Sema/anonymous-struct-union.c

 struct s3 {
   struct { int A __attribute__((deprecated)); }; // expected-note {{'A' has been explicitly marked deprecated here}}
 };
 
 void deprecated_anonymous_struct_member(void) {
   struct s3 s;
   s.A = 1; // expected-warning {{'A' is deprecated}}
 }
+
+struct GH48099 {
+  const struct { // expected-note {{anonymous struct declared const here}}
+    int i;
+  };
+  volatile union {
+    int j;
+  };
+};
+
+// Ensure that qualifiers from the anonymous object are reflected on the
+// fields being hoisted into the outer context.
+void GH48099_test(void) {
+  struct GH48099 x;
+
+  // It's the access path that picks up the qualifiers, not the direct
+  // declaration of the field itself. So 'i' and 'j' are both 'int'.
+  _Static_assert(_Generic(x.i, int : 1, default : 0), "i is not int?");
+  _Static_assert(_Generic(x.j, int : 1, default : 0), "j is not int?");
+
+  // Test that the member access picks up the qualifiers.
+  x.i = 12; // expected-error {{cannot assign to non-static data member 'i' with const-qualified type 'const int'}}
+}

clang/test/Sema/anonymous-struct-union.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions -Wno-microsoft-anon-tag -x c %s
+// expected-no-diagnostics
+
+struct trivial {
+  int a;
+};
+
+struct GH48099 {
+  const struct {
+    int i;
+  };
+#ifndef __cplusplus
+  // This extension is only allowed in C.
+  const struct trivial;
+#endif
+};
+
+// In C++, qualifiers are ignored on the anonymous objects.
+void GH48099_test(void) {
+  struct GH48099 x;
+
+#ifdef __cplusplus  
+  // Test that the member access ignores the qualifiers. This would fail in C.
+  x.i = 12; // ok
+#else
+  // Test that the member access ignores the qualifiers for a Microsoft C
+  // anonymous structure declaration. This would fail in C++ because the
+  // declaration in GH48099 does not declare anything (so 'a' isn't lifted into
+  // the outer structure).
+  x.a = 12; // ok
+#endif
+}
+