Page MenuHomePhabricator

[clang-tidy] New check bugprone-unused-return-value
ClosedPublic

Authored by khuttun on Jan 1 2018, 4:43 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

khuttun created this revision.Jan 1 2018, 4:43 AM
Eugene.Zelenko added inline comments.
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
13 ↗(On Diff #128386)

Please include cassert, Regex.h, raw_ostream.h, SmallString.h.

clang-tidy/bugprone/UnusedReturnValueCheck.h
14 ↗(On Diff #128386)

Please include string.

docs/clang-tidy/checks/bugprone-unused-return-value.rst
17 ↗(On Diff #128386)

Please add round brackets to all functions/methods here and below.

I think it would be more user friendly if the configured list can be a list and the | concatenation is done within your code.

clang-tidy/bugprone/UnusedReturnValueCheck.cpp
29 ↗(On Diff #128386)

Don't use auto here, because the type is not written somewhere

53 ↗(On Diff #128386)

Missing full stop.

aaron.ballman added inline comments.Jan 1 2018, 10:23 AM
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
62–63 ↗(On Diff #128386)

const auto Matched -> const auto *Matched

You can elide the curly braces.

The diagnostic doesn't really help the user understand what's wrong with the code. How about "the value returned by this function should be used" and add a note diagnostic that says "cast expression to void to silence warning". Also, you should pass in the SourceRange for the function call expression to the call to diag() so that it highlights the call in question.

test/clang-tidy/bugprone-unused-return-value.cpp
200 ↗(On Diff #128386)

Can you also add a test for ignoring the result of a call to std::empty(SomeContainerWithAnEmptyMethod)?

khuttun marked 7 inline comments as done.Jan 2 2018, 11:04 AM

I think it would be more user friendly if the configured list can be a list and the | concatenation is done within your code.

What do you exactly mean by list? It seems some other checks use comma or semicolon separators in their configuration strings, but is that really more user friendly than using "|"? At least now it's clear that the whole string is a regex.

khuttun updated this revision to Diff 128544.Jan 3 2018, 11:06 AM

Fix review comments

aaron.ballman added inline comments.Jan 4 2018, 10:47 AM
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
68 ↗(On Diff #128544)

"Normally" is probably a bad term to use here. How about "the value returned by %0 is usually not intended to be discarded"?

69 ↗(On Diff #128544)

In the event this returns null, the diagnostic is going to look rather odd. Because the value of the call expression is unused, this will most often trigger in a context where the method call can be inferred (especially because you're now highlighting the source range). It might make sense to simply replace the %0 with "this call expression" or somesuch in the diagnostic.

khuttun marked an inline comment as done.Jan 6 2018, 2:16 AM
khuttun added inline comments.
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
69 ↗(On Diff #128544)

I can remove the function name from the diagnostic. Out of curiosity, in which situations could it be null?

khuttun updated this revision to Diff 128844.Jan 6 2018, 2:31 AM

Fix review comments

aaron.ballman added inline comments.Jan 6 2018, 8:43 AM
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
69 ↗(On Diff #128544)

Situations where the call doesn't refer back to a declaration at all. For instance, a lambda or block.

docs/clang-tidy/checks/bugprone-unused-return-value.rst
22 ↗(On Diff #128844)

resource leaks, if the -> resource leaks if the
Often -> Often,

31 ↗(On Diff #128844)

I'd reword this sentence to: Not using the return value often indicates that the programmer confused the function with clear().

test/clang-tidy/bugprone-unused-return-value.cpp
163 ↗(On Diff #128844)

Sorry, I just realized that we're missing a test case for a common situation -- where the result is used as part of another call expression. Can you add a test to noWarning() to make sure this does not warn:

std::vector<int> v;
extern void f(bool);

f(v.empty()); // Should not warn
khuttun marked 2 inline comments as done.Jan 7 2018, 10:12 AM
khuttun added inline comments.
test/clang-tidy/bugprone-unused-return-value.cpp
163 ↗(On Diff #128844)

See line 199 in this file.

khuttun updated this revision to Diff 128879.Jan 7 2018, 10:24 AM

Fix review comments

aaron.ballman added inline comments.Jan 8 2018, 5:48 AM
test/clang-tidy/bugprone-unused-return-value.cpp
163 ↗(On Diff #128844)

Ah, my eyes missed that, thank you!

Hmm, I *think* this test should also be okay, but I want to be sure:

std::vector<int> v;
bool b = ({v.empty()}); // Should not warn
({v.empty()}); // ???

I kind of thing that, as an extension to the spirit of this check, any GNU expression statement whose result is unused should probably be diagnosed; what do you think?

You should add tests for the above so we document the expected behavior here.

khuttun added inline comments.Jan 8 2018, 8:50 AM
test/clang-tidy/bugprone-unused-return-value.cpp
163 ↗(On Diff #128844)

Getting a warning when just surrounding the call expression with parenthesis is tested in bugprone-unused-return-value-custom.cpp.

Would your example be parsed as creating an initializer_list? In that case it should not warn. I can add test cases for that.

What do you mean by "GNU expression"?

aaron.ballman added inline comments.Jan 8 2018, 9:14 AM
test/clang-tidy/bugprone-unused-return-value.cpp
163 ↗(On Diff #128844)

Getting a warning when just surrounding the call expression with parenthesis is tested in bugprone-unused-return-value-custom.cpp.

Yup, this is something entirely different, however.

Would your example be parsed as creating an initializer_list? In that case it should not warn. I can add test cases for that.

What do you mean by "GNU expression"?

Those examples are GNU expression statements, which is a GCC extension that Clang also supports. See https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html for more information.

Effectively, the last statement in the GNU expression statement is the "return value" of the expression statement. We should make sure the warnings are sensible with this construct.

khuttun added inline comments.Jan 8 2018, 10:56 AM
test/clang-tidy/bugprone-unused-return-value.cpp
163 ↗(On Diff #128844)

This was a good catch. Code like this

auto Gnu = ({ fun(); });

is currently causing a warning.

khuttun updated this revision to Diff 129090.Jan 9 2018, 8:14 AM

The checker is now disabled inside GNU statement expressions

aaron.ballman added inline comments.Jan 11 2018, 10:56 AM
test/clang-tidy/bugprone-unused-return-value.cpp
210 ↗(On Diff #129090)

Not diagnosing this case is questionable -- the return value *is* unused and that's a bad thing. However, this is likely to be an uncommon code pattern, so I'd be fine if you added a FIXME to the AST matcher code that excludes GNU expression statements to handle this case someday, and add a FIXME comment here as well so we know what we would like the behavior to be (you could fix the FIXMEs in a follow-up patch if you'd like).

High Integrity C++ has the rule 17.5.1 Do not ignore the result of std::remove, std::remove_if or std::unique. Do we want to add those to the preconfigured list?

JonasToth added inline comments.Jan 12 2018, 1:13 PM
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
47 ↗(On Diff #129090)

Is the following type a problem for you check?

std::unique_ptr<std::vector<int>> should not be matchable with regex but I don't know if that would have an impact on the functionality.

High Integrity C++ has the rule 17.5.1 Do not ignore the result of std::remove, std::remove_if or std::unique. Do we want to add those to the preconfigured list?

To me these sound like reasonable additions. I can add them and run the checker against LLVM source to see if we get any false positives with them.

I also noticed that the checker currently misses unused values inside other kinds of statements than compound statement (if statements, case statements etc.). I will also update the checker to handle these.

clang-tidy/bugprone/UnusedReturnValueCheck.cpp
47 ↗(On Diff #129090)

std::unique_ptr<std::vector<int>> is also matched. I added a test case for it.

test/clang-tidy/bugprone-unused-return-value.cpp
210 ↗(On Diff #129090)

I'm not so sure whether this code should cause a warning. I see it as equivalent to this

[]{ return std::async(increment, 42); }();

, where the return value of std::async is used in the return statement.

One situation related to the GNU statement expressions that the checker currently misses is if the return value is unused inside the statement expression at some other than the last statement. I can see if this could be somehow covered.

aaron.ballman added inline comments.Jan 13 2018, 8:03 AM
test/clang-tidy/bugprone-unused-return-value.cpp
210 ↗(On Diff #129090)

It all depends on how far down the rabbit hole you want the check to go, I guess.

auto fn = []{ return std::async(increment, 42); };
fn(); // Could also be reasonable to diagnose

I'm fine leaving it alone, but I'd like to see test coverage and comments showing the cases were explicitly considered.

JonasToth added inline comments.Jan 15 2018, 9:07 AM
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
47 ↗(On Diff #129090)

Alright. I think i had a error in my thinking. Because there are followup letters in the regex there are no problems. I thought it would end on the first >, but thats no the case! Thank you for clarification :)

khuttun updated this revision to Diff 130461.Jan 18 2018, 11:29 AM
  • Detect unused return values also inside other kinds of statements than compound statements
  • Ignore void functions in the checker
  • Check std::remove, std::remove_if and std::unique by default

The checker reports 7 warnings on LLVM + Clang code bases, all on std::unique_ptr::release:

lib/Bitcode/Reader/BitReader.cpp:114:3

  • release() called on moved-from unique_ptr
  • no harm, just unnecessary

lib/ExecutionEngine/ExecutionEngine.cpp:149:7

  • release() called before erasing unique_ptr from a container
  • false positive?

lib/Target/AMDGPU/GCNIterativeScheduler.cpp:196:5

  • release() called before assigning new pointer to unique_ptr
  • false positive?

lib/AsmParser/LLParser.cpp:855:3

  • get() + release() could be replaced with release()

tools/clang/lib/Lex/ModuleMap.cpp:791:3

  • false positive?

tools/clang/tools/extra/clangd/Compiler.cpp:61:3

  • get() + release() could potentially be replaced with release()?

unittests/Support/Casting.cpp:144:3

  • release() called to avoid calling delete on a pointer to global object
  • false positive?

Any more comments on this?

alexfh added inline comments.Feb 2 2018, 5:16 AM
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
36 ↗(On Diff #130461)

Can we avoid creating the regex on each match? For example, by changing the parameter type to llvm::Regex. If we need the matcher at all - see the comment below.

45–48 ↗(On Diff #130461)

I strongly suspect that we could get away without regexs here. hasAnyName supports inline namespaces, so at least the first five entries here are covered. The main problem with std::unique_ptr<.*>::release is the need to match any template parameters. I *think*, we could adjust hasName to allow omitting template parameters (so that std::unique_ptr::release would match specializations of unique_ptr as well). The empty and allocate would need some research. Can we just list all specific classes where we care about empty? (Alternatively, can we match empty unqualified and check that it's somewhere inside std, but I don't like that much, since it would add inconsistency in how this check is configured.)

61 ↗(On Diff #130461)

This is a rather expensive matcher and it is going to be run on each callExpr, which is a lot. Let's at least swap it with the unless(returns(voidType())) below to reduce the number of times it's called.

alexfh added inline comments.Feb 2 2018, 5:22 AM
clang-tidy/bugprone/UnusedReturnValueCheck.cpp
45–48 ↗(On Diff #130461)

A clarification: we could go with your current version and optimize this part later. But the option may start being used by someone and then will change - that would be nice to avoid.

Alternatively, we could switch to hasAnyName right away and leave only the functions that can be easily supported, and then figure out what to do with release, allocate and empty.

I'd probably prefer the latter.

The checker reports 7 warnings on LLVM + Clang code bases, all on std::unique_ptr::release:

lib/Bitcode/Reader/BitReader.cpp:114:3

  • release() called on moved-from unique_ptr
  • no harm, just unnecessary

Agreed.

lib/ExecutionEngine/ExecutionEngine.cpp:149:7

  • release() called before erasing unique_ptr from a container
  • false positive?

False positive.

lib/Target/AMDGPU/GCNIterativeScheduler.cpp:196:5

  • release() called before assigning new pointer to unique_ptr
  • false positive?

False positive, but a bad code smell given that the assignment operator will perform the release.

lib/AsmParser/LLParser.cpp:855:3

  • get() + release() could be replaced with release()

    tools/clang/lib/Lex/ModuleMap.cpp:791:3
  • false positive?

False positive.

tools/clang/tools/extra/clangd/Compiler.cpp:61:3

  • get() + release() could potentially be replaced with release()?

False positive.

unittests/Support/Casting.cpp:144:3

  • release() called to avoid calling delete on a pointer to global object
  • false positive?

False positive.

From what I can tell of these reports, they almost all boil down to ignoring the call to release() because the pointer is no longer owned by the unique_ptr. This is a pretty reasonable code pattern, but it also seems reasonable to expect users to cast the result to void to silence the warning, so I think this is fine. Have you checked any other large C++ code bases, like Qt or boost?

From what I can tell of these reports, they almost all boil down to ignoring the call to release() because the pointer is no longer owned by the unique_ptr. This is a pretty reasonable code pattern, but it also seems reasonable to expect users to cast the result to void to silence the warning, so I think this is fine. Have you checked any other large C++ code bases, like Qt or boost?

Yep, there's already code also in LLVM where the release() return value is cast to void, for example in lib/Bitcode/Reader/BitReader.cpp. I haven't run the checker on other large code bases yet, but I can test also that.

clang-tidy/bugprone/UnusedReturnValueCheck.cpp
45–48 ↗(On Diff #130461)

If the ultimate goal would be to have this checker working without regexes, then I agree that we shouldn't introduce a version that uses them in the config string, as changing that later would break things.

About creating a version of hasName that ignores template arguments: as I understand it we'd need a new PrintingPolicy to suppress printing the template argument list in NamedDecl::printQualifiedName. Is this correct?

WG21 P0600R1 lists 8 allocate() and 24 empty() functions in std that should be marked with [[nodiscard]]. We could list all of them separately in the checker config, but the config string would get quite long. Regex matching handles these nicely, but if the performance is unacceptable, we have to look for other ways or just skip checking these.

khuttun updated this revision to Diff 134802.Feb 17 2018, 7:45 AM

I changed the checker to use hasAnyName. Checking for unique_ptr::release() and all the empty() functions is removed.

The checker doesn't report any warnings from LLVM + clang codebases now.

khuttun marked 3 inline comments as done.Feb 17 2018, 7:58 AM

Should I close this review?

aaron.ballman accepted this revision.Mar 12 2018, 1:10 PM

I think this check LGTM.

This revision is now accepted and ready to land.Mar 12 2018, 1:10 PM
alexfh accepted this revision.Mar 14 2018, 9:05 AM

LG. Thank you!

BTW, there's an old related patch https://reviews.llvm.org/D17043, which mostly focuses on member functions of STL containers. It might be useful as a reference (there's a pretty extensive list of member functions and containers).

Do you need help committing the patch?

Do you need help committing the patch?

Yes please, I don't have commit access to the repo.

I think the next step for improving this checker could be to make it work with class template member functions. That would allow to add checking also to those container functions that D17043 handles.

Do you need help committing the patch?

Yes please, I don't have commit access to the repo.

The patch doesn't apply cleanly. Please rebase against current HEAD.

I think the next step for improving this checker could be to make it work with class template member functions. That would allow to add checking also to those container functions that D17043 handles.

Yep, sounds reasonable. Thank you for working on this!

This revision was automatically updated to reflect the committed changes.