Extraction of a matcher for an unused value from an expression from the bugprone-unused-return-value check

[barcisz]

Summary

This diff extracts the matcher for an unused expression result from bugprone-unused-return-value into a utility. This diff is a prerequisite to D133804 which introduces
the cuda-unsafe-api-call-check

[njames93]

I feel like this matcher could do with some unit testing. I'd say create a test file in clang-tools-extra/unittests/clang-tidy/MatchersTest.cpp

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

143–144

Can all these be removed now?

[barcisz]

Removed unused matcher definitions from bugprone-unused-return-value

[barcisz]

This comment was removed by barcisz.

[barcisz]

Tests for the check

[njames93]

clang-tools-extra/unittests/clang-tidy/MatchersTest.cpp

2

@kwk I seem to recall you saying in another patch that this include will break standalone builds, would stripping the leading ../../ work?

[kwk]

clang-tools-extra/unittests/clang-tidy/MatchersTest.cpp

2

@kwk I seem to recall you saying in another patch that this include will break standalone builds,

@njames93 that's correct. But I don't recall the patch ID :/

would stripping the leading ../../ work?

I don't know if the file is found then but it would make standalone builds certainly easier.

I'm not sure if I calculate correctly but given:

clang-tools-extra/unittests/clang-tidy/MatchersTest.cpp

then ../../ means clang-tools-extra/, no? If so, that directory doesn't contain a clang directory here. It seems as if the ../../ is relative to some other directory but not this file. Or am I too tired to get it?

[njames93]

clang-tools-extra/unittests/clang-tidy/MatchersTest.cpp

2

I'll test when I get a few cycles. If not, may need to copy some of the testing logic for this.

[barcisz]

clang-tools-extra/unittests/clang-tidy/MatchersTest.cpp

2

Yes, the tests do indeed work but it's probably because it's getting the include path from another directory. I was actually thinking of adding the directory for clang as an include path within cmake but thought this would be better. There is also an option of copying the testing setup from clang and pasting it here, or to just create testing checks and use the existing check testing utilities to run the matchers inside of those checks. What do you reckon is the best solution in that case to the problem of standalone builds?

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

 //===--- UnusedReturnValueCheck.cpp - clang-tidy---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "UnusedReturnValueCheck.h"
+#include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::ast_matchers::internal;
 
 namespace clang {

 
 void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
   auto FunVec = utils::options::parseStringList(CheckedFunctions);
   auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts(
       callExpr(callee(functionDecl(
                    // Don't match void overloads of checked functions.
                    unless(returns(voidType())),
                    isInstantiatedFrom(hasAnyName(FunVec)))))
           .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.
-                   unless(hasParent(stmtExpr())));
-  auto UnusedInIfStmt =
-      ifStmt(eachOf(hasThen(MatchedCallExpr), hasElse(MatchedCallExpr)));
-  auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr));
-  auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr));
-  auto UnusedInForStmt =
-      forStmt(eachOf(hasLoopInit(MatchedCallExpr),
-                     hasIncrement(MatchedCallExpr), hasBody(MatchedCallExpr)));
-  auto UnusedInRangeForStmt = cxxForRangeStmt(hasBody(MatchedCallExpr));
-  auto UnusedInCaseStmt = switchCase(forEach(MatchedCallExpr));
-
   Finder->addMatcher(
-      stmt(anyOf(UnusedInCompoundStmt, UnusedInIfStmt, UnusedInWhileStmt,
-                 UnusedInDoStmt, UnusedInForStmt, UnusedInRangeForStmt,
-                 UnusedInCaseStmt)),
-      this);
+      functionDecl(hasBody(matchers::isValueUnused(MatchedCallExpr))), this);
 }
 
 void UnusedReturnValueCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Matched = Result.Nodes.getNodeAs<CallExpr>("match")) {
     diag(Matched->getBeginLoc(),
          "the value returned by this function should be used")
         << Matched->getSourceRange();
     diag(Matched->getBeginLoc(),
          "cast the expression to void to silence this warning",
          DiagnosticIDs::Note);
   }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/utils/Matchers.h

 }
 
 // Returns QualType matcher for pointers to const.
 AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToConst) {
   using namespace ast_matchers;
   return pointerType(pointee(qualType(isConstQualified())));
 }
 
+// Matches the statements in a GNU statement-expression that are not returned
+// from it.
+AST_MATCHER_P(StmtExpr, hasUnreturning,
+              clang::ast_matchers::internal::Matcher<Stmt>, matcher) {
+  const auto compoundStmt = Node.getSubStmt();
+  assert(compoundStmt);
+
+  clang::ast_matchers::internal::BoundNodesTreeBuilder result;
+  bool matched = false;
+  for (auto stmt = compoundStmt->body_begin();
+       stmt + 1 < compoundStmt->body_end(); ++stmt) {
+    clang::ast_matchers::internal::BoundNodesTreeBuilder builderInner(*Builder);
+    assert(stmt && *stmt);
+    if (matcher.matches(**stmt, Finder, &builderInner)) {
+      result.addMatch(builderInner);
+      matched = true;
+    }
+  }
+  *Builder = result;
+  return matched;
+}
+
+// Matches all of the nodes (simmilar to forEach) that match the matcher
+// and have return values not used in any statement.
+AST_MATCHER_FUNCTION_P(ast_matchers::StatementMatcher, isValueUnused,
+                       ast_matchers::StatementMatcher, Matcher) {
+  using namespace ast_matchers;
+  const auto UnusedInCompoundStmt =
+      compoundStmt(forEach(Matcher), unless(hasParent(stmtExpr())));
+  const auto UnusedInGnuExprStmt = stmtExpr(hasUnreturning(Matcher));
+  const auto UnusedInIfStmt =
+      ifStmt(eachOf(hasThen(Matcher), hasElse(Matcher)));
+  const auto UnusedInWhileStmt = whileStmt(hasBody(Matcher));
+  const auto UnusedInDoStmt = doStmt(hasBody(Matcher));
+  const auto UnusedInForStmt = forStmt(
+      eachOf(hasLoopInit(Matcher), hasIncrement(Matcher), hasBody(Matcher)));
+  const auto UnusedInRangeForStmt = cxxForRangeStmt(hasBody(Matcher));
+  const auto UnusedInCaseStmt = switchCase(forEach(Matcher));
+  const auto Unused =
+      stmt(anyOf(UnusedInCompoundStmt, UnusedInGnuExprStmt, UnusedInIfStmt,
+                 UnusedInWhileStmt, UnusedInDoStmt, UnusedInForStmt,
+                 UnusedInRangeForStmt, UnusedInCaseStmt));
+  return stmt(eachOf(Unused, forEachDescendant(Unused)));
+}
+
 // A matcher implementation that matches a list of type name regular expressions
 // against a NamedDecl. If a regular expression contains the substring "::"
 // matching will occur against the qualified name, otherwise only the typename.
 class MatchesAnyListedNameMatcher
     : public ast_matchers::internal::MatcherInterface<NamedDecl> {
 public:
   explicit MatchesAnyListedNameMatcher(llvm::ArrayRef<StringRef> NameList) {
     std::transform(

clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

add_extra_unittest(ClangTidyTests
   AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   DeclRefExprUtilsTest.cpp
   IncludeInserterTest.cpp
   GlobListTest.cpp
   GoogleModuleTest.cpp
   LLVMModuleTest.cpp
+  MatchersTest.cpp
   ModernizeModuleTest.cpp
   NamespaceAliaserTest.cpp
   ObjCModuleTest.cpp
   OptionsProviderTest.cpp
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
   TransformerClangTidyCheckTest.cpp
   )
 
 clang_target_link_libraries(ClangTidyTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend
   clangLex
   clangSerialization
+  clangTesting
   clangTooling
   clangToolingCore
   clangTransformer
   )
 target_link_libraries(ClangTidyTests
   PRIVATE
   clangTidy
   clangTidyAndroidModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyModernizeModule
   clangTidyObjCModule
   clangTidyReadabilityModule
   clangTidyUtils
   LLVMTestingSupport
   )

clang-tools-extra/unittests/clang-tidy/MatchersTest.cpp

+#include "utils/Matchers.h"
+#include "../../clang/unittests/ASTMatchers/ASTMatchersTest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace ast_matchers;
+
+TEST_P(ASTMatchersTest, isValueUnused) {
+  auto Matcher = matchers::isValueUnused(integerLiteral(equals(42)));
+  std::string CodePrefix = "void bar()";
+
+  matches(CodePrefix + "{42;}", Matcher);
+  matches(CodePrefix + "{int x = ({42; 0;});}", Matcher);
+  matches(CodePrefix + "{if (true) 42;}", Matcher);
+  matches(CodePrefix + "{while(true) 42;}", Matcher);
+  matches(CodePrefix + "{do 42; while(true);}", Matcher);
+  matches(CodePrefix + "{for(;;) 42;}", Matcher);
+  matches(CodePrefix + "{for(42;;) bar();}", Matcher);
+  matches(CodePrefix + "{for(;;42) bar();}", Matcher);
+  matches(CodePrefix + "{int t[] = {1, 2, 3}; for(int x : t) 42;}", Matcher);
+  matches(CodePrefix + "{switch(1) {case 42:}", Matcher);
+
+  notMatches(CodePrefix + "{bar();}", Matcher);
+  notMatches(CodePrefix + "{int x = 42;}", Matcher);
+  notMatches(CodePrefix + "{int x = ({42;});}", Matcher);
+  notMatches(CodePrefix + "{if (42) bar();}", Matcher);
+  notMatches(CodePrefix + "{while(42) bar();}", Matcher);
+  notMatches(CodePrefix + "{do bar(); while(42);}", Matcher);
+  notMatches(CodePrefix + "{for(; 42; )} bar();", Matcher);
+  notMatches(CodePrefix + "switch(1) {default: bar();}", Matcher);
+}
+
+} // namespace test
+} // namespace tidy
+} // namespace clang