This is an archive of the discontinued LLVM Phabricator instance.

Check that the result of a library call w/o side effects is used
Needs RevisionPublic

Authored by sidney on Feb 9 2016, 2:23 PM.

Details

Summary

Currently has a whitelist of methods like std::vector::empty() which,
in a standalone statement, likely won't do what the programmer wants.

The whitelist is brittle (Did I misspell a method? Does a container
other than std::vector have a base class like std::__vector_base?) and
unlikely to be maintained, so two directions for improvement could be:

  1. Use the compiler to determine whether a method has side effects.
  1. Annotate methods without side effects. A newly-added method is more

likely to end up with the annotation than in this check's whitelist.

...but this might be a good start.

Project from https://trello.com/c/4sbyrOSv/15-clang-checker-check-that-the-result-of-library-calls-w-o-side-effects-are-used

Diff Detail

Event Timeline

sidney updated this revision to Diff 47367.Feb 9 2016, 2:23 PM
sidney retitled this revision from to Check that the result of a library call w/o side effects is used.
sidney updated this object.
sidney added reviewers: mclow.lists, alexfh.
sidney added a subscriber: cfe-commits.
sidney added a comment.Feb 9 2016, 2:36 PM

Quick note: this is my first contribution, born from the Clang/LLVM sprint last weekend. Marshall helped me get started.

alexfh edited edge metadata.Feb 10 2016, 8:21 AM

Thank you for working on this check!

I'd like to note that there is a library-side way to mitigate this issue using the [[clang::warn_unused_result]] attribute on vector::empty() and similar methods:

$ cat q.cc
#if defined(__clang__)
# if __cplusplus >= 201103L && __has_feature(cxx_attributes)
#  define CLANG_WARN_UNUSED_RESULT [[clang::warn_unused_result]]
# else
#  define CLANG_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
# endif
#else
# define CLANG_WARN_UNUSED_RESULT
#endif

struct Vector {
  CLANG_WARN_UNUSED_RESULT bool empty() const;
};

void f() {
  Vector v;
  v.empty();
}
$ clang_check q.cc -- -std=c++11
q.cc:17:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  v.empty();
  ^~~~~~~
1 warning generated.

It would be super-awesome, if someone could add appropriate annotations to all methods without side effects in libc++ ;)

clang-tidy/misc/NoSideEffectsCheck.cpp
42

I think, there's a slightly more effective way of doing this in clang-tidy/readability/ContainerSizeEmptyCheck.cpp. Also, Samuel is working on a hasAnyName matcher that would allow checking whether a declaration name is one of the declaration names in a list.

52

Can we assume that all const methods shouldn't be called without using their result? This will make this list much shorter.

113

The line violates 80-columns limit. There are other formatting issues as well, so I suggest just running clang-format on the file.

docs/clang-tidy/checks/list.rst
59

Maybe "misc-unused-result" (like a similar -Wunused-result warning that can be used for the same purpose, if the methods are properly annotated)?

test/clang-tidy/misc-no-side-effects.cpp
2

We don't use actual library headers in tests for multiple reasons (longer testing times, changing the system library can break tests, we can't test different libraries that way, some testing setups don't support this, etc.). Instead, we use stubs that model the part of the API required for testing.

I'd like to note that there is a library-side way to mitigate this issue using the [[clang::warn_unused_result]] attribute on vector::empty() and similar methods:

Using an attribute sounds much better than this kind of check and I'd be fine with throwing this patch out completely. IMHO the diagnostic is bad, though. A message like this:

q.cc:17:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  v.empty();
  ^~~~~~~

…is way too literal. It could indicate two completely different issues:

  1. empty() just tells you whether the container is empty.
  2. empty() has side effects and returns a value which should never be ignored (like recv, send, and scanf).

Searching for this warning (https://www.google.com/search?q="Wunused-result") turns up tons of confusion.

I'm not an expert on attributes. Is there a way to attach metadata (like a subtype, or a string description), so that calls without side effects could have a diagnostic more like the one I wrote, and things like recv(2) and scanf(3) could have rich messages?

I'm going to hold of on addressing any of your other comments until I hear back. Thanks for the review!

alexfh edited edge metadata.Feb 10 2016, 1:23 PM
alexfh added a subscriber: rsmith.

I'd like to note that there is a library-side way to mitigate this issue using the [[clang::warn_unused_result]] attribute on vector::empty() and similar methods:

Using an attribute sounds much better than this kind of check and I'd be fine with throwing this patch out completely. IMHO the diagnostic is bad, though. A message like this:

q.cc:17:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  v.empty();
  ^~~~~~~

…is way too literal. It could indicate two completely different issues:

  1. empty() just tells you whether the container is empty.
  2. empty() has side effects and returns a value which should never be ignored (like recv, send, and scanf).

Searching for this warning (https://www.google.com/search?q="Wunused-result") turns up tons of confusion.

I'm not an expert on attributes. Is there a way to attach metadata (like a subtype, or a string description), so that calls without side effects could have a diagnostic more like the one I wrote, and things like recv(2) and scanf(3) could have rich messages?

The attribute can have arguments (e.g. [[deprecated("use some other API")]] void f();), so if there is a good reason, an argument (e.g. a custom message) can be added to the warn_unused_result attribute. Richard might have thoughts on the matter.

I'm going to hold of on addressing any of your other comments until I hear back. Thanks for the review!

The attribute can have arguments (e.g. [[deprecated("use some other API")]] void f();), so if there is a good reason, an argument (e.g. a custom message) can be added to the warn_unused_result attribute. Richard might have thoughts on the matter.

Ah, nice! It looks like the format is fairly flexible, too. Maybe it could take presets like warn_unused_result(no_side_effects) for common cases in addition to a string for a custom one? I'd be happy to try to work on this once there's a consensus on how/if it should work.

clang-tidy/misc/NoSideEffectsCheck.cpp
52

Good idea. I originally considered making a check that just looks for const methods with unused return values, but Marshall reminded me that const methods in third party code might have side effects. Now that this patch is restricted to STL containers it's safe, though.

alexfh requested changes to this revision.Apr 1 2016, 7:00 PM
alexfh edited edge metadata.

What's the status of this patch? Do you still want to continue working on it or are you fine with the warn_unused_result/nodiscard-based solution?

This revision now requires changes to proceed.Apr 1 2016, 7:00 PM

What's the status of this patch? Do you still want to continue working on it or are you fine with the warn_unused_result/nodiscard-based solution?

I'm still interested in working on this, but I was waiting for y'all (maybe Richard?).

IMHO, the current diagnostic for warn_unused_result is confusing (see the Google search and examples above). Adding a parameter sounds great as long as it supports both canned answers (e.g. nodiscard(nosideeffects)), to avoid having the same message in many places, and literal strings (e.g. nodiscard("recv() returns the length of the received message, its return value should always be used.")).

If that won't happen and you all don't have a better idea, I can keep working on this patch.

The version of this attribute that was voted into the C++ standard (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0189r1.pdf) does not support a message, and I expect that is the version that libc++ will want to use.

I think we should at least improve Clang's diagnostic message here, maybe something like:

warning: value returned by call to function 'blah' should always be used
note: 'blah' declared with attribute 'nodiscard' here

It would also seem sensible to propose extending the standard attribute with an optional message.