[analyzer][NFC] Add unittests for CallDescription and split the old ones

[steakhal]

Summary

This NFC change accomplishes three things:

1. Splits up the single unittest into reasonable segments.
2. Extends the test infra using a template to select the AST-node from which it is supposed to construct a CallEvent.
3. Adds a *lot* of different tests, documenting the current capabilities of the CallDescription. The corresponding tests are marked with FIXMEs, where the current behavior should be different.

Both CXXMemberCallExpr and CXXOperatorCallExpr are derived from
CallExpr, so they are matched by using the default template parameter.
On the other hand, CXXConstructExpr is not derived from CallExpr.
In case we want to match for them, we need to pass the type explicitly
to the CallDescriptionAction.

About destructors:
They have no AST-node, but they are generated in the CFG machinery in
the analyzer. Thus, to be able to match against them, we would need to
construct a CFG and walk on that instead of simply walking the AST.

I'm also relaxing the EXPECTation in the
CallDescriptionConsumer::performTest(), to check the LookupResult
only if we matched for the CallDescription.
This is necessary to allow tests in which we expect *no* matches at all.

[martong]

Nice work! LGTM (with some nits)!

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

151

If we had a call expr foo(1, 2) would that match {"foo", None} or {"foo", 2} ?

235

Clever to use this macro!

348

Could you please either have a better and more descriptive name for these tests or could you add a comment? I could hardly found the difference between AliasNames1 and AliasNames2.

Maybe these could go under the same test case with Code1 and Code2 ?

[steakhal]

Restructured the tests for alias handling.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

151

It actually won't.
The CallDescriptionMap expects at most one CallDescription to match the Call.
Thus, having *two* CDs that we expect to match, breaks this invariant.
Maybe we could have an expensive check enforcing this property, that at most one could match.

In the implementation, we should have the freedom of the evaluation order of the CallDescriptions within the map. It's also important for caching, in the future.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

 //===- unittests/StaticAnalyzer/CallDescriptionTest.cpp -------------------===//
 //
 // 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 "Reusables.h"
 
+#include "clang/AST/ExprCXX.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include <type_traits>
 
 namespace clang {
 namespace ento {
 namespace {
 
 // A wrapper around CallDescriptionMap<bool> that allows verifying that
 // all functions have been found. This is needed because CallDescriptionMap
 // isn't supposed to support iteration.
public:
   // Fail the test if we haven't found all the true-calls we were looking for.
   ~ResultMap() { EXPECT_EQ(Found, Total); }
 };
 
 // Scan the code body for call expressions and see if we find all calls that
 // we were supposed to find ("true" in the provided ResultMap) and that we
 // don't find the ones that we weren't supposed to find
 // ("false" in the ResultMap).
+template <typename MatchedExprT>
 class CallDescriptionConsumer : public ExprEngineConsumer {
   ResultMap &RM;
   void performTest(const Decl *D) {
     using namespace ast_matchers;
+    using T = MatchedExprT;
 
     if (!D->hasBody())
       return;
 
-    const CallExpr *CE = findNode<CallExpr>(D, callExpr());
     const StackFrameContext *SFC =
         Eng.getAnalysisDeclContextManager().getStackFrame(D);
-    ProgramStateRef State = Eng.getInitialState(SFC);
-    CallEventRef<> Call =
-        Eng.getStateManager().getCallEventManager().getCall(CE, State, SFC);
+    const ProgramStateRef State = Eng.getInitialState(SFC);
 
+    // FIXME: Maybe use std::variant and std::visit for these.
+    const auto MatcherCreator = []() {
+      if (std::is_same<T, CallExpr>::value)
+        return callExpr();
+      if (std::is_same<T, CXXConstructExpr>::value)
+        return cxxConstructExpr();
+      if (std::is_same<T, CXXMemberCallExpr>::value)
+        return cxxMemberCallExpr();
+      if (std::is_same<T, CXXOperatorCallExpr>::value)
+        return cxxOperatorCallExpr();
+      llvm_unreachable("Only these expressions are supported for now.");
+    };
+
+    const Expr *E = findNode<T>(D, MatcherCreator());
+
+    CallEventManager &CEMgr = Eng.getStateManager().getCallEventManager();
+    CallEventRef<> Call = [=, &CEMgr]() -> CallEventRef<CallEvent> {
+      if (std::is_base_of<CallExpr, T>::value)
+        return CEMgr.getCall(E, State, SFC);
+      if (std::is_same<T, CXXConstructExpr>::value)
+        return CEMgr.getCXXConstructorCall(cast<CXXConstructExpr>(E),
+                                           /*Target=*/nullptr, State, SFC);
+      llvm_unreachable("Only these expressions are supported for now.");
+    }();
+
+    // If the call actually matched, check if we really expected it to match.
     const bool *LookupResult = RM.lookup(*Call);
-    // Check that we've found the function in the map
-    // with the correct description.
-    EXPECT_TRUE(LookupResult && *LookupResult);
+    EXPECT_TRUE(!LookupResult || *LookupResult);
 
     // ResultMap is responsible for making sure that we've found *all* calls.
   }
 
 public:
   CallDescriptionConsumer(CompilerInstance &C,
                           ResultMap &RM)
       : ExprEngineConsumer(C), RM(RM) {}
 
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
     for (const auto *D : DG)
       performTest(D);
     return true;
   }
 };
 
+template <typename MatchedExprT = CallExpr>
 class CallDescriptionAction : public ASTFrontendAction {
   ResultMap RM;
 
 public:
   CallDescriptionAction(
       std::initializer_list<std::pair<CallDescription, bool>> Data)
       : RM(Data) {}
 
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
                                                  StringRef File) override {
-    return std::make_unique<CallDescriptionConsumer>(Compiler, RM);
+    return std::make_unique<CallDescriptionConsumer<MatchedExprT>>(Compiler,
+                                                                   RM);
   }
 };
 
-TEST(CallEvent, CallDescription) {
-  // Test simple name matching.
+TEST(CallDescription, SimpleNameMatching) {
   EXPECT_TRUE(tooling::runToolOnCode(
-      std::unique_ptr<CallDescriptionAction>(new CallDescriptionAction({
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
           {{"bar"}, false}, // false: there's no call to 'bar' in this code.
           {{"foo"}, true},  // true: there's a call to 'foo' in this code.
-      })), "void foo(); void bar() { foo(); }"));
+      })),
+      "void foo(); void bar() { foo(); }"));
+}
 
-  // Test arguments check.
+TEST(CallDescription, RequiredArguments) {
   EXPECT_TRUE(tooling::runToolOnCode(
-      std::unique_ptr<CallDescriptionAction>(new CallDescriptionAction({
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
           {{"foo", 1}, true},
           {{"foo", 2}, false},
-      })), "void foo(int); void foo(int, int); void bar() { foo(1); }"));
+      })),
+      "void foo(int); void foo(int, int); void bar() { foo(1); }"));
+}
 
-  // Test lack of arguments check.
+TEST(CallDescription, LackOfRequiredArguments) {
   EXPECT_TRUE(tooling::runToolOnCode(
-      std::unique_ptr<CallDescriptionAction>(new CallDescriptionAction({
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
           {{"foo", None}, true},
           {{"foo", 2}, false},
-      })), "void foo(int); void foo(int, int); void bar() { foo(1); }"));
+      })),
+      "void foo(int); void foo(int, int); void bar() { foo(1); }"));
+}
 
-  // Test qualified names.
+constexpr StringRef MockStdStringHeader = R"code(
+  namespace std { inline namespace __1 {
+    template<typename T> class basic_string {
+      class Allocator {};
+    public:
+      basic_string();
+      explicit basic_string(const char*, const Allocator & = Allocator());
+      ~basic_string();
+      T *c_str();
+    };
+  } // namespace __1
+  using string = __1::basic_string<char>;
+  } // namespace std
+)code";
+
+TEST(CallDescription, QualifiedNames) {
+  constexpr StringRef AdditionalCode = R"code(
+    void foo() {
+      using namespace std;
+      basic_string<char> s;
+      s.c_str();
+    })code";
+  const std::string Code = (Twine{MockStdStringHeader} + AdditionalCode).str();
   EXPECT_TRUE(tooling::runToolOnCode(
-      std::unique_ptr<CallDescriptionAction>(new CallDescriptionAction({
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
           {{{"std", "basic_string", "c_str"}}, true},
       })),
-      "namespace std { inline namespace __1 {"
-      "  template<typename T> class basic_string {"
-      "  public:"
-      "    T *c_str();"
-      "  };"
-      "}}"
-      "void foo() {"
-      "  using namespace std;"
-      "  basic_string<char> s;"
-      "  s.c_str();"
-      "}"));
+      Code));
+}
+
+TEST(CallDescription, MatchConstructor) {
+  constexpr StringRef AdditionalCode = R"code(
+    void foo() {
+      using namespace std;
+      basic_string<char> s("hello");
+    })code";
+  // FIXME: We should match.
+  const std::string Code = (Twine{MockStdStringHeader} + AdditionalCode).str();
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::unique_ptr<FrontendAction>(
+          new CallDescriptionAction<CXXConstructExpr>({
+              {{{"std", "basic_string", "basic_string"}, 2, 2}, false},
+          })),
+      Code));
+}
+
+// FIXME: Test matching destructors: {"std", "basic_string", "~basic_string"}
+//        This feature is actually implemented, but the test infra is not yet
+//        sophisticated enough for testing this. To do that, we will need to
+//        implement a much more advanced dispatching mechanism using the CFG for
+//        the implicit destructor events.
+
+TEST(CallDescription, MatchConversionOperator) {
+  constexpr StringRef Code = R"code(
+    namespace aaa {
+    namespace bbb {
+    struct Bar {
+      operator int();
+    };
+    } // bbb
+    } // aaa
+    void foo() {
+      aaa::bbb::Bar x;
+      int tmp = x;
+    })code";
+  // FIXME: We should match.
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+          {{{"aaa", "bbb", "Bar", "operator int"}}, false},
+      })),
+      Code));
+}
+
+TEST(CallDescription, RejectOverQualifiedNames) {
+  constexpr auto Code = R"code(
+    namespace my {
+    namespace std {
+      struct container {
+        const char *data() const;
+      };
+    } // namespace std
+    } // namespace my
+
+    void foo() {
+      using namespace my;
+      std::container v;
+      v.data();
+    })code";
+
+  // FIXME: We should **not** match.
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+          {{{"std", "container", "data"}}, true},
+      })),
+      Code));
+}
+
+TEST(CallDescription, DontSkipNonInlineNamespaces) {
+  constexpr auto Code = R"code(
+    namespace my {
+    /*not inline*/ namespace v1 {
+      void bar();
+    } // namespace v1
+    } // namespace my
+    void foo() {
+      my::v1::bar();
+    })code";
+
+  {
+    SCOPED_TRACE("my v1 bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"my", "v1", "bar"}}, true},
+        })),
+        Code));
+  }
+  {
+    // FIXME: We should **not** skip non-inline namespaces.
+    SCOPED_TRACE("my bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"my", "bar"}}, true},
+        })),
+        Code));
+  }
+}
+
+TEST(CallDescription, SkipTopInlineNamespaces) {
+  constexpr auto Code = R"code(
+    inline namespace my {
+    namespace v1 {
+      void bar();
+    } // namespace v1
+    } // namespace my
+    void foo() {
+      using namespace v1;
+      bar();
+    })code";
+
+  {
+    SCOPED_TRACE("my v1 bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"my", "v1", "bar"}}, true},
+        })),
+        Code));
+  }
+  {
+    SCOPED_TRACE("v1 bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"v1", "bar"}}, true},
+        })),
+        Code));
+  }
+}
+
+TEST(CallDescription, SkipAnonimousNamespaces) {
+  constexpr auto Code = R"code(
+    namespace {
+    namespace std {
+    namespace {
+    inline namespace {
+      struct container {
+        const char *data() const { return nullptr; };
+      };
+    } // namespace inline anonymous
+    } // namespace anonymous
+    } // namespace std
+    } // namespace anonymous
+
+    void foo() {
+      std::container v;
+      v.data();
+    })code";
+
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+          {{{"std", "container", "data"}}, true},
+      })),
+      Code));
+}
+
+TEST(CallDescription, AliasNames) {
+  constexpr StringRef AliasNamesCode = R"code(
+  namespace std {
+    struct container {
+      const char *data() const;
+    };
+    using cont = container;
+  } // std
+)code";
+
+  constexpr StringRef UseAliasInSpelling = R"code(
+    void foo() {
+      std::cont v;
+      v.data();
+    })code";
+  constexpr StringRef UseStructNameInSpelling = R"code(
+    void foo() {
+      std::container v;
+      v.data();
+    })code";
+  const std::string UseAliasInSpellingCode =
+      (Twine{AliasNamesCode} + UseAliasInSpelling).str();
+  const std::string UseStructNameInSpellingCode =
+      (Twine{AliasNamesCode} + UseStructNameInSpelling).str();
+
+  // Test if the code spells the alias, wile we match against the struct name,
+  // and again matching against the alias.
+  {
+    SCOPED_TRACE("Using alias in spelling");
+    {
+      SCOPED_TRACE("std container data");
+      EXPECT_TRUE(tooling::runToolOnCode(
+          std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+              {{{"std", "container", "data"}}, true},
+          })),
+          UseAliasInSpellingCode));
+    }
+    {
+      // FIXME: We should be able to see-through aliases.
+      SCOPED_TRACE("std cont data");
+      EXPECT_TRUE(tooling::runToolOnCode(
+          std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+              {{{"std", "cont", "data"}}, false},
+          })),
+          UseAliasInSpellingCode));
+    }
+  }
 
-  // A negative test for qualified names.
+  // Test if the code spells the struct name, wile we match against the struct
+  // name, and again matching against the alias.
+  {
+    SCOPED_TRACE("Using struct name in spelling");
+    {
+      SCOPED_TRACE("std container data");
+      EXPECT_TRUE(tooling::runToolOnCode(
+          std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+              {{{"std", "container", "data"}}, true},
+          })),
+          UseAliasInSpellingCode));
+    }
+    {
+      // FIXME: We should be able to see-through aliases.
+      SCOPED_TRACE("std cont data");
       EXPECT_TRUE(tooling::runToolOnCode(
-      std::unique_ptr<CallDescriptionAction>(new CallDescriptionAction({
+          std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+              {{{"std", "cont", "data"}}, false},
+          })),
+          UseAliasInSpellingCode));
+    }
+  }
+}
+
+TEST(CallDescription, AliasSingleNamespace) {
+  constexpr StringRef Code = R"code(
+    namespace aaa {
+    namespace bbb {
+    namespace ccc {
+      void bar();
+    }} // namespace bbb::ccc
+    namespace bbb_alias = bbb;
+    } // namespace aaa
+    void foo() {
+      aaa::bbb_alias::ccc::bar();
+    })code";
+  {
+    SCOPED_TRACE("aaa bbb ccc bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"aaa", "bbb", "ccc", "bar"}}, true},
+        })),
+        Code));
+  }
+  {
+    // FIXME: We should be able to see-through namespace aliases.
+    SCOPED_TRACE("aaa bbb_alias ccc bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"aaa", "bbb_alias", "ccc", "bar"}}, false},
+        })),
+        Code));
+  }
+}
+
+TEST(CallDescription, AliasMultipleNamespaces) {
+  constexpr StringRef Code = R"code(
+    namespace aaa {
+    namespace bbb {
+    namespace ccc {
+      void bar();
+    }}} // namespace aaa::bbb::ccc
+    namespace aaa_bbb_ccc = aaa::bbb::ccc;
+    void foo() {
+      using namespace aaa_bbb_ccc;
+      bar();
+    })code";
+  {
+    SCOPED_TRACE("aaa bbb ccc bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"aaa", "bbb", "ccc", "bar"}}, true},
+        })),
+        Code));
+  }
+  {
+    // FIXME: We should be able to see-through namespace aliases.
+    SCOPED_TRACE("aaa_bbb_ccc bar");
+    EXPECT_TRUE(tooling::runToolOnCode(
+        std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
+            {{{"aaa_bbb_ccc", "bar"}}, false},
+        })),
+        Code));
+  }
+}
+
+TEST(CallDescription, NegativeMatchQualifiedNames) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>({
           {{{"foo", "bar"}}, false},
           {{{"bar", "foo"}}, false},
           {{"foo"}, true},
-      })), "void foo(); struct bar { void foo(); }; void test() { foo(); }"));
+      })),
+      "void foo(); struct bar { void foo(); }; void test() { foo(); }"));
+}
 
+TEST(CallDescription, MatchBuiltins) {
   // Test CDF_MaybeBuiltin - a flag that allows matching weird builtins.
   EXPECT_TRUE(tooling::runToolOnCode(
-      std::unique_ptr<CallDescriptionAction>(new CallDescriptionAction({
-          {{"memset", 3}, false},
-          {{CDF_MaybeBuiltin, "memset", 3}, true}
-      })),
+      std::unique_ptr<FrontendAction>(new CallDescriptionAction<>(
+          {{{"memset", 3}, false}, {{CDF_MaybeBuiltin, "memset", 3}, true}})),
       "void foo() {"
       "  int x;"
       "  __builtin___memset_chk(&x, 0, sizeof(x),"
       "                         __builtin_object_size(&x, 0));"
       "}"));
 }
 
 } // namespace
 } // namespace ento
 } // namespace clang