[clang-tidy] Add options to bugprone-unused-return-value.

[balazske]

Summary

Extend the check with option to include or exclude specific functions.
This makes it more easy to add extra functions or remove from the checked function set
without specifying all functions.

[balazske]

These options are added to make the configuration more comfortable if only functions are to be added or removed from the list. I plan another commit that extends the list of checked functions significantly (to include all or some of the functions listed in CERT ERR33-C) (but after looking at D76083 I am not sure).

[aaron.ballman]

I think this is missing changes to the documentation as well.

In D111228#3045319, @balazske wrote:

These options are added to make the configuration more comfortable if only functions are to be added or removed from the list. I plan another commit that extends the list of checked functions significantly (to include all or some of the functions listed in CERT ERR33-C) (but after looking at D76083 I am not sure).

Personally, I think that would be very useful. One possible approach if there's concerns over adding that list in bugprone would be to add an alias to this check named cert-err33-c and have its default configuration include the listed functions.

clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp

33

Any reason not to use StringRef here given that it's a constant list?

[balazske]

Using StringRef, added documentation (untested).

[aaron.ballman]

Thank you for the new documentation, that helps! The one thing I'm still struggling with is the design approach and how well this will scale. Basically, the issue here is that CheckedFunctions already has a very long list of options, and you want to be able to manage that list (add entries to it or remove entries from it) without having to re-specify the entire list, correct? My concerns are: 0) It's a confusing design because the complete list is now spread over three different options that you have to think about, instead of just "here's the list of entries", 1) this problem can appear in *any* config list that gets large (for example, when you have a config file in a parent directory and a child directory and the parent adds a bunch of items to a list that the child wants to manage).

I wonder if we should be considering a more general approach like adding some set manipulation functionality to the config list itself. Thinking out loud (haven't put a ton of thought into this), we could devise some sort of syntax like:

{key: bugprone-unused-return-value.IncludedFunctions, value: "$add$($remove$(bugprone-unused-return-value.IncludedFunctions, "::excludedFun"), "::includedFun,::otherFun"},

That would remove ::excludedFun from the default list and add ::includedFun and ::otherFun to the list. Then this functionality could be used in any config setting that involves lists of elements rather than adding additional config options to each check. WDYT?

[balazske]

Probably it can work to only have a "add" or "remove" possibility in the config string, and not using an option name like in the provided example. Otherwise it can be possible to mix string lists from different checks, that seems to be a difficult problem. Other solution is to have a special list (or set) configuration format.

[aaron.ballman]

In D111228#3051686, @balazske wrote:

Probably it can work to only have a "add" or "remove" possibility in the config string, and not using an option name like in the provided example. Otherwise it can be possible to mix string lists from different checks, that seems to be a difficult problem. Other solution is to have a special list (or set) configuration format.

Hmm, I was thinking that was more of a feature than a problem. We have some checks that already have parallel lists of somewhat related strings, so being able to say "define this list in terms of that list plus/minus these elements" seems useful. Or are you saying there's an implementation difficulty with supporting that which might make it less of a good approach?

[balazske]

I think a configuration option that depends on configuration of another check causes too many problems. It causes dependencies between checks, possible circular dependency (that is an error). What if the other check is not enabled (if a check configuration depends on other it may mean that these two belong together and should both be enabled, or not)? It becomes less clear from where a value comes.

A probably better way: Define configuration values like "variables" that can be used at any check, probably with manipulation of the original value. But not reuse configuration of another check.

[aaron.ballman]

In D111228#3054263, @balazske wrote:

I think a configuration option that depends on configuration of another check causes too many problems. It causes dependencies between checks, possible circular dependency (that is an error). What if the other check is not enabled (if a check configuration depends on other it may mean that these two belong together and should both be enabled, or not)? It becomes less clear from where a value comes.

Okay, that make sense to me, thanks!

A probably better way: Define configuration values like "variables" that can be used at any check, probably with manipulation of the original value. But not reuse configuration of another check.

That might make more sense. Are we agreed that it would be better to solve the larger issue instead of just for this check?

[balazske]

Checker configuration can be solved in reusable way by introducing "configuration variables" and manipulation of these with set operations.

https://reviews.llvm.org/D111623 was created to update the checker documentation.

clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.h

 public:
   UnusedReturnValueCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   std::string CheckedFunctions;
+  std::string IncludedFunctions;
+  std::string ExcludedFunctions;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDRETURNVALUECHECK_H

clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp

 // member function are matched directly with InnerMatcher.
 AST_MATCHER_P(FunctionDecl, isInstantiatedFrom, Matcher<FunctionDecl>,
               InnerMatcher) {
   FunctionDecl *InstantiatedFrom = Node.getInstantiatedFromMemberFunction();
   return InnerMatcher.matches(InstantiatedFrom ? *InstantiatedFrom : Node,
                               Finder, Builder);
 }
 
-} // namespace
-
-UnusedReturnValueCheck::UnusedReturnValueCheck(llvm::StringRef Name,
-                                               ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      CheckedFunctions(Options.get("CheckedFunctions",
-                                   "::std::async;"
+const std::string DefaultCheckedFunctions = "::std::async;"
                                             "::std::launder;"
                                             "::std::remove;"
                                             "::std::remove_if;"
                                             "::std::unique;"
                                             "::std::unique_ptr::release;"
                                             "::std::basic_string::empty;"
                                             "::std::vector::empty;"
                                             "::std::back_inserter;"
                                             "::std::distance;"
                                             "::std::find;"
                                             "::std::find_if;"
                                             "::std::inserter;"
                                             "::std::lower_bound;"
                                             "::std::make_pair;"
                                             "::std::map::count;"
                                             "::std::map::find;"
                                             "::std::map::lower_bound;"
                                             "::std::multimap::equal_range;"
                                             "::std::multimap::upper_bound;"
                                             "::std::set::count;"
                                             "::std::set::find;"
                                             "::std::setfill;"
                                             "::std::setprecision;"
                                             "::std::setw;"
                                             "::std::upper_bound;"
                                             "::std::vector::at;"
                                             // C standard library
                                             "::bsearch;"
                                             "::ferror;"
                                             "::feof;"
                                             "::isalnum;"
                                             "::isalpha;"
                                             "::isblank;"
                                             "::iscntrl;"
                                             "::isdigit;"
                                             "::isgraph;"
                                             "::islower;"
                                             "::isprint;"
                                             "::ispunct;"
                                             "::isspace;"
                                             "::isupper;"
                                             "::iswalnum;"
                                             "::iswprint;"
                                             "::iswspace;"
                                             "::isxdigit;"
                                             "::memchr;"
                                             "::memcmp;"
                                             "::strcmp;"
                                             "::strcoll;"
                                             "::strncmp;"
                                             "::strpbrk;"
                                             "::strrchr;"
                                             "::strspn;"
                                             "::strstr;"
                                             "::wcscmp;"
                                             // POSIX
                                             "::access;"
                                             "::bind;"
                                             "::connect;"
                                             "::difftime;"
                                             "::dlsym;"
                                             "::fnmatch;"
                                             "::getaddrinfo;"
                                             "::getopt;"
                                             "::htonl;"
                                             "::htons;"
                                             "::iconv_open;"
                                             "::inet_addr;"
                                             "::isascii;"
                                             "::isatty;"
                                             "::mmap;"
                                             "::newlocale;"
                                             "::openat;"
                                             "::pathconf;"
                                             "::pthread_equal;"
                                             "::pthread_getspecific;"
                                             "::pthread_mutex_trylock;"
                                             "::readdir;"
                                             "::readlink;"
                                             "::recvmsg;"
                                             "::regexec;"
                                             "::scandir;"
                                             "::semget;"
                                             "::setjmp;"
                                             "::shm_open;"
                                             "::shmget;"
                                             "::sigismember;"
                                             "::strcasecmp;"
                                             "::strsignal;"
-                                   "::ttyname")) {}
+                                            "::ttyname";
+
+} // namespace
+
+UnusedReturnValueCheck::UnusedReturnValueCheck(llvm::StringRef Name,
+                                               ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      CheckedFunctions(
+          Options.get("CheckedFunctions", DefaultCheckedFunctions)),
+      IncludedFunctions(Options.get("IncludedFunctions", "")),
+      ExcludedFunctions(Options.get("ExcludedFunctions", "")) {}
 
 void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckedFunctions", CheckedFunctions);
+  Options.store(Opts, "IncludedFunctions", IncludedFunctions);
+  Options.store(Opts, "ExcludedFunctions", ExcludedFunctions);
 }
 
 void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
   auto FunVec = utils::options::parseStringList(CheckedFunctions);
+  std::set<std::string> Functions(FunVec.begin(), FunVec.end());
+  FunVec = utils::options::parseStringList(IncludedFunctions);
+  Functions.insert(FunVec.begin(), FunVec.end());
+  FunVec = utils::options::parseStringList(ExcludedFunctions);
+  for (const std::string &Str : FunVec)
+    Functions.erase(Str);
+
   auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts(
       callExpr(callee(functionDecl(
                    // Don't match void overloads of checked functions.
                    unless(returns(voidType())),
-                   isInstantiatedFrom(hasAnyName(
-                       std::vector<StringRef>(FunVec.begin(), FunVec.end()))))))
+                   isInstantiatedFrom(hasAnyName(std::vector<StringRef>(
+                       Functions.begin(), Functions.end()))))))
           .bind("match"))));
 
   auto UnusedInCompoundStmt =
       compoundStmt(forEach(MatchedCallExpr),
                    // The checker can't currently differentiate between the
                    // return statement and other statements inside GNU statement
                    // expressions, so disable the checker inside them to avoid
                    // false positives.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-return-value-custom.cpp

 // RUN: %check_clang_tidy %s bugprone-unused-return-value %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: bugprone-unused-return-value.CheckedFunctions, \
-// RUN:    value: "::fun;::ns::Outer::Inner::memFun;::ns::Type::staticFun;::ns::ClassTemplate::memFun;::ns::ClassTemplate::staticFun"}]}' \
+// RUN:    value: "::fun;::ns::Outer::Inner::memFun;::ns::Type::staticFun;::ns::ClassTemplate::memFun;::ns::ClassTemplate::staticFun;::excludedFun"}, \
+// RUN:  {key: bugprone-unused-return-value.IncludedFunctions, \
+// RUN:    value: "::includedFun;::excludedFun"}, \
+// RUN:  {key: bugprone-unused-return-value.ExcludedFunctions, \
+// RUN:    value: "::excludedFun"}]}' \
 // RUN: --
 
 namespace std {
 
 template <typename T>
 T *launder(T *);
 
 } // namespace std
struct ClassTemplate {
   Retval memFun();
   static Retval staticFun();
 };
 
 } // namespace ns
 
 int fun();
 void fun(int);
+int includedFun();
+int excludedFun();
 
 void warning() {
   fun();
   // CHECK-NOTES: [[@LINE-1]]:3: warning: the value returned by this function should be used
   // CHECK-NOTES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning
 
   (fun());
   // CHECK-NOTES: [[@LINE-1]]:4: warning: the value {{.*}} should be used
void warning() {
   ns::ClassTemplate<int> ObjA4;
   ObjA4.memFun();
   // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used
   // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning
 
   ns::ClassTemplate<int>::staticFun();
   // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used
   // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning
+
+  includedFun();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used
+  // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning
 }
 
 void noWarning() {
   auto R1 = fun();
 
   ns::Outer::Inner ObjB1;
   auto R2 = ObjB1.memFun();
 
   auto R3 = ns::Type::staticFun();
 
   ns::ClassTemplate<int> ObjB2;
   auto R4 = ObjB2.memFun();
 
   auto R5 = ns::ClassTemplate<int>::staticFun();
 
   // test calling a void overload of a checked function
   fun(5);
 
   // test discarding return value of functions that are not configured to be checked
   int I = 1;
   std::launder(&I);
 
   ns::Type ObjB3;
   ObjB3.memFun();
+
+  // no warning on function that is specified as excluded and as to be checked
+  excludedFun();
 }