Filter string_view from the nullptr diagnosis of bugprone-string-constructor to prevent duplicate warnings with bugprone-stringview-nullptr

[CJ-Johnson]

Summary

Updates the check and tests to not diagnose the null case for string_view (but retains it for string). This prevents the check from giving duplicate warnings that are caught by bugprone-stringview-nullptr (D113148).

[ymandel]

CJ -- please mention "bugprone-stringview-nullptr" explicitly and explain that it will cover those features, etc. so that the description will be clear on its own. Thanks!

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

181–182

Slightly prefer an early return here:
Filter out basic_string_view to avoid conflicts with
bugprone-stringview-nullptr
if (Result.Nodes.getNodeAs<CXXRecordDecl>("basic_string_view_decl") != nullptr)

return;

[CJ-Johnson]

Switch to early return for the filter

[njames93]

I'm in 2 minds about this. This diagnostic is a good fit for this warning and shouldn't be removed. Likewise duplicate diagnostics from different checks is annoying. Maybe a better path forward would be to suppress the diagnostic if the bugprone-stringview-nullptr check is enabled. These kinds of intercheck dependencies are found in modernize-prefer-member-init check IIRC. You can just copy the impl from there.

[CJ-Johnson]

Only filter out basic_string_view when bugprone-stringview-nullptr is enabled

[CJ-Johnson]

In D114823#3229990, @njames93 wrote:

I'm in 2 minds about this. This diagnostic is a good fit for this warning and shouldn't be removed. Likewise duplicate diagnostics from different checks is annoying. Maybe a better path forward would be to suppress the diagnostic if the bugprone-stringview-nullptr check is enabled. These kinds of intercheck dependencies are found in modernize-prefer-member-init check IIRC. You can just copy the impl from there.

Great suggestion! I've updated this diff to do exactly that. Thanks! :)

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.h

public:
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
+  const bool IsStringviewNullptrCheckEnabled;
   const bool WarnOnLargeLength;
   const unsigned int LargeLengthThreshold;
   std::vector<std::string> StringNames;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRING_CONSTRUCTOR_H

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

removeNamespaces(const std::vector<std::string> &Names) {
   return Result;
 }
 
 } // namespace
 
 StringConstructorCheck::StringConstructorCheck(StringRef Name,
                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
+      IsStringviewNullptrCheckEnabled(
+          Context->isCheckEnabled("bugprone-stringview-nullptr")),
       WarnOnLargeLength(Options.get("WarnOnLargeLength", true)),
       LargeLengthThreshold(Options.get("LargeLengthThreshold", 0x800000)),
       StringNames(utils::options::parseStringList(
           Options.get("StringNames", DefaultStringNames))) {}
 
 void StringConstructorCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnLargeLength", WarnOnLargeLength);
   Options.store(Opts, "LargeLengthThreshold", LargeLengthThreshold);
Finder->addMatcher(
                     hasArgument(1, ignoringParenImpCasts(
                                        integerLiteral().bind("int"))))))
           .bind("constructor"),
       this);
 
   // Check the literal string constructor with char pointer.
   // [i.e. string (const char* s);]
   Finder->addMatcher(
-      traverse(TK_AsIs,
-               cxxConstructExpr(
-                   hasDeclaration(cxxConstructorDecl(ofClass(cxxRecordDecl(
-                       hasAnyName(removeNamespaces(StringNames)))))),
-                   hasArgument(0, expr().bind("from-ptr")),
-                   // do not match std::string(ptr, int)
-                   // match std::string(ptr, alloc)
-                   // match std::string(ptr)
-                   anyOf(hasArgument(1, unless(hasType(isInteger()))),
-                         argumentCountIs(1)))
-                   .bind("constructor")),
+      traverse(
+          TK_AsIs,
+              hasDeclaration(cxxConstructorDecl(ofClass(anyOf(
+                  cxxRecordDecl(hasName("basic_string_view"))
+                      .bind("basic_string_view_decl"),
+                  cxxRecordDecl(hasAnyName(removeNamespaces(StringNames))))))),
       this);
 }
 
 void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
   const ASTContext &Ctx = *Result.Context;
   const auto *E = Result.Nodes.getNodeAs<CXXConstructExpr>("constructor");
   assert(E && "missing constructor expression");
   SourceLocation Loc = E->getBeginLoc();
if (Lit->getValue().ugt(Str->getLength())) {
       diag(Loc, "length is bigger than string literal size");
     }
   } else if (const auto *Ptr = Result.Nodes.getNodeAs<Expr>("from-ptr")) {
     Expr::EvalResult ConstPtr;
     if (!Ptr->isInstantiationDependent() &&
         Ptr->EvaluateAsRValue(ConstPtr, Ctx) &&
         ((ConstPtr.Val.isInt() && ConstPtr.Val.getInt().isZero()) ||
          (ConstPtr.Val.isLValue() && ConstPtr.Val.isNullPointer()))) {
+      if (IsStringviewNullptrCheckEnabled &&
+          Result.Nodes.getNodeAs<CXXRecordDecl>("basic_string_view_decl")) {
+        // Filter out `basic_string_view` to avoid conflicts with
+        // `bugprone-stringview-nullptr`
+        return;
+      }
       diag(Loc, "constructing string from nullptr is undefined behaviour");
     }
   }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang