[clang-tidy] Fix crash in readability-redundant-string-cstr

[njames93]

Summary

Addresses clang-tidy-11: Crash in DynTypedMatcher::matches during readability-redundant-string-cstr check

[gribozavr2]

Could you provide more information about why these null checks are needed in this case?

[njames93]

In D76761#1941070, @gribozavr2 wrote:

Could you provide more information about why these null checks are needed in this case?

Well the fact it was crashing without those null checks was the reason they were added. I'm not 100% sure of the root cause and this could just be a case of applying a plaster to a broken arm. I'm guessing the root cause of the crash is the fact the callee is a CXXMethodDecl which is not being handled but I honestly don't know.

[gribozavr2]

In D76761#1941380, @njames93 wrote:

In D76761#1941070, @gribozavr2 wrote:

Could you provide more information about why these null checks are needed in this case?

Well the fact it was crashing without those null checks was the reason they were added. I'm not 100% sure of the root cause and this could just be a case of applying a plaster to a broken arm. I'm guessing the root cause of the crash is the fact the callee is a CXXMethodDecl which is not being handled but I honestly don't know.

Right -- what I meant is a more detailed description of why, for example, tryGetCallExprAncestorForCxxConstructExpr can't find the CallExpr in this case -- is it not there, or does it not have the expected shape, or something else? What does the AST look like?

I'm worried about adding defensive checks because they can make code more difficult to fix in future.

[njames93]

• Fixed by replacing cumbersome code with the arguably proper solution

[njames93]

In D76761#1941476, @gribozavr2 wrote:

Right -- what I meant is a more detailed description of why, for example, tryGetCallExprAncestorForCxxConstructExpr can't find the CallExpr in this case -- is it not there, or does it not have the expected shape, or something else? What does the AST look like?

I'm worried about adding defensive checks because they can make code more difficult to fix in future.

To be honest the whole fix that caused the crash in the first place was a mess, I have decided to fix this by checking if the parent is a temporary expr bound to an r value, as well as removing the hacky looking code from the previous patch.

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

if (Text.empty())
     return std::string();
   // Add leading '*'.
   if (needParensAfterUnaryOperator(ExprNode)) {
     return (llvm::Twine("*(") + Text + ")").str();
   }
   return (llvm::Twine("*") + Text).str();
 }
 
-// Trying to get CallExpr in which CxxConstructExpr is called.
-static const clang::CallExpr *
-tryGetCallExprAncestorForCxxConstructExpr(const Expr *TheExpr,
-                                          ASTContext &Context) {
-  // We skip nodes such as CXXBindTemporaryExpr, MaterializeTemporaryExpr.
-  for (ast_type_traits::DynTypedNode DynParent : Context.getParents(*TheExpr)) {
-    if (const auto *Parent = DynParent.get<Expr>()) {
-      if (const auto *TheCallExpr = dyn_cast<CallExpr>(Parent))
-        return TheCallExpr;
-
-      if (const clang::CallExpr *TheCallExpr =
-              tryGetCallExprAncestorForCxxConstructExpr(Parent, Context))
-        return TheCallExpr;
-    }
-  }
-
-  return nullptr;
-}
-
-// Check that ParamDecl of CallExprDecl has rvalue type.
-static bool checkParamDeclOfAncestorCallExprHasRValueRefType(
-    const Expr *TheCxxConstructExpr, ASTContext &Context) {
-  if (const clang::CallExpr *TheCallExpr =
-          tryGetCallExprAncestorForCxxConstructExpr(TheCxxConstructExpr,
-                                                    Context)) {
-    for (unsigned i = 0; i < TheCallExpr->getNumArgs(); ++i) {
-      const Expr *Arg = TheCallExpr->getArg(i);
-      if (Arg->getSourceRange() == TheCxxConstructExpr->getSourceRange()) {
-        if (const auto *TheCallExprFuncProto =
-                TheCallExpr->getCallee()
-                    ->getType()
-                    ->getPointeeType()
-                    ->getAs<FunctionProtoType>()) {
-          if (TheCallExprFuncProto->getParamType(i)->isRValueReferenceType())
-            return true;
-        }
-      }
-    }
-  }
-
-  return false;
-}
-
-AST_MATCHER(CXXConstructExpr,
-            matchedParamDeclOfAncestorCallExprHasRValueRefType) {
-  return checkParamDeclOfAncestorCallExprHasRValueRefType(
-      &Node, Finder->getASTContext());
+AST_MATCHER(MaterializeTemporaryExpr, isBoundToLValue) {
+  return Node.isBoundToLvalueReference();
 }
 
 } // end namespace
 
 void RedundantStringCStrCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   // Match expressions of type 'string' or 'string*'.
   const auto StringDecl = type(hasUnqualifiedDesugaredType(recordType(
const auto StringCStrCallExpr =
       cxxMemberCallExpr(on(StringExpr.bind("arg")),
                         callee(memberExpr().bind("member")),
                         callee(cxxMethodDecl(hasAnyName("c_str", "data"))))
           .bind("call");
 
   // Detect redundant 'c_str()' calls through a string constructor.
   // If CxxConstructExpr is the part of some CallExpr we need to
   // check that matched ParamDecl of the ancestor CallExpr is not rvalue.
-  Finder->addMatcher(
-      cxxConstructExpr(
-          StringConstructorExpr, hasArgument(0, StringCStrCallExpr),
-          unless(matchedParamDeclOfAncestorCallExprHasRValueRefType())),
+  Finder->addMatcher(cxxConstructExpr(StringConstructorExpr,
+                                      hasArgument(0, StringCStrCallExpr),
+                                      unless(hasParent(materializeTemporaryExpr(
+                                          unless(isBoundToLValue()))))),
                      this);
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(
       cxxOperatorCallExpr(
           hasAnyOverloadedOperatorName("<", ">", ">=", "<=", "!=", "==", "+"),
           anyOf(allOf(hasArgument(0, StringExpr),
                       hasArgument(1, StringCStrCallExpr)),
                 allOf(hasArgument(0, StringCStrCallExpr),

clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp

void m1(std::string&&) {
   void (*m1p1)(std::string&&);
   m1p1 = m1;
   m1p1(s.c_str());
 
   using m1tp = void (*)(std::string &&);
   m1tp m1p2 = m1;
   m1p2(s.c_str());  
 }
+
+namespace PR45286 {
+struct Foo {
+  void func(const std::string &) {}
+  void func2(std::string &&) {}
+};
+
+void bar() {
+  std::string Str{"aaa"};
+  Foo Foo;
+  Foo.func(Str.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}Foo.func(Str);{{$}}
+
+  // Ensure it doesn't transform Binding to r values
+  Foo.func2(Str.c_str());
+
+  // Ensure its not confused by parens
+  Foo.func((Str.c_str()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}Foo.func((Str));{{$}}
+  Foo.func2((Str.c_str()));
+}
+} // namespace PR45286