[clang-tidy] Expand the list of functions in bugprone-unused-return-value

[jranieri-grammatech]

Summary

This change adds common C, C++, and POSIX functions to the clang-tidy unused return value checker.

We ran our commercial static analysis tool, CodeSonar, over the source code of more than 6000 projects from Fedora's package manager. During this run, we gathered information about the return value usages of various library functions. To create a list of library functions that should always have their return value checked, we required at least 20 projects to have used the library function and for over 95% of those uses to have checked the return value. Finally, we limited the list of functions to those from the C standard library, the C++ standard library, and the POSIX specification, as these are the most likely to be of interest to clang-tidy users.

No test cases have been added as there is already a test that ensures functions on this list trigger correctly and a second test that ensures the functions on this list do not trigger if a custom list is specified.

[alexfh]

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

57

This will also affect "the other std::move" (https://en.cppreference.com/w/cpp/algorithm/move).

98

bind has a side effect and returns a success status. Thus, the result being unused isn't necessarily a bug. Same for connect. And probably for setjmp as well.

[njames93]

Please upload diffs with full context. If you are using git diff to generate the patch, pass the flag -U999999

[jranieri-grammatech]

Adding context.

[jranieri-grammatech]

Removing std::move.

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

57

The ambiguity here is unfortunate, but I'll remove it from the list.

98

In terms of bind, connect, and setjmp: while I personally would say that code not using the return value is bugprone, the data suggests that the vast majority of developers are using these functions in the intended manner and the false-positive rate should be low.

[aaron.ballman]

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

98

I think we have sufficient statistical data to suggest that these APIs should be on the list because the majority of programmers *do not* use them solely for side effects without using the return value, so my preference is to keep them in the list.

[sammccall]

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

98

I stumbled upon this review as we're considering turning this check on by default in clangd.

There's a significant difference between unused std::async() (programmer misunderstood contract) and unused ::connect() (ignoring error conditions). The former is ~never noise, and the latter may be (e.g. in experimental or incomplete code).

So there's some value in separating these two lists out either as an option or a separate named check (bugprone-unhandled-error?) I think we probably wouldn't enable this check by default if it includes the error-code functions.

the majority of programmers *do not* use them solely for side effects

...in popular, distributed software :-)

[aaron.ballman]

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

98

So there's some value in separating these two lists out either as an option or a separate named check (bugprone-unhandled-error?) I think we probably wouldn't enable this check by default if it includes the error-code functions.

I think that adds complexity to this check when the complexity isn't necessary. clang-tidy has traditionally been a place for checks that are chattier than what the compiler should provide, and this check has a trivial, well-understood mechanism to silence the diagnostics (cast to void) which also expresses intent properly to the toolchain.

the majority of programmers *do not* use them solely for side effects

...in popular, distributed software :-)

I have not seen anyone provide data to suggest that the functions in question appear in any statistically significant amount in practice without checking the return value, just worries that they *could*. I don't think that's compelling in the face of data. Remember, this is for bugprone patterns, not bugknown patterns.

[sammccall]

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

98

I think we're talking past each other here. I'm not saying clang-tidy shouldn't have the check, or that it's not a bugprone pattern, or that the clang-tidy default should be different.

But there are scenarios where you want one but not the other. Concretely, warnings shown in an IDE as you type and by default. If you're misusing an API rendering it completely useless, you should see that ASAP. If you fail to check an error code, some users won't want to be warned about that until later.

By bundling them into a single check without options (other than duplicating the whole list), it's hard to create that useful but inoffensive default setup. That's OK, clangd can remove the check from the whitelist, but I think we'd get a lot of value out of it.

I have not seen anyone provide data to suggest that the functions in question appear in any statistically significant amount in practice

Right, we don't have data either way on incomplete code. Based on experience of writing code and watching others write code, I believe people write sloppy code they'd never check in, and appreciate being told early when it doesn't do what they intend, but some don't appreciate being told to be less sloppy. Is your intuition different? Do you think the data provided addresses this question?

[aaron.ballman]

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

98

But there are scenarios where you want one but not the other. Concretely, warnings shown in an IDE as you type and by default. If you're misusing an API rendering it completely useless, you should see that ASAP. If you fail to check an error code, some users won't want to be warned about that until later.

You're right, we were talking past one another because this was not a scenario I had in mind at all. Thank you for raising it!

Is your intuition different? Do you think the data provided addresses this question?

I will have to spend some time figuring out what my intuition is for checks that are run as you are typing the code, but my wildly unconsidered opinion is that there's not a big difference between as-you-type and not for these checks. Either it's a bad idea to ignore the return value or it's not a bad idea -- whether you're still typing or not does seem to change the answer. However, as I said, this is an unconsidered opinion. :-)

However, the data we've found *definitely* does not address the question.

One thing this check does not do is add a fix-it hint to automatically cast to void. If such a fixit were added, would that help the as-you-type scenario by providing a way to quickly insert the way to silence the diagnostic for those who really do want to ignore the return value?

[aaron.ballman]

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

98

I thought about this more over the weekend and my thinking is that these really are APIs where we believe ignoring the return value is almost always a correctness issue regardless of whether it happens in test code, or while the user is typing in their editor, experimental code, etc. If ignoring the return value is desired behavior for one of these functions, it would most likely be an outlier where the code could stand to be made more clear via casting the return to void (and probably some comments explaining why it's still correct to ignore the return value). We could have separate lists for these identifiers, as suggested, but it's not clear to me what metric would be used to decide which list to put any given identifier on. So, to me, this is about designing the feature for the 95% case or the 5% case, and I think maintaining two separate lists would add a lot of overhead for little value. I think there is value to telling the programmer "you're ignoring this return value and 'most' people don't do that" as quickly as possible as it also may catch a bug the developer wasn't thinking about yet.

[sammccall]

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

98

I think this is pretty simple, it's the distinction between "almost always" vs "always", and "a correctness issue" vs "wrong".

Calling bind() or whatever without checking the error code is sometimes useful, if only for prototyping, and calling back_inserter() never is.

But we're going around circles on this and it's blocking useful work. I'll send a separate patch to split the lists or remove the few bad entries or something.

[aaron.ballman]

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

98

I think this is pretty simple, it's the distinction between "almost always" vs "always", and "a correctness issue" vs "wrong".

I do not see a distinction between "correctness issue" and "wrong" -- they're the same, to me.

Calling bind() or whatever without checking the error code is sometimes useful, if only for prototyping, and calling back_inserter() never is.

Have you run this patch over some code bases and are seeing this sort of thing showing up in test code where it's onerous for the user to type (void) for some reason? Because the only situations I can think of where ignoring these return types is valid is while you are typing the code or for throw-away code, but I do not think checks should be constrained by either of those use cases.

But we're going around circles on this and it's blocking useful work. I'll send a separate patch to split the lists or remove the few bad entries or something.

That could also work. If we don't currently have evidence that this will be chatty, we could land the patch as-is and see how well it bakes for a while and then decide if we need changes based on feedback.

[jranieri-grammatech]

Ping? It sounds like the consensus is to commit this as-is and, if there's a negative fallout for users of clang-tidy, either split out the functions or pare the list down later?

[aaron.ballman]

In D76083#2026375, @jranieri-grammatech wrote:

Ping? It sounds like the consensus is to commit this as-is and, if there's a negative fallout for users of clang-tidy, either split out the functions or pare the list down later?

That is my understanding of the consensus position.

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

: ClangTidyCheck(Name, Context),
       CheckedFunctions(Options.get("CheckedFunctions",
                                    "::std::async;"
                                    "::std::launder;"
                                    "::std::remove;"
                                    "::std::remove_if;"
                                    "::std::unique;"
                                    "::std::unique_ptr::release;"
                                    "::std::basic_string::empty;"
-                                   "::std::vector::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::move;"
+                                   "::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")) {}
 
 void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckedFunctions", CheckedFunctions);
 }
 
 void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
   auto FunVec = utils::options::parseStringList(CheckedFunctions);
   auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts(