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

[sidney]

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.

2. 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

[sidney]

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

[alexfh]

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.

[sidney]

In D17043#348525, @alexfh wrote:

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]

In D17043#348569, @sidney wrote:

In D17043#348525, @alexfh wrote:

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!

[sidney]

In D17043#348982, @alexfh wrote:

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]

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?

[sidney]

In D17043#390188, @alexfh wrote:

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.

[rsmith]

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.

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
+  NoSideEffectsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
+#include "NoSideEffectsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "misc-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveConstantArgumentCheck>(
         "misc-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
+    CheckFactories.registerCheck<NoSideEffectsCheck>(
+        "misc-no-side-effects");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(

clang-tidy/misc/NoSideEffectsCheck.h

+//===--- NoSideEffectsCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NO_SIDE_EFFECTS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NO_SIDE_EFFECTS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Finds calls of side-effect-free methods on STL containers that ignore
+/// the return value. This could catch mistakes like using .empty() to
+/// (try to) empty a container.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-no-side-effects.html
+class NoSideEffectsCheck : public ClangTidyCheck {
+public:
+  NoSideEffectsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NO_SIDE_EFFECTS_H

clang-tidy/misc/NoSideEffectsCheck.cpp

+//===--- NoSideEffectsCheck.cpp - clang-tidy-------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NoSideEffectsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <unordered_set>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+AST_MATCHER(CXXRecordDecl, isStdContainer) {
+  static const std::unordered_set<std::string> whitelist{
+    "std::array",
+    "std::deque",
+    "std::forward_list",
+    "std::list",
+    "std::map",
+    "std::multimap",
+    "std::multiset",
+    "std::priority_queue",
+    "std::queue",
+    "std::set",
+    "std::stack",
+    "std::unordered_map",
+    "std::unordered_multimap",
+    "std::unordered_multiset",
+    "std::unordered_set",
+    "std::vector",
+    "std::__vector_base"
+  };
+
+  std::string QualName;
+  llvm::raw_string_ostream OS{QualName};
+  PrintingPolicy Policy = Node.getASTContext().getPrintingPolicy();
+  Policy.SuppressUnwrittenScope = true;
+  Node.printQualifiedName(OS, Policy);
+
+  return whitelist.find(OS.str()) != whitelist.end();
+}
+
+AST_MATCHER(CXXMethodDecl, isAPurelyInformationalMethod) {
+  static const std::unordered_set<std::string> whitelist{
+    "get_allocator",
+    "top",
+    "at",
+    "front",
+    "back",
+    "data",
+    "before_begin",
+    "cbefore_begin",
+    "begin",
+    "cbegin",
+    "end",
+    "cend",
+    "rbegin",
+    "crbegin",
+    "rend",
+    "crend",
+    "empty",
+    "size",
+    "max_size",
+    "capacity",
+    "count",
+    "find",
+    "equal_range",
+    "lower_bound",
+    "upper_bound",
+    "bucket_count",
+    "max_bucket_count",
+    "bucket_size",
+    "bucket",
+    "load_factor",
+    "max_load_factor",
+    "key_comp",
+    "value_comp",
+    "hash_function",
+    "key_eq"
+  };
+
+  // Some containers have a getter and a setter with the same name
+  // (e.g. max_load_factor). The setter will return void.
+  if (Node.getReturnType()->isVoidType()) {
+      return false;
+  }
+
+  return whitelist.find(Node.getName()) != whitelist.end();
+}
+
+
+void NoSideEffectsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(cxxMemberCallExpr(allOf(
+    anyOf(hasParent(compoundStmt()), hasParent(exprWithCleanups())),
+    callee(cxxMethodDecl(allOf(
+      ofClass(isStdContainer()),
+      isAPurelyInformationalMethod()
+    )))
+  )).bind("call"), this);
+}
+
+void NoSideEffectsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto &Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+
+  diag(Call->getCallee()->getExprLoc(), "method '%0' has no side effects and its return value is not used")
+    << Call->getMethodDecl()->getName()
+  ;
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-widening-cast
    misc-move-constructor-init
    misc-new-delete-overloads
+   misc-no-side-effects
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
    misc-string-integer-assignment
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-undelegated-constructor

docs/clang-tidy/checks/misc-no-side-effects.rst

+.. title:: clang-tidy - misc-no-side-effects
+
+misc-no-side-effects
+====================
+
+Warn when specific methods without side effects (e.g. ``v.empty()``) are called
+and the return value ignored: the programmer might think that it does have side
+effects (e.g. emptying the container).

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

+// RUN: %check_clang_tidy %s misc-no-side-effects %t
+#include <vector>
+
+class derivedVector: public std::vector<int> {};
+class customVector { public: bool empty() { return false; } };
+
+int main() {
+
+  std::vector<int>{}.empty();
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: method 'empty' has no side effects and its return value is not used [misc-no-side-effects]
+  derivedVector{}.empty();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: method 'empty' has no side effects and its return value is not used [misc-no-side-effects]
+
+  // A future version of this checker might catch this one either by
+  // determining that the method has no side effects or by an annotation, but
+  // the current one should not.
+  customVector{}.empty();
+}