[clang] Add the candiscard attribute to suppress nodiscard #154943
Conversation
Fulfills the requirement that @huixie90 stated in llvm#139651 that Clang should have a way to disable [[nodiscard]] on a function by function basis. This will allow us finally to resume fixing llvm#130656.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (halbi2) ChangesFulfills the requirement that @huixie90 and @philnik777 stated in #139651 that Clang will have a way to disable [[nodiscard]] on a function by function basis. This will allow us finally to resume fixing #130656 via #139651. Full diff: https://github.com/llvm/llvm-project/pull/154943.diff 9 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0d85b6f426995..f9d9b4355cef7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -199,6 +199,10 @@ Removed Compiler Flags
Attribute Changes in Clang
--------------------------
+- A new attribute ``[[clang::candiscard]]`` can be applied to a function returning a nodiscard type
+ to suppress the nodiscard warning on that function in particular. Also, it can be applied to
+ a typedef alias to suppress the nodiscard warning on all functions returning values of the
+ typedef type.
Improvements to Clang's diagnostics
-----------------------------------
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 29364c5903d31..c92a962802594 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3646,6 +3646,14 @@ def Unavailable : InheritableAttr {
let MeaningfulToClassTemplateDefinition = 1;
}
+def CanDiscard : InheritableAttr {
+ let Spellings = [CXX11<"clang", "candiscard">,
+ GCC<"candiscard">];
+ let Subjects = SubjectList<[ObjCMethod, FunctionLike, TypedefName]>;
+ let Documentation = [CanDiscardDocs];
+ let SimpleHandler = 1;
+}
+
def DiagnoseIf : InheritableAttr {
// Does not have a [[]] spelling because this attribute requires the ability
// to parse function arguments but the attribute is not written in the type
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index b9405c5fc86c1..524bcb0e74cab 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -2452,6 +2452,44 @@ use the annotated ``[[nodiscard]]`` constructor or result in an annotated type.
}];
}
+def CanDiscardDocs : Documentation {
+ let Category = DocCatFunction;
+ let Heading = "candiscard";
+ let Content = [{
+A function whose return type is marked with ``[[nodiscard]]`` generally cannot have
+its return value discarded, even though this may be safe in some rare situations.
+Clang allows an individual function to be marked with ``[[clang::candiscard]]``
+or ``__attribute__((candiscard))`` to override the effect of a ``[[nodiscard]]``
+return type.
+
+.. code-block:: c++
+
+ struct [[nodiscard]] error_info { /*...*/ };
+ error_info enable_missile_safety_mode();
+ [[clang::candiscard]] error_info reload_missiles();
+
+ void test_missiles() {
+ enable_missile_safety_mode(); // diagnoses
+ reload_missiles(); // does not diagnose
+ }
+
+Also, a type alias can be marked with ``[[clang::candiscard]]`` to mask the
+effect of ``[[nodiscard]]`` on the underlying type.
+
+.. code-block:: c++
+
+ struct [[nodiscard]] error_info { /*...*/ };
+ using informational_error_info [[clang::candiscard]] = error_info;
+ error_info enable_missile_safety_mode();
+ informational_error_info reload_missiles();
+
+ void test_missiles() {
+ enable_missile_safety_mode(); // diagnoses
+ reload_missiles(); // does not diagnose
+ }
+ }];
+}
+
def FallthroughDocs : Documentation {
let Category = DocCatStmt;
let Heading = "fallthrough";
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 340de6d4be934..ee1a3e5f4961b 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -1632,21 +1632,34 @@ QualType CallExpr::getCallReturnType(const ASTContext &Ctx) const {
std::pair<const NamedDecl *, const WarnUnusedResultAttr *>
Expr::getUnusedResultAttrImpl(const Decl *Callee, QualType ReturnType) {
- // If the callee is marked nodiscard, return that attribute
- if (Callee != nullptr)
+ // If the callee is marked nodiscard, return that attribute for the diagnostic.
+ // If the callee is marked candiscard, do not diagnose.
+ // If seen on the same level, candiscard beats nodiscard.
+ if (Callee != nullptr) {
+ if (const auto *A = Callee->getAttr<CanDiscardAttr>())
+ return {nullptr, nullptr};
if (const auto *A = Callee->getAttr<WarnUnusedResultAttr>())
return {nullptr, A};
+ }
- // If the return type is a struct, union, or enum that is marked nodiscard,
- // then return the return type attribute.
- if (const TagDecl *TD = ReturnType->getAsTagDecl())
- if (const auto *A = TD->getAttr<WarnUnusedResultAttr>())
- return {TD, A};
-
+ // Walk the return type's (chain of) type aliases. The first alias
+ // that is marked either nodiscard or candiscard ends the walk.
for (const auto *TD = ReturnType->getAs<TypedefType>(); TD;
- TD = TD->desugar()->getAs<TypedefType>())
+ TD = TD->desugar()->getAs<TypedefType>()) {
+ if (const auto *A = TD->getDecl()->getAttr<CanDiscardAttr>())
+ return {nullptr, nullptr};
if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>())
return {TD->getDecl(), A};
+ }
+
+ // Check whether the return type's class declaration is marked nodiscard.
+ if (const TagDecl *TD = ReturnType->getAsTagDecl()) {
+ if (const auto *A = TD->getAttr<CanDiscardAttr>())
+ return {nullptr, nullptr};
+ if (const auto *A = TD->getAttr<WarnUnusedResultAttr>())
+ return {TD, A};
+ }
+
return {nullptr, nullptr};
}
diff --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
index 37ff33e5a1523..38a771156c3cc 100644
--- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -48,6 +48,7 @@
// CHECK-NEXT: CallableWhen (SubjectMatchRule_function_is_member)
// CHECK-NEXT: Callback (SubjectMatchRule_function)
// CHECK-NEXT: CalledOnce (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: CanDiscard (SubjectMatchRule_objc_method, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
// CHECK-NEXT: Capability (SubjectMatchRule_record, SubjectMatchRule_type_alias)
// CHECK-NEXT: CarriesDependency (SubjectMatchRule_variable_is_parameter, SubjectMatchRule_objc_method, SubjectMatchRule_function)
// CHECK-NEXT: Cleanup (SubjectMatchRule_variable_is_local)
diff --git a/clang/test/Sema/c2x-nodiscard.c b/clang/test/Sema/c2x-nodiscard.c
index 852c74721693b..07d76485c3702 100644
--- a/clang/test/Sema/c2x-nodiscard.c
+++ b/clang/test/Sema/c2x-nodiscard.c
@@ -65,15 +65,31 @@ void GH104391() {
M; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
}
+struct S4 get_s_ignored(void) __attribute__((candiscard));
+enum E2 get_e_ignored(void) __attribute__((candiscard));
+typedef __attribute__((candiscard)) enum E2 EIgnored;
+EIgnored get_e_ignored2();
+
+void f4(void) {
+ get_s_ignored();
+ get_e_ignored();
+ get_e_ignored2();
+}
+
[[nodiscard]] typedef int NoDInt; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef}}
typedef __attribute__((warn_unused)) int WUInt; // expected-warning {{'warn_unused' attribute only applies to structs, unions, and classes}}
typedef __attribute__((warn_unused_result)) int WURInt;
+typedef __attribute__((candiscard)) WURInt WURIntIgnored;
NoDInt get_nodint();
WUInt get_wuint();
WURInt get_wurint();
+WURIntIgnored get_wurint_ignored();
+WURIntIgnored get_wurint_ignored2() __attribute__((candiscard));
-void f4(void) {
+void f5(void) {
get_nodint(); // no warning because attribute is ignored
get_wuint(); // no warning because attribute is ignored
get_wurint(); // expected-warning {{ignoring return value of type 'WURInt' declared with 'warn_unused_result' attribute}}
+ get_wurint_ignored(); // no warning
+ get_wurint_ignored2(); // no warning
}
diff --git a/clang/test/SemaCXX/warn-unused-result.cpp b/clang/test/SemaCXX/warn-unused-result.cpp
index 098817729efb1..48963449155a4 100644
--- a/clang/test/SemaCXX/warn-unused-result.cpp
+++ b/clang/test/SemaCXX/warn-unused-result.cpp
@@ -425,19 +425,36 @@ struct [[gnu::warn_unused_result]] WarnUnusedResult {
WarnUnusedResult(const char*);
};
+using NoDIgnored [[clang::candiscard]] = NoDiscard;
+using WUIgnored [[clang::candiscard]] = WarnUnused;
+using WURIgnored [[clang::candiscard]] = WarnUnusedResult;
+
NoDiscard return_nodiscard();
WarnUnused return_warnunused();
WarnUnusedResult return_warnunusedresult();
+NoDIgnored return_nodiscard_ignored();
+WUIgnored return_warnunused_ignored();
+WURIgnored return_warnunusedresult_ignored();
+[[clang::candiscard]] NoDiscard return_nodiscard_ignored2();
+[[clang::candiscard]] WarnUnused return_warnunused_ignored2();
+[[clang::candiscard]] WarnUnusedResult return_warnunusedresult_ignored2();
NoDiscard (*p_return_nodiscard)();
WarnUnused (*p_return_warnunused)();
WarnUnusedResult (*p_return_warnunusedresult)();
+NoDIgnored (*p_return_nodiscard_ignored)();
+WUIgnored (*p_return_warnunused_ignored)();
+WURIgnored (*p_return_warnunusedresult_ignored)();
+[[clang::candiscard]] NoDiscard (*p_return_nodiscard_ignored2)();
+[[clang::candiscard]] WarnUnused (*p_return_warnunused_ignored2)();
+[[clang::candiscard]] WarnUnusedResult (*p_return_warnunusedresult_ignored2)();
NoDiscard (*(*pp_return_nodiscard)())();
WarnUnused (*(*pp_return_warnunused)())();
WarnUnusedResult (*(*pp_return_warnunusedresult)())();
template <class T> T from_a_template();
+template <class T> [[clang::candiscard]] T from_a_template_ignored();
void test() {
// Unused but named variables
@@ -474,11 +491,23 @@ void test() {
return_nodiscard(); // expected-warning {{ignoring return value of type 'NoDiscard' declared with 'nodiscard' attribute}}
return_warnunused(); // no warning
return_warnunusedresult(); // expected-warning {{ignoring return value of type 'WarnUnusedResult' declared with 'gnu::warn_unused_result' attribute}}
+ return_nodiscard_ignored(); // no warning
+ return_warnunused_ignored(); // no warning
+ return_warnunusedresult_ignored(); // no warning
+ return_nodiscard_ignored2(); // no warning
+ return_warnunused_ignored2(); // no warning
+ return_warnunusedresult_ignored2(); // no warning
// Function pointer return values
p_return_nodiscard(); // expected-warning {{ignoring return value of type 'NoDiscard' declared with 'nodiscard' attribute}}
p_return_warnunused(); // no warning
p_return_warnunusedresult(); // expected-warning {{ignoring return value of type 'WarnUnusedResult' declared with 'gnu::warn_unused_result' attribute}}
+ p_return_nodiscard_ignored(); // no warning
+ p_return_warnunused_ignored(); // no warning
+ p_return_warnunusedresult_ignored(); // no warning
+ p_return_nodiscard_ignored2(); // no warning
+ p_return_warnunused_ignored2(); // no warning
+ p_return_warnunusedresult_ignored2(); // no warning
// Function pointer expression return values
pp_return_nodiscard()(); // expected-warning {{ignoring return value of type 'NoDiscard' declared with 'nodiscard' attribute}}
@@ -489,6 +518,16 @@ void test() {
from_a_template<NoDiscard>(); // expected-warning {{ignoring return value of type 'NoDiscard' declared with 'nodiscard' attribute}}
from_a_template<WarnUnused>(); // no warning
from_a_template<WarnUnusedResult>(); // expected-warning {{ignoring return value of type 'WarnUnusedResult' declared with 'gnu::warn_unused_result' attribute}}
+
+ // In a template instantiation the information about the typedef is lost,
+ // so the candiscard attribute is lost, so the diagnostic is not suppressed
+ from_a_template<NoDIgnored>(); // expected-warning {{ignoring return value of type 'NoDiscard' declared with 'nodiscard' attribute}}
+ from_a_template<WUIgnored>(); // no warning
+ from_a_template<WURIgnored>(); // expected-warning {{ignoring return value of type 'WarnUnusedResult' declared with 'gnu::warn_unused_result' attribute}}
+
+ from_a_template_ignored<NoDiscard>(); // no warning
+ from_a_template_ignored<WarnUnused>(); // no warning
+ from_a_template_ignored<WarnUnusedResult>(); // no warning
}
} // namespace candiscard
diff --git a/clang/test/SemaObjC/attr-nodiscard.m b/clang/test/SemaObjC/attr-nodiscard.m
index 26bbd247d4a3d..71bb2bb355220 100644
--- a/clang/test/SemaObjC/attr-nodiscard.m
+++ b/clang/test/SemaObjC/attr-nodiscard.m
@@ -6,6 +6,8 @@
[[nodiscard]] typedef int NI; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef}}
typedef __attribute__((warn_unused_result)) int WUR;
+typedef __attribute__((candiscard)) struct expected EIgnored;
+typedef __attribute__((candiscard)) WUR WURIgnored;
@interface INTF
- (int) a [[nodiscard]];
@@ -13,10 +15,15 @@ + (int) b [[nodiscard]];
- (struct expected) c;
+ (struct expected) d;
- (E) e;
+- (EIgnored) e_ignored;
+- (E) e_ignored2 __attribute__((candiscard));
+ (E) f;
- (void) g [[nodiscard]]; // expected-warning {{attribute 'nodiscard' cannot be applied to Objective-C method without return value}}
- (NI) h;
+- (NI) h_ignored __attribute__((candiscard));
- (WUR) i;
+- (WURIgnored) i_ignored;
+- (WUR) i_ignored2 __attribute__((candiscard));
@end
void foo(INTF *a) {
@@ -28,5 +35,8 @@ void foo(INTF *a) {
[INTF f]; // expected-warning {{ignoring return value of type 'expected' declared with 'nodiscard' attribute}}
[a g]; // no warning because g returns void
[a h]; // no warning because attribute is ignored when applied to a typedef
+ [a h_ignored]; // no warning
[a i]; // expected-warning {{ignoring return value of type 'WUR' declared with 'warn_unused_result' attribute}}
+ [a i_ignored]; // no warning
+ [a i_ignored2]; // no warning
}
diff --git a/clang/test/SemaObjCXX/attr-nodiscard.mm b/clang/test/SemaObjCXX/attr-nodiscard.mm
index 18d829632e428..7a4e101049093 100644
--- a/clang/test/SemaObjCXX/attr-nodiscard.mm
+++ b/clang/test/SemaObjCXX/attr-nodiscard.mm
@@ -8,16 +8,26 @@
using NI [[nodiscard]] = int; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef}}
using WURI [[clang::warn_unused_result]] = int;
+using EIgnored [[clang::candiscard]] = E;
+using NIIgnored [[clang::candiscard]] = NI;
+using WURIgnored [[clang::candiscard]] = WURI;
+
@interface INTF
- (int) a [[nodiscard]];
+ (int) b [[nodiscard]];
- (expected<int>) c;
+ (expected<int>) d;
- (E) e;
+- (EIgnored) e_ignored;
+- (E) e_ignored2 [[clang::candiscard]];
+ (E) f;
- (void) g [[nodiscard]]; // expected-warning {{attribute 'nodiscard' cannot be applied to Objective-C method without return value}}
- (NI) h;
+- (NIIgnored) h_ignored;
+- (NI) h_ignored2 [[clang::candiscard]];
- (WURI) i;
+- (WURIgnored) i_ignored;
+- (WURI) i_ignored2 [[clang::candiscard]];
@end
void foo(INTF *a) {
@@ -26,8 +36,14 @@ void foo(INTF *a) {
[a c]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
[INTF d]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
[a e]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
+ [a e_ignored]; // no warning
+ [a e_ignored2]; // no warning
[INTF f]; // expected-warning {{ignoring return value of type 'expected<int>' declared with 'nodiscard' attribute}}
[a g]; // no warning because g returns void
[a h]; // no warning because attribute is ignored
+ [a h_ignored]; // no warning
+ [a h_ignored2]; // no warning
[a i]; // expected-warning {{ignoring return value of type 'WURI' declared with 'clang::warn_unused_result' attribute}}
+ [a i_ignored]; // no warning
+ [a i_ignored2]; // no warning
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thank you working on this! Can you explain why we introduce this new attribute in summary? |
|
|
||
| Attribute Changes in Clang | ||
| -------------------------- | ||
| - A new attribute ``[[clang::candiscard]]`` can be applied to a function returning a nodiscard type |
There was a problem hiding this comment.
What do you think of a shorter name [[clang::discard]]? I have no preference for this。
There was a problem hiding this comment.
IMO discardable would be a good name, but I'm not super attached to any of the names so far.
There was a problem hiding this comment.
discardable looks good to me.
There was a problem hiding this comment.
Where did we land on a name? I'm not particularly attached to anything, but this discussion happened and I didn't see any response to it.
There was a problem hiding this comment.
I’m not aware of any discussion but perhaps I just missed it; discardable sgtm though.
There was a problem hiding this comment.
Would like some tests on templated type aliases
(template<typename T> using RetTy = SomeTempl::type) to make sure that this properly suppresses, or collects the type. Particularly when RetTy, or ::type has either /both of the attributes (that is, the cartesian product of options for alias vs type).
| def CanDiscard : InheritableAttr { | ||
| let Spellings = [CXX11<"clang", "candiscard">, | ||
| GCC<"candiscard">]; | ||
| let Subjects = SubjectList<[ObjCMethod, FunctionLike, TypedefName]>; |
There was a problem hiding this comment.
Are we sure about this subject list? ObjCMethod seems odd here, and FunctionLike ALSO allows this to be added to function/mem function pointers/references/etc. I suspect this whole list should just be Function + TypedefName (though maybe Typedef and TypeAlias, as including Objective C ParamDecls seems odd here).
IF you leave any of the above, please make sure that ALL of the FunctionLike has well defined semantics/etc, and that the ObjectiveC types are tested/have well defined semantics.
There was a problem hiding this comment.
This is the same subject list as [[nodiscard]]. Functions and function pointers are tested. What is the "etc."?
There was a problem hiding this comment.
See: https://clang.llvm.org/doxygen/DeclBase_8cpp_source.html#l01188
IF you want to support all of those, we need to see tests for all of those.I was trying to help limit work by making this apply to functions ONLY, that way we can expand it if/when someone requests it.
There was a problem hiding this comment.
I have added tests for member function pointers. Function references should work the same as functions and function pointers. I could not find any tests for warn_unused_result or nodiscard applied to blocks... should I start a new test.cpp file for blocks?
There was a problem hiding this comment.
Yes, that seems reasonable. We should still have function reference tests all the same. IMO, I'm not sure the value is there for having this on pointers/references though.
| // that is marked either nodiscard or candiscard ends the walk. | ||
| for (const auto *TD = ReturnType->getAs<TypedefType>(); TD; | ||
| TD = TD->desugar()->getAs<TypedefType>()) | ||
| TD = TD->desugar()->getAs<TypedefType>()) { |
There was a problem hiding this comment.
Can we make sure we have tests where this is done on a template alias? We want to make sure semantics for this are sensible there too.
There was a problem hiding this comment.
Bug #68456 prevents these attributes from working on alias templates
@yronglin Compiler support for this new attribute is required by @philnik777 before libc++ can mark its Personally I do not think this attribute will be used by programmers, it is just purely a requirement for fixing #130656. |
The new attribute is meant to be used by end users. Right now, in the stl, there is no function that returns std::expected yet, so we don’t actually need to use the new attribute within the library. What we want is to mark the type std::expected nodiscard, but currently if a user (library writer) wants to create a function that returns std::expected, there is no way for the library writer to say “it is ok to discard the return value” and this new attribute is the right tool for those library writers (not for the Libc++ ourself) |
|
@erichkeane would you look again please? |
I don’t think I have any further comments; I think the implementation lgtm but I’ll leave it to @erichkeane to approve this. |
|
Is an RFC required? CC @cor3ntin |
| A function whose return type is marked with ``[[nodiscard]]`` generally cannot have | ||
| its return value discarded, even though this may be safe in some rare situations. | ||
| Clang allows an individual function to be marked with ``[[clang::candiscard]]`` | ||
| or ``__attribute__((candiscard))`` to override the effect of a ``[[nodiscard]]`` | ||
| return type. |
There was a problem hiding this comment.
What is the motivation for exposing it as as a gnu attribute? afaik GCC does not have this behavior
| return_bothattributes1(); // no warning because candiscard takes priority | ||
| return_bothattributes2(); // no warning because candiscard takes priority |
There was a problem hiding this comment.
Why though? Have you considered warning in this case?
I this so, yes, this is sufficiently weird and novel that it needs an RFC - I am a bit surprised by the motivation, especially in light of MSVC successfully deploying expected with |
|
How was the naming decided here? Any reason it isn't made similar to maybe_unused, as in may_discard if we are going this way? Neither of these make great language sense to me though. Since we are applying it to the return value I'd say something like discardable would make the most sense as it describes the return value, like maybe_unused describes an argument for example. Was there an RFC for the name, and if not maybe it would be good to have one. |
The original PR contains the reasoning of the libcxx maintainers about why they won't greenlight nodiscard std::expected without an attribute with this function. Some more discussions were on the libcxx discord channel. |
|
On the naming, I chose I don't know what an RFC is. Is it needed? What if this is "solution in search of a problem", what is the path to fix libc++ |
|
Unfortunately my browser is unsupported so I cannot log in. Could you help create such a thread? |
|
Implementing an attribute as a clang extension requires an RFC. An attribute is part of the clang frontend so as such post under that category on discourse. |
Unfortunately my browser is unsupported. Can you help? @cor3ntin you said MSVC has already deployed |
10 participants
Fulfills the requirement that huixie90 and philnik777 stated in #139651 that Clang will have a way to disable [[nodiscard]] on a function by function basis.
This will allow us finally to resume fixing #130656 via #139651.