[analyzer] Allow matching non-CallExprs using CallDescriptions

[steakhal]

Summary

Fallback to stringification and string comparison if we cannot compare
the IdentifierInfos, which is the case for C++ overloaded operators,
constructors, destructors, etc.

Examples:

{ "std", "basic_string", "basic_string", 2} // match the 2 param std::string constructor
{ "std", "basic_string", "~basic_string" }  // match the std::string destructor
{ "aaa", "bbb", "operator int" } // matches the struct bbb conversion operator to int

[whisperity]

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

67

Oh yeah, if only. But also, sadly, D98477 is stalled... ☹

69

Is there a chance people accidentally passing const X (perhaps involving decltype somewhere) to T cause a problem in this logic? is_same does not remove qualifiers, right?

77

"Unsupported expression kind in CallDescriptor" for message? When an UNREACHABLE is hit, the user is only given the message (just like in the case of assert()) and not a context useful enough to deduce what only these is supposed to mean...

89

(Ditto, as before.)

[steakhal]

Thanks for the review @whisperity.
If you don't have strong objections I wouldn't change the code.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

69

It simply explicitly express the limitations of the unit-test itself.
It's not supposed to be user-facing to the developers.
That being said, I don't want to spew metaprogramming without much reason.

The best would be to somehow reuse the actual dispatching logic embedded into the analyzer expr engine.
However, that is tightly coupled with the exploded graph-building logic, so it won't happen any time soon.

77

By the end of the day, it doesn't really matter what the message is, it's for developers only.
They would have to check the code to make a conscious decision about *what* exactly is missing :D

I don't mind improving the message, but it won't help by any means.

[balazske]

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

333–335

[martong]

Nice way of refactoring the tests!

However, could have a test that checks if the logic "sees through" type aliases? E.g. can we match the ctor of std::string even if we have using X = std::string; X x("banana");

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

333–335

334

Is it possible to have an empty QualifiedName at this point? Should we assert that it is not empty?

[steakhal]

In D111535#3060932, @martong wrote:

Nice way of refactoring the tests!

However, could have a test that checks if the logic "sees through" type aliases? E.g. can we match the ctor of std::string even if we have using X = std::string; X x("banana");

I agree but did not want to pollute the blame of this commit. I want to have tests covering *only* the changed parts.
I'm going to create a follow-up patch covering this case.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

334

Technically it's a class invariant. We must have at least one element in that vector, representing the name of the function/operator/etc. of the entity that we want to match against. In that sense, we should add an assertion in the constructor of the class enforcing this property.

I wanted to focus only on the isCalled() function in this patch, but I can introduce that assertion into the CallDescription class without much problem I suppose. I'm gonna address this.

[steakhal]

I reorganized the changes across the patches. The first NFC patch introduced the unittests to cover a lot more cases and gain confidence that the actual refactor in the second NFC patch does not break anything.
Finally, this patch simply switches the behavior in that sense, to have the expected behavior.

[martong]

LGTM!

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

bool CallEvent::isCalled(const CallDescription &CD) const {
   }
 
   const auto MatchNameOnly = [](const CallDescription &CD,
                                 const NamedDecl *ND) -> bool {
     DeclarationName Name = ND->getDeclName();
     if (const auto *II = Name.getAsIdentifierInfo())
       return II == CD.II.getValue(); // Fast case.
 
-    // Simply report mismatch for:
+    // Fallback to the slow stringification and comparison for:
     // C++ overloaded operators, constructors, destructors, etc.
-    return false;
+    // FIXME This comparison is way SLOWER than comparing pointers.
+    // At some point in the future, we should compare FunctionDecl pointers.
+    return Name.getAsString() == CD.getFunctionName();
   };
 
   const auto ExactMatchArgAndParamCounts =
       [](const CallEvent &Call, const CallDescription &CD) -> bool {
     const bool ArgsMatch =
         !CD.RequiredArgs || CD.RequiredArgs == Call.getNumArgs();
     const bool ParamsMatch =
         !CD.RequiredParams || CD.RequiredParams == Call.parameters().size();

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

void performTest(const Decl *D) {
 
     if (!D->hasBody())
       return;
 
     const StackFrameContext *SFC =
         Eng.getAnalysisDeclContextManager().getStackFrame(D);
     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);
     EXPECT_TRUE(!LookupResult || *LookupResult);
 
     // ResultMap is responsible for making sure that we've found *all* calls.
   }

 }
 
 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},
+              {{{"std", "basic_string", "basic_string"}, 2, 2}, true},
           })),
       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},
+          {{{"aaa", "bbb", "Bar", "operator int"}}, true},
       })),
       Code));
 }
 
 TEST(CallDescription, RejectOverQualifiedNames) {
   constexpr auto Code = R"code(
     namespace my {
     namespace std {