[clang-tools-extra] [clang-tidy] Speed up/rewrite `bugprone-stringview-nullptr` (PR #192889)

[Victor Chernyakin via cfe-commits]

================
@@ -7,294 +7,93 @@
 //===----------------------------------------------------------------------===//
 
 #include "StringviewNullptrCheck.h"
-#include "../utils/TransformerClangTidyCheck.h"
-#include "clang/AST/Decl.h"
-#include "clang/AST/OperationKinds.h"
+#include "../utils/LexerUtils.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Tooling/Transformer/RangeSelector.h"
-#include "clang/Tooling/Transformer/RewriteRule.h"
-#include "clang/Tooling/Transformer/Stencil.h"
-#include "llvm/ADT/StringRef.h"
-
-namespace clang::tidy::bugprone {
 
 using namespace ::clang::ast_matchers;
-using namespace ::clang::transformer;
+
+namespace clang::tidy::bugprone {
 
 namespace {
 
-AST_MATCHER_P(InitListExpr, initCountIs, unsigned, N) {
-  return Node.getNumInits() == N;
-}
+AST_MATCHER(CXXConstructExpr, constructedFromNullptr) {
+  if (Node.getNumArgs() != 1)
+    return false;
 
-AST_MATCHER(VarDecl, isDirectInitialization) {
-  return Node.getInitStyle() != VarDecl::InitializationStyle::CInit;
+  const Expr *Arg = Node.getArg(0);
+  bool ArgValue; // NOLINT(cppcoreguidelines-init-variables)
+  return !Arg->isValueDependent() &&
+         Arg->EvaluateAsBooleanCondition(ArgValue, Finder->getASTContext()) &&
+         !ArgValue;
 }
 
 } // namespace
 
-static RewriteRuleWith<std::string> stringviewNullptrCheckImpl() {
-  auto ConstructionWarning =
-      cat("constructing basic_string_view from null is undefined; replace with "
-          "the default constructor");
-  auto StaticCastWarning =
-      cat("casting to basic_string_view from null is undefined; replace with "
-          "the empty string");
-  auto ArgumentConstructionWarning =
-      cat("passing null as basic_string_view is undefined; replace with the "
-          "empty string");
-  auto AssignmentWarning =
-      cat("assignment to basic_string_view from null is undefined; replace "
-          "with the default constructor");
-  auto RelativeComparisonWarning =
-      cat("comparing basic_string_view to null is undefined; replace with the "
-          "empty string");
-  auto EqualityComparisonWarning =
-      cat("comparing basic_string_view to null is undefined; replace with the "
-          "emptiness query");
-
-  // Matches declarations and expressions of type `basic_string_view`
-  auto HasBasicStringViewType = hasType(hasUnqualifiedDesugaredType(recordType(
-      hasDeclaration(cxxRecordDecl(hasName("::std::basic_string_view"))))));
-
-  // Matches `nullptr` and `(nullptr)` binding to a pointer
-  auto NullLiteral = implicitCastExpr(
-      hasCastKind(CK_NullToPointer),
-      hasSourceExpression(ignoringParens(cxxNullPtrLiteralExpr())));
-
-  // Matches `{nullptr}` and `{(nullptr)}` binding to a pointer
-  auto NullInitList = initListExpr(initCountIs(1), hasInit(0, NullLiteral));
-
-  // Matches `{}`
-  auto EmptyInitList = initListExpr(initCountIs(0));
-
-  // Matches null construction without `basic_string_view` type spelling
-  auto BasicStringViewConstructingFromNullExpr =
-      cxxConstructExpr(
-          HasBasicStringViewType, argumentCountIs(1),
-          hasAnyArgument(/* `hasArgument` would skip over parens */ anyOf(
-              NullLiteral, NullInitList, EmptyInitList)),
-          unless(cxxTemporaryObjectExpr(/* filters out type spellings */)),
-          has(expr().bind("null_arg_expr")))
-          .bind("construct_expr");
-
-  // `std::string_view(null_arg_expr)`
-  auto HandleTemporaryCXXFunctionalCastExpr =
-      makeRule(cxxFunctionalCastExpr(hasSourceExpression(
-                   BasicStringViewConstructingFromNullExpr)),
-               remove(node("null_arg_expr")), ConstructionWarning);
+void StringviewNullptrCheck::registerMatchers(MatchFinder *Finder) {
+  const auto HasBasicStringViewType =
+      hasType(hasUnqualifiedDesugaredType(recordType(
+          hasDeclaration(cxxRecordDecl(hasName("::std::basic_string_view"))))));
 
-  // `std::string_view{null_arg_expr}` and `(std::string_view){null_arg_expr}`
-  auto HandleTemporaryCXXTemporaryObjectExprAndCompoundLiteralExpr = makeRule(
-      cxxTemporaryObjectExpr(cxxConstructExpr(
-          HasBasicStringViewType, argumentCountIs(1),
-          hasAnyArgument(/* `hasArgument` would skip over parens */ anyOf(
-              NullLiteral, NullInitList, EmptyInitList)),
-          has(expr().bind("null_arg_expr")))),
-      remove(node("null_arg_expr")), ConstructionWarning);
-
-  // `(std::string_view) null_arg_expr`
-  auto HandleTemporaryCStyleCastExpr =
-      makeRule(cStyleCastExpr(hasSourceExpression(
-                   BasicStringViewConstructingFromNullExpr)),
-               changeTo(node("null_arg_expr"), cat("{}")), ConstructionWarning);
-
-  // `static_cast<std::string_view>(null_arg_expr)`
-  auto HandleTemporaryCXXStaticCastExpr =
-      makeRule(cxxStaticCastExpr(hasSourceExpression(
-                   BasicStringViewConstructingFromNullExpr)),
-               changeTo(node("null_arg_expr"), cat("\"\"")), StaticCastWarning);
-
-  // `std::string_view sv = null_arg_expr;`
-  auto HandleStackCopyInitialization =
-      makeRule(varDecl(HasBasicStringViewType,
-                       hasInitializer(ignoringImpCasts(cxxConstructExpr(
-                           BasicStringViewConstructingFromNullExpr,
-                           unless(isListInitialization())))),
-                       unless(isDirectInitialization())),
-               changeTo(node("null_arg_expr"), cat("{}")), ConstructionWarning);
-
-  // `std::string_view sv = {null_arg_expr};`
-  auto HandleStackCopyListInitialization =
-      makeRule(varDecl(HasBasicStringViewType,
-                       hasInitializer(cxxConstructExpr(
-                           BasicStringViewConstructingFromNullExpr,
-                           isListInitialization())),
-                       unless(isDirectInitialization())),
-               remove(node("null_arg_expr")), ConstructionWarning);
-
-  // `std::string_view sv(null_arg_expr);`
-  auto HandleStackDirectInitialization =
-      makeRule(varDecl(HasBasicStringViewType,
-                       hasInitializer(cxxConstructExpr(
-                           BasicStringViewConstructingFromNullExpr,
-                           unless(isListInitialization()))),
-                       isDirectInitialization())
-                   .bind("var_decl"),
-               changeTo(node("construct_expr"), cat(name("var_decl"))),
-               ConstructionWarning);
-
-  // `std::string_view sv{null_arg_expr};`
-  auto HandleStackDirectListInitialization =
-      makeRule(varDecl(HasBasicStringViewType,
-                       hasInitializer(cxxConstructExpr(
-                           BasicStringViewConstructingFromNullExpr,
-                           isListInitialization())),
-                       isDirectInitialization()),
-               remove(node("null_arg_expr")), ConstructionWarning);
-
-  // `struct S { std::string_view sv = null_arg_expr; };`
-  auto HandleFieldInClassCopyInitialization = makeRule(
-      fieldDecl(HasBasicStringViewType,
-                hasInClassInitializer(ignoringImpCasts(
-                    cxxConstructExpr(BasicStringViewConstructingFromNullExpr,
-                                     unless(isListInitialization()))))),
-      changeTo(node("null_arg_expr"), cat("{}")), ConstructionWarning);
-
-  // `struct S { std::string_view sv = {null_arg_expr}; };` and
-  // `struct S { std::string_view sv{null_arg_expr}; };`
-  auto HandleFieldInClassCopyListAndDirectListInitialization = makeRule(
-      fieldDecl(HasBasicStringViewType,
-                hasInClassInitializer(ignoringImpCasts(
-                    cxxConstructExpr(BasicStringViewConstructingFromNullExpr,
-                                     isListInitialization())))),
-      remove(node("null_arg_expr")), ConstructionWarning);
+  Finder->addMatcher(
+      cxxConstructExpr(HasBasicStringViewType, constructedFromNullptr())
+          .bind("construct_expr"),
+      this);
+}
 
-  // `class C { std::string_view sv; C() : sv(null_arg_expr) {} };`
-  auto HandleConstructorDirectInitialization =
-      makeRule(cxxCtorInitializer(forField(fieldDecl(HasBasicStringViewType)),
-                                  withInitializer(cxxConstructExpr(
-                                      BasicStringViewConstructingFromNullExpr,
-                                      unless(isListInitialization())))),
-               remove(node("null_arg_expr")), ConstructionWarning);
+void StringviewNullptrCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ConstructExpr =
+      Result.Nodes.getNodeAs<CXXConstructExpr>("construct_expr");
+  const Expr *NullArg = ConstructExpr->getArg(0);
 
-  // `class C { std::string_view sv; C() : sv{null_arg_expr} {} };`
-  auto HandleConstructorDirectListInitialization =
-      makeRule(cxxCtorInitializer(forField(fieldDecl(HasBasicStringViewType)),
-                                  withInitializer(cxxConstructExpr(
-                                      BasicStringViewConstructingFromNullExpr,
-                                      isListInitialization()))),
-               remove(node("null_arg_expr")), ConstructionWarning);
+  auto Diag = diag(NullArg->getBeginLoc(),
+                   "constructing basic_string_view from null is undefined");
----------------
localspook wrote:

I agree that "constructing" is a bit weird for `sv != nullptr`, but I would argue that "using a null pointer as a basic_string_view is undefined" is a bit weird for *explicit* constructions:
```cpp
std::string_view sv {nullptr}; // warning: using a null pointer as a basic_string_view is undefined
```
We could have separate diagnostic messages for the two cases, but I think that would be better as a separate PR.

https://github.com/llvm/llvm-project/pull/192889