-
Notifications
You must be signed in to change notification settings - Fork 15k
[clang] Add the candiscard attribute to suppress nodiscard #154943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3646,6 +3646,13 @@ def Unavailable : InheritableAttr { | |
| let MeaningfulToClassTemplateDefinition = 1; | ||
| } | ||
|
|
||
| def CanDiscard : InheritableAttr { | ||
| let Spellings = [Clang<"candiscard">]; | ||
| let Subjects = SubjectList<[ObjCMethod, FunctionLike, TypedefName]>; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure about this subject list? 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same subject list as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Comment on lines
+2459
to
+2463
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the motivation for exposing it as as a gnu attribute? afaik GCC does not have this behavior |
||
|
|
||
| .. 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"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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>()) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug #68456 prevents these attributes from working on alias templates |
||
| 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}; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -425,21 +425,76 @@ 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() { | ||
| struct A { | ||
| using NoDInt [[clang::warn_unused_result]] = int; | ||
| struct [[nodiscard]] NoDClass {}; | ||
| template <class T> using NoDTemplate [[clang::warn_unused_result]] = T; | ||
| }; | ||
|
|
||
| template <class T> | ||
| struct B { | ||
| using NoDInt = typename T::NoDInt; | ||
| using NoDIntIgnored [[clang::candiscard]] = typename T::NoDInt; | ||
| using NoDClass = typename T::NoDClass; | ||
| using NoDClassIgnored [[clang::candiscard]] = typename T::NoDClass; | ||
| template <class U> using NoDInt2 = typename U::NoDInt; | ||
| template <class U> using NoDIntIgnored2 [[clang::candiscard]] = typename U::NoDInt; | ||
| template <class U> using NoDClass2 = typename U::NoDClass; | ||
| template <class U> using NoDClassIgnored2 [[clang::candiscard]] = typename U::NoDClass; | ||
| }; | ||
|
|
||
| A::NoDTemplate<int> return_nodtemplate(); | ||
| B<A>::NoDInt return_nodint(); | ||
| B<A>::NoDIntIgnored return_nodint_ignored(); | ||
| B<A>::NoDClass return_nodclass(); | ||
| B<A>::NoDClassIgnored return_nodclass_ignored(); | ||
| B<A>::NoDInt2<A> return_nodint2(); | ||
| B<A>::NoDIntIgnored2<A> return_nodint_ignored2(); | ||
| B<A>::NoDClass2<A> return_nodclass2(); | ||
| B<A>::NoDClassIgnored2<A> return_nodclass_ignored2(); | ||
|
|
||
| NoDiscard (A::*mp_return_nodiscard)(); | ||
| WarnUnused (A::*mp_return_warnunused)(); | ||
| WarnUnusedResult (A::*mp_return_warnunusedresult)(); | ||
| NoDIgnored (A::*mp_return_nodiscard_ignored)(); | ||
| WUIgnored (A::*mp_return_warnunused_ignored)(); | ||
| WURIgnored (A::*mp_return_warnunusedresult_ignored)(); | ||
| [[clang::candiscard]] NoDiscard (A::*mp_return_nodiscard_ignored2)(); | ||
| [[clang::candiscard]] WarnUnused (A::*mp_return_warnunused_ignored2)(); | ||
| [[clang::candiscard]] WarnUnusedResult (A::*mp_return_warnunusedresult_ignored2)(); | ||
|
|
||
| void test(A *pa) { | ||
| // Unused but named variables | ||
| NoDiscard unused_variable1(1); // no warning | ||
| NoDiscard unused_variable2(""); // no warning | ||
|
|
@@ -471,14 +526,37 @@ void test() { | |
| static_cast<WarnUnusedResult>(""); // expected-warning {{ignoring temporary of type 'WarnUnusedResult' declared with 'gnu::warn_unused_result' attribute}} | ||
|
|
||
| // Function return values | ||
| 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(); // 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(); // 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 | ||
|
|
||
| // Member function pointer return values | ||
| (pa->*mp_return_nodiscard)(); // expected-warning {{ignoring return value of type 'NoDiscard' declared with 'nodiscard' attribute}} | ||
| (pa->*mp_return_warnunused)(); // no warning | ||
| (pa->*mp_return_warnunusedresult)(); // expected-warning {{ignoring return value of type 'WarnUnusedResult' declared with 'gnu::warn_unused_result' attribute}} | ||
| (pa->*mp_return_nodiscard_ignored)(); // no warning | ||
| (pa->*mp_return_warnunused_ignored)(); // no warning | ||
| (pa->*mp_return_warnunusedresult_ignored)(); // no warning | ||
| (pa->*mp_return_nodiscard_ignored2)(); // no warning | ||
| (pa->*mp_return_warnunused_ignored2)(); // no warning | ||
| (pa->*mp_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 +567,44 @@ 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 | ||
|
|
||
| return_nodint(); // expected-warning {{ignoring return value of type 'NoDInt' declared with 'clang::warn_unused_result' attribute}} | ||
| return_nodint_ignored(); // no warning | ||
| return_nodclass(); // expected-warning {{ignoring return value of type 'NoDClass' declared with 'nodiscard' attribute}} | ||
| return_nodclass_ignored(); // no warning | ||
|
|
||
| // In an alias template the information about the typedef is lost, | ||
| // so the diagnostic is not issued. GH#68456 | ||
| return_nodtemplate(); | ||
|
|
||
| // In an alias template the information about the typedef is lost, | ||
| // so the diagnostic is not suppressed. GH#68456 | ||
| return_nodint2(); // expected-warning {{ignoring return value of type 'NoDInt' declared with 'clang::warn_unused_result' attribute}} | ||
| return_nodint_ignored2(); // expected-warning {{ignoring return value of type 'NoDInt' declared with 'clang::warn_unused_result' attribute}} | ||
| return_nodclass2(); // expected-warning {{ignoring return value of type 'NoDClass' declared with 'nodiscard' attribute}} | ||
| return_nodclass_ignored2(); // expected-warning {{ignoring return value of type 'NoDClass' declared with 'nodiscard' attribute}} | ||
| } | ||
|
|
||
| using BothAttributes [[clang::warn_unused_result, clang::candiscard]] = int; | ||
|
|
||
| BothAttributes return_bothattributes1(); | ||
| [[nodiscard, clang::candiscard]] int return_bothattributes2(); | ||
| [[nodiscard]] NoDIgnored return_nodignored_nodiscard(); | ||
|
|
||
| void testBothAttributes() { | ||
| return_bothattributes1(); // no warning because candiscard takes priority | ||
| return_bothattributes2(); // no warning because candiscard takes priority | ||
|
Comment on lines
+605
to
+606
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why though? Have you considered warning in this case? |
||
| return_nodignored_nodiscard(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}} | ||
| } | ||
|
|
||
| } // namespace candiscard | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of a shorter name
[[clang::discard]]? I have no preference for this。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO
discardablewould be a good name, but I'm not super attached to any of the names so far.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discardablelooks good to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not aware of any discussion but perhaps I just missed it;
discardablesgtm though.