Allow `__attribute__((warn_unused))` on individual constructors

[sberg]

Summary

This takes inspiration from the standard nodiscard attribute, which can also be declared on individual constructors. (I have used this change locally for about a year on the LibreOffice code base, marking various compilers as warn_unused, without any positive or negative effects. Until I happened to notice an additional constructor that would benefit from warn_unused, and which found about 20 unused variables across the LibreOffice code base, see e.g., https://git.libreoffice.org/core/+/7cdbe504ff3b59d3aec1b1e86caf7f24223dce72%5E! "Fix what looks like copy/paste typos".)

(The changes in clang/test/Misc/pragma-attribute-strict-subjects.c, clang/test/Parser/pragma-attribute.cpp, and clang/test/Sema/pragma-attribute-strict-subjects.c are necessary to make those tests not fail after adding SubRuleForCXXConstructor to SubjectMatcherForFunction in clang/include/clang/Basic/Attr.td. It appears that the exact diagnostic output that is generated is somewhat brittle, depending on implementation details.)

[erichkeane]

I don't see a problem with this, but the changes to the pragma tests concern me a bit, so I'd like aaron to scroll through. The Expr.cpp and Sema changes both look right to me.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

189–190

clang/include/clang/Basic/Attr.td

2996

I'm confused -- if you want this to behave like nodiscard, why aren't these changes for warn_unused_result (which is what implements nodiscard)? I don't think this should go on warn_unused because that's for the declaration of the type as a unit and not written on functions.

I think you don't need any changes to Attr.td for this functionality.

[sberg]

clang/include/clang/Basic/Attr.td

2996

[[nodiscard]] and __attribute__((warn_unused)) already have different semantics when applied to a class: While both warn about unused expression results (-Wunused-value), only the latter warns about unused variables (-Wunused-variable). This patch keeps that difference, just extends it to individual constructors:

$ cat test.cc
struct [[nodiscard]] S1 {
  S1(int) {}
  S1(double) {}
};
struct S2 {
  [[nodiscard]] S2(int) {}
  S2(double) {}
};
struct __attribute__((warn_unused)) S3 {
  S3(int) {}
  S3(double) {}
};
struct S4 {
  __attribute__((warn_unused)) S4(int) {}
  S4(double) {}
};
int main() {
  S1(0); // expected-warning {{ignoring temporary}}
  S1(0.0); // expected-warning {{ignoring temporary}}
  S2(0); // expected-warning {{ignoring temporary}}
  S2(0.0);
  S3(0); // expected-warning {{expression result unused}}
  S3(0.0); // expected-warning {{expression result unused}}
  S4(0); // expected-warning {{expression result unused}}
  S4(0.0);
  S1 s1a(0);
  S1 s1b(0.0);
  S2 s2a(0);
  S2 s2b(0.0);
  S3 s3a(0); // expected-warning {{unused variable}}
  S3 s3b(0.0); // expected-warning {{unused variable}}
  S4 s4a(0); // expected-warning {{unused variable}}
  S4 s4b(0.0);
}

$ clang++ -Wunused-value -Wunused-variable -fsyntax-only -Xclang -verify test.cc

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2996

Hmmm, well, that's compelling and now I don't know what I think.

I'm worried that this further muddies the water between warn_unused and warn_unused_result which are already pretty murky to begin with. warn_unused_result goes on functions, warn_unused goes on the declaration of a type. That symmetry is something I think people can understand and remember despite the attribute names. With this patch, now it's warn_unused_result goes on functions, and warn_unused goes on constructor functions or the declaration of a type. It's not the worst thing ever (constructors don't really have "results" for example). But the fact that these two attributes already confuse users (AND we've got unused as an attribute in the mix as well, which doesn't help) means we should proceed with caution (and likely coordinate with GCC given that all three of these attributes came from their implementation).

I'm not certain what the best design approach is yet, but I'll think about the problem a bit more over the next few days to see if my thoughts crystalize on this.

(FWIW, if we do decide to go with warn_unused, I think it's no longer defensible to leave this attribute as undocumented in Clang -- we should take the opportunity to push up some documentation for this one.)

[erichkeane]

clang/include/clang/Basic/Attr.td

2996

To double-down on what Aaron says: He and I discussed this extensively offline, and we are both really quite unsure what to do here. I can make a reasonable/good argument to convince myself and Aaron in either direction (and he's been able to convince me both ways!), so I also think the ergonomics of these two needs to be defined/fleshed out for this.

[sberg]

But the fact that these two attributes already confuse users (AND we've got unused as an attribute in the mix as well, which doesn't help) means we should proceed with caution (and likely coordinate with GCC given that all three of these attributes came from their implementation).

I've brought this up at https://gcc.gnu.org/pipermail/gcc/2023-April/241205.html "GCC/Clang attributes guiding warnings about unused entities" now. (I tried to cross-post that also to the Clang Frontend discourse category, but that probably didn't work.)

[aaron.ballman]

In D148505#4300483, @sberg wrote:

But the fact that these two attributes already confuse users (AND we've got unused as an attribute in the mix as well, which doesn't help) means we should proceed with caution (and likely coordinate with GCC given that all three of these attributes came from their implementation).

I've brought this up at https://gcc.gnu.org/pipermail/gcc/2023-April/241205.html "GCC/Clang attributes guiding warnings about unused entities" now. (I tried to cross-post that also to the Clang Frontend discourse category, but that probably didn't work.)

Thank you for poking on this! FWIW, I don't know that there's a way to cross-post to Discourse (but if I'm wrong and there is, I'd love to know how!).

[sberg]

In D148505#4302702, @aaron.ballman wrote:

Thank you for poking on this! FWIW, I don't know that there's a way to cross-post to Discourse (but if I'm wrong and there is, I'd love to know how!).

Ping, any further input from anybody?

(The cross-posting didn't work, but there was also almost no response on the GCC side, https://gcc.gnu.org/pipermail/gcc/2023-April/241220.html "Re: GCC/Clang attributes guiding warnings about unused entities".)

[aaron.ballman]

In D148505#4331647, @sberg wrote:

In D148505#4302702, @aaron.ballman wrote:

Thank you for poking on this! FWIW, I don't know that there's a way to cross-post to Discourse (but if I'm wrong and there is, I'd love to know how!).

Ping, any further input from anybody?

(The cross-posting didn't work, but there was also almost no response on the GCC side, https://gcc.gnu.org/pipermail/gcc/2023-April/241220.html "Re: GCC/Clang attributes guiding warnings about unused entities".)

I've thought about it a bit since the last time around, and I'm still kind of uncomfortable with the changes especially given that GCC folks don't seem to have an appetite to change the behavior. However, I'm also not firmly opposed to the idea either as there is some nice symmetry. Most of my concerns really boil down to the ergonomics of how all the various attributes in this space are slightly different from one another; unless GCC goes the same direction, this is one more difference between the attributes that you need to worry about. However, at least GCC warns that they're ignoring the attribute when written on a constructor: https://godbolt.org/z/bzTojhs1f so perhaps this difference is reasonable so long as we have some amazing documentation.

I'm not certain if @erichkeane has new thoughts or not.

[erichkeane]

I'm still on the fence. I think properly documenting all of this entire attribute would make me more comfortable here, particularly since it will allow GCC to follow us easily some day. I think that would move the needle for me.

clang/include/clang/Basic/Attr.td

2997

For this to go through, I think we'd want this properly documented.

[aaron.ballman]

In D148505#4356951, @erichkeane wrote:

I'm still on the fence. I think properly documenting all of this entire attribute would make me more comfortable here, particularly since it will allow GCC to follow us easily some day. I think that would move the needle for me.

I think I've got a slightly different idea there: let's add a new documentation section for all of the related attributes in this area and document (or update) all of them in a block. This way, users have a good way to answer "which one do I use in what circumstance?" as well as other compiler devs who want to match our behavior. I think if we can make the documentation sufficiently clear, then this is fine.

clang/docs/ReleaseNotes.rst

- ``__declspec`` attributes can now be used together with the using keyword. Before
   point where the alias is used.
 - Introduced a new ``USR`` (unified symbol resolution) clause inside of the
   existing ``__attribute__((external_source_symbol))`` attribute. Clang's indexer
   uses the optional USR value when indexing Clang's AST. This value is expected
   to be generated by an external compiler when generating C++ bindings during
   the compilation of the foreign language sources (e.g. Swift).
 - The ``__has_attribute``, ``__has_c_attribute`` and ``__has_cpp_attribute``
   preprocessor operators now return 1 also for attributes defined by plugins.
+- In C++, the ``__attribute__((warn_unused))`` can now also be used on individual
+  constructors,
 
 Improvements to Clang's diagnostics
 -----------------------------------
 - We now generate a diagnostic for signed integer overflow due to unary minus
   in a non-constant expression context.
   (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`_)
 - Clang now warns by default for C++20 and later about deprecated capture of
   ``this`` with a capture default of ``=``. This warning can be disabled with

clang/include/clang/Basic/Attr.td

class AttrSubjectMatcherRule<string name, list<AttrSubject> subjects,
   // attribute to be applicable. If empty, no language options are required.
   list<LangOpt> LangOpts = [];
 }
 
 // function(is_member)
 def SubRuleForCXXMethod : AttrSubjectMatcherSubRule<"is_member", [CXXMethod]> {
   let LangOpts = [CPlusPlus];
 }
+def SubRuleForCXXConstructor :
+    AttrSubjectMatcherSubRule<"constructor", [CXXConstructor]> {
+  let LangOpts = [CPlusPlus];
+}
 def SubjectMatcherForFunction : AttrSubjectMatcherRule<"function", [Function], [
-  SubRuleForCXXMethod
+  SubRuleForCXXMethod, SubRuleForCXXConstructor
 ]>;
 // hasType is abstract, it should be used with one of the sub-rules.
 def SubjectMatcherForType : AttrSubjectMatcherRule<"hasType", [], [
   AttrSubjectMatcherSubRule<"functionType", [FunctionLike]>
 
   // FIXME: There's a matcher ambiguity with objc methods and blocks since
   // functionType excludes them but functionProtoType includes them.
   // AttrSubjectMatcherSubRule<"functionProtoType", [HasFunctionProto]>
def VecReturn : InheritableAttr {
   // FIXME: should this attribute have a CPlusPlus language option?
   let Spellings = [Clang<"vecreturn", 0>];
   let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 def WarnUnused : InheritableAttr {
   let Spellings = [GCC<"warn_unused">];
-  let Subjects = SubjectList<[Record]>;
+  let Subjects = SubjectList<[Record, CXXConstructor]>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
 def WarnUnusedResult : InheritableAttr {
   let Spellings = [CXX11<"", "nodiscard", 201907>,
                    C2x<"", "nodiscard", 202003>,
                    CXX11<"clang", "warn_unused_result">,
                    GCC<"warn_unused_result">];

clang/include/clang/Basic/DeclNodes.td

def Value : DeclNode<Named, "value declarations", 1>;
     def Declarator : DeclNode<Value, "declarators", 1>;
       def Field : DeclNode<Declarator, "non-static data members">;
         def ObjCIvar : DeclNode<Field>;
         def ObjCAtDefsField : DeclNode<Field>;
       def MSProperty : DeclNode<Declarator>;
       def Function : DeclNode<Declarator, "functions">, DeclContext;
         def CXXDeductionGuide : DeclNode<Function>;
         def CXXMethod : DeclNode<Function>;
-          def CXXConstructor : DeclNode<CXXMethod>;
+          def CXXConstructor : DeclNode<CXXMethod, "constructors">;
           def CXXDestructor : DeclNode<CXXMethod>;
           def CXXConversion : DeclNode<CXXMethod>;
       def Var : DeclNode<Declarator, "variables">;
         def VarTemplateSpecialization : DeclNode<Var>;
           def VarTemplatePartialSpecialization
             : DeclNode<VarTemplateSpecialization>;
         def ImplicitParam : DeclNode<Var>;
         def ParmVar : DeclNode<Var, "parameters">;

clang/lib/AST/Expr.cpp

bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
   case RecoveryExprClass:
     return false;
 
   case CXXTemporaryObjectExprClass:
   case CXXConstructExprClass: {
     if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
       const auto *WarnURAttr = Type->getAttr<WarnUnusedResultAttr>();
       if (Type->hasAttr<WarnUnusedAttr>() ||
-          (WarnURAttr && WarnURAttr->IsCXX11NoDiscard())) {
+          (WarnURAttr && WarnURAttr->IsCXX11NoDiscard()) ||
+          cast<CXXConstructExpr>(this)->getConstructor()->hasAttr<WarnUnusedAttr>()) {
         WarnE = this;
         Loc = getBeginLoc();
         R1 = getSourceRange();
         return true;
       }
     }
 
     const auto *CE = cast<CXXConstructExpr>(this);

clang/lib/Sema/SemaDecl.cpp

if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     Ty = Ty->getBaseElementTypeUnsafe();
 
     if (const TagType *TT = Ty->getAs<TagType>()) {
       const TagDecl *Tag = TT->getDecl();
       if (Tag->hasAttr<UnusedAttr>())
         return false;
 
       if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(Tag)) {
-        if (!RD->hasTrivialDestructor() && !RD->hasAttr<WarnUnusedAttr>())
-          return false;
-
+        bool ConstructorWarnUnused = false;
         if (Init) {
           const CXXConstructExpr *Construct =
             dyn_cast<CXXConstructExpr>(Init);
-          if (Construct && !Construct->isElidable()) {
+          if (Construct) {
             CXXConstructorDecl *CD = Construct->getConstructor();
-            if (!CD->isTrivial() && !RD->hasAttr<WarnUnusedAttr>() &&
+            ConstructorWarnUnused = CD->hasAttr<WarnUnusedAttr>();
+            if (!Construct->isElidable() && !CD->isTrivial() &&
+                !RD->hasAttr<WarnUnusedAttr>() && !ConstructorWarnUnused &&
                 (VD->getInit()->isValueDependent() || !VD->evaluateValue()))
               return false;
           }
 
           // Suppress the warning if we don't know how this is constructed, and
           // it could possibly be non-trivial constructor.
           if (Init->isTypeDependent()) {
             for (const CXXConstructorDecl *Ctor : RD->ctors())
               if (!Ctor->isTrivial())
                 return false;
           }
 
           // Suppress the warning if the constructor is unresolved because
           // its arguments are dependent.
           if (isa<CXXUnresolvedConstructExpr>(Init))
             return false;
         }
+
+        if (!RD->hasTrivialDestructor() && !RD->hasAttr<WarnUnusedAttr>()
+            && !ConstructorWarnUnused)
+          return false;
       }
     }
 
     // TODO: __attribute__((unused)) templates?
   }
 
   return true;
 }

clang/test/Misc/pragma-attribute-strict-subjects.c

 int testRecoverSubRuleContradiction = 0;
 // CHECK-LABEL: VarDecl{{.*}} testRecoverSubRuleContradiction
 // CHECK-NEXT: IntegerLiteral
 // CHECK-NEXT: AnnotateAttr{{.*}} "subRuleContradictions"
 
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((annotate("negatedSubRuleContradictions2"))), apply_to = any(variable(unless(is_parameter)), variable(is_thread_local), function, variable(is_global)))
-// expected-error@-1 {{negated attribute subject matcher sub-rule 'variable(unless(is_parameter))' contradicts sub-rule 'variable(is_global)'}}
+// expected-error@-1 {{negated attribute subject matcher sub-rule 'variable(unless(is_parameter))' contradicts sub-rule 'variable(is_thread_local)'}}
 // We have just one error, don't error on 'variable(is_global)'
 
 // Ensure that we've recovered from the error:
 int testRecoverNegatedContradiction = 0;
 // CHECK-LABEL: VarDecl{{.*}} testRecoverNegatedContradiction
 // CHECK-NEXT: IntegerLiteral
 // CHECK-NEXT: AnnotateAttr{{.*}} "negatedSubRuleContradictions2"
 

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: TargetVersion (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local)
 // CHECK-NEXT: UnsafeBufferUsage (SubjectMatchRule_function)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
-// CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
+// CHECK-NEXT: WarnUnused (SubjectMatchRule_record, SubjectMatchRule_function_constructor)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportModule (SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyImportName (SubjectMatchRule_function)
 // CHECK-NEXT: WorkGroupSizeHint (SubjectMatchRule_function)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: ZeroCallUsedRegs (SubjectMatchRule_function)
 // CHECK-NEXT: End of supported attributes.

clang/test/Parser/pragma-attribute.cpp

 #pragma clang attribute push([[clang::uninitialized]], apply_to = function) // expected-error {{attribute 'uninitialized' can't be applied to 'function'}}
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::uninitialized]], apply_to = variable) // expected-error {{attribute 'uninitialized' can't be applied to 'variable'}}
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::uninitialized]], apply_to = variable(is_thread_local)) // expected-error {{attribute 'uninitialized' can't be applied to 'variable(is_thread_local)'}}
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::uninitialized]], apply_to = variable(is_global)) // expected-error {{attribute 'uninitialized' can't be applied to 'variable(is_global)'}}
 #pragma clang attribute pop
-#pragma clang attribute push([[clang::uninitialized]], apply_to = any(variable(is_parameter), variable(unless(is_parameter)))) // expected-error {{attribute 'uninitialized' can't be applied to 'variable(is_parameter)', and 'variable(unless(is_parameter))'}}
+#pragma clang attribute push([[clang::uninitialized]], apply_to = any(variable(is_parameter), variable(unless(is_parameter)))) // expected-error {{attribute 'uninitialized' can't be applied to 'variable(unless(is_parameter))', and 'variable(is_parameter)'}}
 #pragma clang attribute pop
 // We're allowed to apply attributes to subsets of allowed subjects.
 #pragma clang attribute push([[clang::no_destroy]], apply_to = variable)
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::no_destroy]], apply_to = variable(is_thread_local))
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::no_destroy]], apply_to = variable(is_global))
 #pragma clang attribute pop

clang/test/Sema/pragma-attribute-strict-subjects.c

 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((annotate("negatedSubRuleContradictions1"))), apply_to = any(variable(is_parameter), variable(unless(is_parameter))))
 // expected-error@-1 {{negated attribute subject matcher sub-rule 'variable(unless(is_parameter))' contradicts sub-rule 'variable(is_parameter)'}}
 
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((annotate("negatedSubRuleContradictions2"))), apply_to = any(variable(unless(is_parameter)), variable(is_thread_local), function, variable(is_global)))
-// expected-error@-1 {{negated attribute subject matcher sub-rule 'variable(unless(is_parameter))' contradicts sub-rule 'variable(is_global)'}}
+// expected-error@-1 {{negated attribute subject matcher sub-rule 'variable(unless(is_parameter))' contradicts sub-rule 'variable(is_thread_local)'}}
 // We have just one error, don't error on 'variable(is_global)'
 
 #pragma clang attribute pop
 
 // Verify the strict subject set verification.
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(function))
 // No error

clang/test/SemaCXX/warn-unused-attribute.cpp

 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -verify %s
 struct __attribute__((warn_unused)) Test {
   Test();
   ~Test();
   void use();
 };
 
 struct TestNormal {
   TestNormal();
 };
 
+struct TestCtor {
+  __attribute__((warn_unused)) TestCtor();
+  TestCtor(int);
+  void __attribute__((warn_unused)) f(); // expected-warning {{'warn_unused' attribute only applies to structs, unions, classes, and constructors}}
+};
+
 int main(void) {
   Test unused;         // expected-warning {{unused variable 'unused'}}
   Test used;
   TestNormal normal;
   used.use();
 
-  int i __attribute__((warn_unused)) = 12; // expected-warning {{'warn_unused' attribute only applies to structs, unions, and classes}}
+  TestCtor ctorUnused; // expected-warning {{unused variable 'ctorUnused'}}
+  TestCtor ctorUsed(0);
+
+  int i __attribute__((warn_unused)) = 12; // expected-warning {{'warn_unused' attribute only applies to structs, unions, classes, and constructors}}
   return i;
 }