Add Clang attribute to ensure that fields are initialized explicitly

[higher-performance]

This is a new Clang-specific attribute to ensure that field initializations are performed explicitly.

For example, if we have

struct B {
  [[clang::explicit]] int f1;
};

then the diagnostic would trigger if we do B b{};:

field 'f1' is left uninitialized, but was marked as requiring initialization

This prevents callers from accidentally forgetting to initialize fields, particularly when new fields are added to the class.

Naming:

We are open to alternative names; we would just like their meanings to be clear. For example, must_init, requires_init, etc. are some alternative suggestions that would be fine. However, we would like to avoid a name such as required as must_specify, as their meanings might be potentially unclear or confusing (e.g., due to confusion with requires).

Note:

I'm running into an issue with duplicated diagnostics (see lit tests) that I'm not sure how to properly resolve, but I suspect it revolves around VerifyOnly. If you know the proper fix please let me know.

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: None (higher-performance)

Changes

This is a new Clang-specific attribute to ensure that field initializations are performed explicitly.

For example, if we have

  struct B {
    [[clang::explicit]] int f1;
  };

---
Full diff: https://github.com/llvm/llvm-project/pull/102040.diff


10 Files Affected:

- (modified) clang/include/clang/AST/CXXRecordDeclDefinitionBits.def (+8) 
- (modified) clang/include/clang/AST/DeclCXX.h (+5) 
- (modified) clang/include/clang/Basic/Attr.td (+8) 
- (modified) clang/include/clang/Basic/AttrDocs.td (+22) 
- (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1) 
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3) 
- (modified) clang/lib/AST/DeclCXX.cpp (+9) 
- (modified) clang/lib/Sema/SemaDeclAttr.cpp (+7) 
- (modified) clang/lib/Sema/SemaInit.cpp (+11) 
- (modified) clang/test/SemaCXX/uninitialized.cpp (+22) 


``````````diff
diff --git a/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def b/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
index cdf0804680ad0..a782026462566 100644
--- a/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
+++ b/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
@@ -119,6 +119,14 @@ FIELD(HasInitMethod, 1, NO_MERGE)
 /// within anonymous unions or structs.
 FIELD(HasInClassInitializer, 1, NO_MERGE)
 
+/// Custom attribute that is True if any field is marked as explicit in a type
+/// without a user-provided default constructor, or if this is the case for any
+/// base classes and/or member variables whose types are aggregates.
+///
+/// In this case, default-construction is diagnosed, as it would not explicitly
+/// initialize the field.
+FIELD(HasUninitializedExplicitFields, 1, NO_MERGE)
+
 /// True if any field is of reference type, and does not have an
 /// in-class initializer.
 ///
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index bf6a5ce92d438..8228595d84b0f 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1151,6 +1151,11 @@ class CXXRecordDecl : public RecordDecl {
   /// structs).
   bool hasInClassInitializer() const { return data().HasInClassInitializer; }
 
+  bool hasUninitializedExplicitFields() const {
+    return !isUnion() && !hasUserProvidedDefaultConstructor() &&
+           data().HasUninitializedExplicitFields;
+  }
+
   /// Whether this class or any of its subobjects has any members of
   /// reference type which would make value-initialization ill-formed.
   ///
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 8ac2079099c85..409a7cd9177e8 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1823,6 +1823,14 @@ def Leaf : InheritableAttr {
   let SimpleHandler = 1;
 }
 
+def Explicit : InheritableAttr {
+  let Spellings = [Clang<"explicit", 0>];
+  let Subjects = SubjectList<[Field], ErrorDiag>;
+  let Documentation = [ExplicitDocs];
+  let LangOpts = [CPlusPlus];
+  let SimpleHandler = 1;
+}
+
 def LifetimeBound : DeclOrTypeAttr {
   let Spellings = [Clang<"lifetimebound", 0>];
   let Subjects = SubjectList<[ParmVar, ImplicitObjectParameter], ErrorDiag>;
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 94c284fc73158..66d0044aa979d 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -1419,6 +1419,28 @@ is not specified.
   }];
 }
 
+def ExplicitDocs : Documentation {
+  let Category = DocCatField;
+  let Content = [{
+The ``clang::explicit`` attribute indicates that the field must be initialized
+explicitly by the caller when the class is constructed.
+
+Example usage:
+
+.. code-block:: c++
+
+  struct some_aggregate {
+    int x;
+    int y [[clang::explicit]];
+  };
+
+  some_aggregate create() {
+    return {.x = 1};  // error: y is not initialized explicitly
+  }
+
+  }];
+}
+
 def NoUniqueAddressDocs : Documentation {
   let Category = DocCatField;
   let Content = [{
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 19c3f1e043349..52af3d3a7af39 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -787,6 +787,7 @@ def Trigraphs      : DiagGroup<"trigraphs">;
 def UndefinedReinterpretCast : DiagGroup<"undefined-reinterpret-cast">;
 def ReinterpretBaseClass : DiagGroup<"reinterpret-base-class">;
 def Unicode  : DiagGroup<"unicode">;
+def UninitializedExplicit : DiagGroup<"uninitialized-explicit">;
 def UninitializedMaybe : DiagGroup<"conditional-uninitialized">;
 def UninitializedSometimes : DiagGroup<"sometimes-uninitialized">;
 def UninitializedStaticSelfInit : DiagGroup<"static-self-init">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 581434d33c5c9..fb493e3c4f7f0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2325,6 +2325,9 @@ def err_init_reference_member_uninitialized : Error<
   "reference member of type %0 uninitialized">;
 def note_uninit_reference_member : Note<
   "uninitialized reference member is here">;
+def warn_field_requires_init : Warning<
+  "field %0 is left uninitialized, but was marked as requiring initialization">,
+  InGroup<UninitializedExplicit>;
 def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">,
   InGroup<Uninitialized>;
 def warn_base_class_is_uninit : Warning<
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 9a3ede426e914..732117ae263c2 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -81,6 +81,7 @@ CXXRecordDecl::DefinitionData::DefinitionData(CXXRecordDecl *D)
       HasPrivateFields(false), HasProtectedFields(false),
       HasPublicFields(false), HasMutableFields(false), HasVariantMembers(false),
       HasOnlyCMembers(true), HasInitMethod(false), HasInClassInitializer(false),
+      HasUninitializedExplicitFields(false),
       HasUninitializedReferenceMember(false), HasUninitializedFields(false),
       HasInheritedConstructor(false), HasInheritedDefaultConstructor(false),
       HasInheritedAssignment(false),
@@ -1108,6 +1109,10 @@ void CXXRecordDecl::addedMember(Decl *D) {
     } else if (!T.isCXX98PODType(Context))
       data().PlainOldData = false;
 
+    if (Field->hasAttr<ExplicitAttr>() && !Field->hasInClassInitializer()) {
+      data().HasUninitializedExplicitFields = true;
+    }
+
     if (T->isReferenceType()) {
       if (!Field->hasInClassInitializer())
         data().HasUninitializedReferenceMember = true;
@@ -1359,6 +1364,10 @@ void CXXRecordDecl::addedMember(Decl *D) {
         if (!FieldRec->hasCopyAssignmentWithConstParam())
           data().ImplicitCopyAssignmentHasConstParam = false;
 
+        if (FieldRec->hasUninitializedExplicitFields() &&
+            FieldRec->isAggregate() && !Field->hasInClassInitializer())
+          data().HasUninitializedExplicitFields = true;
+
         if (FieldRec->hasUninitializedReferenceMember() &&
             !Field->hasInClassInitializer())
           data().HasUninitializedReferenceMember = true;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 9011fa547638e..b55c845b74528 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5943,6 +5943,10 @@ static void handleNoMergeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   D->addAttr(NoMergeAttr::Create(S.Context, AL));
 }
 
+static void handleExplicitAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  D->addAttr(ExplicitAttr::Create(S.Context, AL));
+}
+
 static void handleNoUniqueAddressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   D->addAttr(NoUniqueAddressAttr::Create(S.Context, AL));
 }
@@ -6848,6 +6852,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
   case ParsedAttr::AT_NoMerge:
     handleNoMergeAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_Explicit:
+    handleExplicitAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_NoUniqueAddress:
     handleNoUniqueAddressAttr(S, D, AL);
     break;
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 90fd6df782f09..47b5c4bd388eb 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -743,6 +743,11 @@ void InitListChecker::FillInEmptyInitForField(unsigned Init, FieldDecl *Field,
         ILE->updateInit(SemaRef.Context, Init, Filler);
       return;
     }
+
+    if (Field->hasAttr<ExplicitAttr>()) {
+      SemaRef.Diag(ILE->getExprLoc(), diag::warn_field_requires_init) << Field;
+    }
+
     // C++1y [dcl.init.aggr]p7:
     //   If there are fewer initializer-clauses in the list than there are
     //   members in the aggregate, then each member not explicitly initialized
@@ -4475,6 +4480,12 @@ static void TryConstructorInitialization(Sema &S,
 
   CXXConstructorDecl *CtorDecl = cast<CXXConstructorDecl>(Best->Function);
   if (Result != OR_Deleted) {
+    if (!IsListInit && Kind.getKind() == InitializationKind::IK_Default &&
+        DestRecordDecl != nullptr && DestRecordDecl->isAggregate() &&
+        DestRecordDecl->hasUninitializedExplicitFields()) {
+      S.Diag(Kind.getLocation(), diag::warn_field_requires_init) << "in class";
+    }
+
     // C++11 [dcl.init]p6:
     //   If a program calls for the default initialization of an object
     //   of a const-qualified type T, T shall be a class type with a
diff --git a/clang/test/SemaCXX/uninitialized.cpp b/clang/test/SemaCXX/uninitialized.cpp
index 8a640c9691b32..3a339b4f9c9c4 100644
--- a/clang/test/SemaCXX/uninitialized.cpp
+++ b/clang/test/SemaCXX/uninitialized.cpp
@@ -1472,3 +1472,25 @@ template<typename T> struct Outer {
   };
 };
 Outer<int>::Inner outerinner;
+
+void aggregate() {
+  struct B {
+    [[clang::explicit]] int f1;
+  };
+
+  struct S : B { // expected-warning {{uninitialized}}
+    int f2;
+    int f3 [[clang::explicit]];
+  };
+
+#if __cplusplus >= 202002L
+  S a({}, 0);  // expected-warning {{'f1' is left uninitialized}} expected-warning {{'f3' is left uninitialized}}
+#endif
+  S b{.f3 = 1}; // expected-warning {{'f1' is left uninitialized}}
+  S c{.f2 = 5}; // expected-warning {{'f1' is left uninitialized}} expected-warning {{'f3' is left uninitialized}} expected-warning {{'f3' is left uninitialized}}
+  c = {{}, 0};  // expected-warning {{'f1' is left uninitialized}} expected-warning {{'f3' is left uninitialized}}
+  S d; // expected-warning {{uninitialized}} expected-note {{constructor}}
+  (void)b;
+  (void)c;
+  (void)d;
+}

[cor3ntin]

@AaronBallman I think this is worth of an RFC, WDYT?

[AaronBallman]

Thank you for this!

@AaronBallman I think this is worth of an RFC, WDYT?

Yes, this should definitely get an RFC. Some things worth discussing in the RFC:

• Is there a larger design here beyond just fields? e.g., what about local variables?
• Should there be a class-level attribute for cases where you want all fields to be handled the same way?
• Why is the default to assume all fields don't require initialization? Would it make more sense to have a driver flag to opt in to a mode where it's an error to not initialize a field unless it has an attribute saying it's okay to leave it uninitialized?
• Does there need to be a way to silence the diagnostic on a case-by-case basis? e.g., the field should always be initialized explicitly except in one special case that needs to opt out of that behavior

(I'm sure there are other questions, but basically, it's good to have a big-picture understanding of why a particular design is the way you think we should go.)

[higher-performance]

RFC: https://discourse.llvm.org/t/rfc-add-clang-attribute-to-ensure-that-fields-are-initialized-explicitly/80626

[higher-performance]

I updated the PR to change the attribute name from explicit to explicit_init and to clarify the error message that initializations must be explicit.

@AaronBallman are we good to move forward?

[ilya-biryukov]

I hope folks are ok with me chiming in as a reviewer for this.
I've left quite a few comments in the RFC and is also supportive of landing this change and happy to invest into supporting it going forward inside our team.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ilya-biryukov]

I think there are some good parallel disscussions happening in the RFC, but despite their outcomes, we could probably update the PR to capture current behavior in those interesting cases.

I left a few comments along those lines, PTAL.

[ilya-biryukov]

See my comment about VerifyOnly and duplicate diagnostics.
The rest are small NITs.

[ilya-biryukov]

As an idea for future improvements: we could also collect all unitialized fields and emit a single diagnostic that lists them all (with notes to the locations of the fields).

However, I think this is good enough for the first version, I don't necessarily feel we should do it right away.

[higher-performance]

Yeah that's more complicated than I can invest in right now, it's something we can do in the future though.

[ilya-biryukov]

@AaronBallman @cor3ntin I believe we are getting close to finalizing this PR.
Would you be okay with this feature landing and myself approving this when it's ready?

There was some discussion here and in the RFC, but I don't think there was explicit approval (or objection) to land this, so I wanted to clarify this.

[cor3ntin]

I don't think the rfc has reached its conclusion yet, and consensus has not been called (for example, i still need to think about whether my questions were addressed) so we should wait for the RFC process before continuing with that PR.

Thanks

[ilya-biryukov]

I don't think the rfc has reached its conclusion yet, and consensus has not been called (for example, i still need to think about whether my questions were addressed) so we should wait for the RFC process before continuing with that PR.

Thanks

Thanks for explicitly calling this out. There were no replies from you on the RFC for some time, so it was unclear whether there is anything left. We will be waiting for your feedback on Discourse.

[cor3ntin]

Usually the process requires someone like Aaron to call consensus either way. we are aware that establishing consensus is a bit nebulous at the moment but it's something we are working on improving.

…

On Fri, Sep 13, 2024, 16:03 Ilya Biryukov ***@***.***> wrote: I don't think the rfc has reached its conclusion yet, and consensus has not been called (for example, i still need to think about whether my questions were addressed) so we should wait for the RFC process before continuing with that PR. Thanks Thanks for explicitly calling this out. There were no replies from you on the RFC for some time, so it was unclear whether there is anything left. We will be waiting for your feedback on Discourse. — Reply to this email directly, view it on GitHub <#102040 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKX7633PBO3XB4FJRFVRGTZWLWCNAVCNFSM6AAAAABMA3ZTVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBZGA2DCMRWGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[erichkeane]

Another one I'd like Aaron's opinion here.

[erichkeane]

Marking 'request-changes' not as there are changes needed, but that I have concerns about the implementation that I have to spend some time in a debugger to assuage.

[erichkeane]

There isn't a way to remove the 'request changes' without approval, but I did some debugging and see what I was assuming was wrong. So I don't have any concerns on the approach.

However, in using it for the last few hours, I REALLY dislike the diagnostic that you get. I think the additional effort (which is, IMO, fairly minor once you have a RecordDecl) of printing the first field is worth it, and necessary for me. The function should be pretty easy though, and you mentioned not really having much time, so here's what it should look like more or less:

FieldDecl *getFirstExplicitInitField(RecordDecl *RD) {
  // Current class field seems worth it, and gives the best answer in 99% of uses. Other 1% is when there is just fields in bases.
  for (FieldDecl *FD : RD) {
    if (FD->hasAttr<ExplicitInitAttr>())
      return FD;
  }
  // DFS for bases, but it doesn't seem worth the effort to work much harder to make sure we do the 'shallowest' field, which, while better diagnostics, is comparatively rare.
  if (auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
    for (CXXBaseSpecifier BS : CXXRD->bases()) {
      CXXRecordDecl *Base = BS.getType()->getAsCXXRecordDecl();
      if (Base->hasUninitializedExplicitInitFields())
        if (FieldDecl *FD = getFirstExplicitInitField(Base))
          return FD;
    }
    // TODO: Figure out whether `bases` contains virtual bases, and not just non-virtual bases.  If not, do the above on `RecordDecl::vbases`
  }
}

That should be within a bit of const-correctness/few typos from doing what you need.

THOUGH: that makes me wonder, have you played with virtual bases yet? Are we 'sane' for those?

[higher-performance]

THOUGH: that makes me wonder, have you played with virtual bases yet? Are we 'sane' for those?

I have not, but those can't be aggregates, right?

The function should be pretty easy though

// DFS for bases, but it doesn't seem worth the effort to work much harder to make sure we do the 'shallowest' field, which, while better diagnostics, is comparatively rare.

Your comment is exactly hinting at why I said this is neither easy, nor efficient. Notice that the fields might not be directly inside the class at all. They might be embedded in some deep (even anonymous) subitem of array of anonymous union of subobject of some far-away base. Digging them up is nowhere as trivial as in your example, and it requires traversing a potentially very expansive object tree. You could easily traverse 1,000+ fields (possibly inside precompiled modules/headers) to dig up 1 name. That seems... not great?

If you're happy with digging up the field name only in a few simple cases (like if it's a direct member of the class or a direct member of one of its direct bases) then sure it's as trivial as your code suggests, and we could do that, but that wouldn't cover everything.

[erichkeane]

THOUGH: that makes me wonder, have you played with virtual bases yet? Are we 'sane' for those?

I have not, but those can't be aggregates, right?

Right, good, yeah, that shouldn't be a problem then.

The function should be pretty easy though

// DFS for bases, but it doesn't seem worth the effort to work much harder to make sure we do the 'shallowest' field, which, while better diagnostics, is comparatively rare.

Your comment is exactly hinting at why I said this is neither easy, nor efficient. Notice that the fields might not be directly inside the class at all. They might be embedded in some deep (even anonymous) subitem of array of anonymous union of subobject of some far-away base. Digging them up is nowhere as trivial as in your example, and it requires traversing a potentially very expansive object tree. You could easily traverse 1,000+ fields (possibly inside precompiled modules/headers) to dig up 1 name. That seems... not great?

If you're happy with digging up the field name only in a few simple cases (like if it's a direct member of the class or a direct member of one of its direct bases) then sure it's as trivial as your code suggests, and we could do that, but that wouldn't cover everything.

I very much think we need to 'do better'. Upon further reflection, I would think that any amount of depth beyond direct fields is perhaps gilding the lily (and would probably require breadcrumbs), so direct fields is perhaps good enough.

That said, the diagnostic was really quite opaque when trying to use it/figure out what I was doing. We need to improve the diagnostics in those cases to make it more clear(at least SOME level of hint) to users of this feature what they are looking for. For simple Fields, we can just print the 1st, for types in fields/more complicated fields, perhaps just point at the field (and mention the type name that causes the problem), and do a similar thing for the base classes.

[ilya-biryukov]

@AaronBallman this seems to have progressed much further after your review and is currently pending on input from you. Could you please do a round of review here/in the RFC? (Or ask someone else from the community to address your concerns and be a reviewer instead, not sure if that's more useful)

@cor3ntin and also the same question. Could you take another look to see if your concerns were addressed?

[erichkeane]

@AaronBallman this seems to have progressed much further after your review and is currently pending on input from you. Could you please do a round of review here/in the RFC? (Or ask someone else from the community to address your concerns and be a reviewer instead, not sure if that's more useful)

@cor3ntin and also the same question. Could you take another look to see if your concerns were addressed?

IIRC, Aaron is away this week (and was away last week for WG14). So we'll have to wait on him a bit.

[erichkeane]

See: https://discourse.llvm.org/t/review-availability/81342

[higher-performance]

@erichkeane I ended up just implementing a full-ish traversal since it didn't. I'm not entirely sure if it might miss any corner cases (like anonymous objects etc.), but it should handle any cases that practically come up. Please take another look.

[erichkeane]

Just about there IMO, still have an open on the two diagnostic things, but otherwise only a few nit-like things (but please update the tests, it makes it much easier to review/update in the future)

[erichkeane]

I'm happy, pending Aaron's comments on the two diagnostic things.

[higher-performance]

Hi all, we've been waiting a few weeks for approval on this (from @AaronBallman I imagine), but I'm guessing there's a lack of bandwidth on the reviewers' sides. Is there a way we can move forward?

[erichkeane]

We still want the final OK from Aaron, but he's at 35k feet right now on the way back from LLVM-Dev, so hopefully he'll have time to do a pass next week.

[erichkeane]

1 more pass thru, just noticed 1 thing, plus highlighting things aaron needs to look at.

[erichkeane]

Need Aaron's comment, re: should this be its own diagnostic group?

[erichkeane]

Another place for Aaron, whether warn_field_requires_explicit_init and warn_attribute_needs_aggregate should be errors, or can remain warnings.

[AaronBallman]

We still want the final OK from Aaron, but he's at 35k feet right now on the way back from LLVM-Dev, so hopefully he'll have time to do a pass next week.

I'm back in the office now and digging out from under last week. I plan to call consensus (for or against) on the RFC this week as conversation seems to have died down on it, so if anyone has last minute arguments for/against the feature, please leave comments on Discourse!