SERVER-87848 Add const ref StringData tidy check (#19889)

GitOrigin-RevId: 14331b669a6ffdd68485c4bb1392d6d8e7c52d6f

.clang-tidy.in

@@ -55,6 +55,7 @@ Checks:          '-*,
                   mongo-unstructured-log-check,
                   mongo-volatile-check,
                   mongo-fcv-constant-check,
+                  mongo-stringdata-const-ref-check,
                   performance-faster-string-find,
                   performance-implicit-conversion-in-loop,
                   performance-inefficient-algorithm,

src/mongo/bson/util/bsoncolumn.h

@@ -526,7 +526,7 @@ public:
         _collection.push_back(_last);
     }
 
-    void append(const StringData& val) {
+    void append(StringData val) {
         _last = CMaterializer::materialize(*_allocator, val);
         _collection.push_back(_last);
     }

src/mongo/db/commands/dbcheck_command.cpp

@@ -1249,7 +1249,7 @@ Status DbChecker::_getCatalogSnapshotAndRunReverseLookup(
 bool DbChecker::_shouldEndCatalogSnapshotOrBatch(
     OperationContext* opCtx,
     const CollectionPtr& collection,
-    const StringData& indexName,
+    StringData indexName,
     const key_string::Value& keyString,
     const BSONObj& keyStringBson,
     const int64_t numKeysInSnapshot,

src/mongo/db/commands/dbcheck_command.h

@@ -212,7 +212,7 @@ private:
     bool _shouldEndCatalogSnapshotOrBatch(
         OperationContext* opCtx,
         const CollectionPtr& collection,
-        const StringData& indexName,
+        StringData indexName,
         const key_string::Value& keyString,
         const BSONObj& keyStringBson,
         int64_t numKeysInSnapshot,

src/mongo/tools/mongo_tidy_checks/MongoStringDataConstRefCheck.cpp

@@ -0,0 +1,58 @@
+/**
+ *    Copyright (C) 2024-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+
+#include "MongoStringDataConstRefCheck.h"
+
+namespace mongo::tidy {
+
+using namespace clang;
+using namespace clang::ast_matchers;
+
+void MongoStringDataConstRefCheck::registerMatchers(ast_matchers::MatchFinder* Finder) {
+    Finder->addMatcher(
+        traverse(
+            TK_IgnoreUnlessSpelledInSource,
+            parmVarDecl(hasType(qualType(references(cxxRecordDecl(hasName("mongo::StringData"))))),
+                        hasType(references(isConstQualified())))
+                .bind("constSDRef")),
+        this);
+}
+
+void MongoStringDataConstRefCheck::check(const ast_matchers::MatchFinder::MatchResult& Result) {
+    auto decl = Result.Nodes.getNodeAs<ParmVarDecl>("constSDRef");
+    if (!decl) {
+        return;
+    }
+    if (decl->getASTContext().getSourceManager().isMacroBodyExpansion(decl->getLocation())) {
+        return;
+    }
+    diag(decl->getBeginLoc(), "Prefer passing StringData by value.");
+}
+
+}  // namespace mongo::tidy

src/mongo/tools/mongo_tidy_checks/MongoStringDataConstRefCheck.h

@@ -0,0 +1,52 @@
+/**
+ *    Copyright (C) 2024-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+#pragma once
+
+#include <clang-tidy/ClangTidy.h>
+#include <clang-tidy/ClangTidyCheck.h>
+
+namespace mongo::tidy {
+
+/**
+ * MongoStringDataConstRefCheck is a custom clang-tidy check for detecting
+ * the usage of 'const StringData&' in the parameter list of functions in the source code.
+ *
+ * It extends ClangTidyCheck and overrides the registerMatchers
+ * and check functions. The registerMatchers function adds matchers
+ * to identify the usage of 'const StringData&',
+ * while the check function flags the matched occurrences.
+ */
+class MongoStringDataConstRefCheck : public clang::tidy::ClangTidyCheck {
+public:
+    using clang::tidy::ClangTidyCheck::ClangTidyCheck;
+    void registerMatchers(clang::ast_matchers::MatchFinder* Finder) override;
+    void check(const clang::ast_matchers::MatchFinder::MatchResult& Result) override;
+};
+
+}  // namespace mongo::tidy

src/mongo/tools/mongo_tidy_checks/MongoTidyModule.cpp

@@ -42,6 +42,7 @@
 #include "MongoRandCheck.h"
 #include "MongoStdAtomicCheck.h"
 #include "MongoStdOptionalCheck.h"
+#include "MongoStringDataConstRefCheck.h"
 #include "MongoTraceCheck.h"
 #include "MongoUninterruptibleLockGuardCheck.h"
 #include "MongoUnstructuredLogCheck.h"
@@ -81,6 +82,8 @@ public:
         CheckFactories.registerCheck<MongoRandCheck>("mongo-rand-check");
         CheckFactories.registerCheck<MongoPolyFillCheck>("mongo-polyfill-check");
         CheckFactories.registerCheck<MongoNoUniqueAddressCheck>("mongo-no-unique-address-check");
+        CheckFactories.registerCheck<MongoStringDataConstRefCheck>(
+            "mongo-stringdata-const-ref-check");
     }
 };
 

src/mongo/tools/mongo_tidy_checks/SConscript

@@ -142,6 +142,7 @@ mongo_custom_check = env.SharedLibrary(
         "MongoMacroDefinitionLeaksCheck.cpp",
         "MongoNoUniqueAddressCheck.cpp",
         "MongoRandCheck.cpp",
+        "MongoStringDataConstRefCheck.cpp",
     ],
     LIBDEPS_NO_INHERIT=[
         '$BUILD_DIR/third_party/shim_allocator',

src/mongo/tools/mongo_tidy_checks/tests/MongoTidyCheck_unittest.py

@@ -158,7 +158,8 @@ class MongoTidyTests(unittest.TestCase):
                 """))
         prohibited_types = ["day", "day", "month", "year", "month_day", "month", "day", "day"]
         self.expected_output = [
-            f"Illegal use of prohibited type 'std::chrono::{t}'." for t in prohibited_types]
+            f"Illegal use of prohibited type 'std::chrono::{t}'." for t in prohibited_types
+        ]
         self.run_clang_tidy()
 
     def test_MongoStdOptionalCheck(self):
@@ -375,13 +376,53 @@ class MongoTidyTests(unittest.TestCase):
                 WarningsAsErrors: '*'
                 """))
 
-        self.expected_output =[
+        self.expected_output = [
             "error: Use of rand or srand, use <random> or PseudoRandom instead. [mongo-rand-check,-warnings-as-errors]\n    srand(time(0));",
             "error: Use of rand or srand, use <random> or PseudoRandom instead. [mongo-rand-check,-warnings-as-errors]\n    int random_number = rand();",
         ]
 
         self.run_clang_tidy()
 
+    def test_MongoStringDataConstRefCheck1(self):
+        self.write_config(
+            textwrap.dedent("""\
+                Checks: '-*,mongo-stringdata-const-ref-check'
+                WarningsAsErrors: '*'
+                """))
+
+        self.expected_output = [
+            "Prefer passing StringData by value.",
+        ]
+
+        self.run_clang_tidy()
+
+    def test_MongoStringDataConstRefCheck2(self):
+        self.write_config(
+            textwrap.dedent("""\
+                Checks: '-*,mongo-stringdata-const-ref-check'
+                WarningsAsErrors: '*'
+                """))
+
+        self.expected_output = [
+            "Prefer passing StringData by value.",
+        ]
+
+        self.run_clang_tidy()
+
+    def test_MongoStringDataConstRefCheck3(self):
+        self.write_config(
+            textwrap.dedent("""\
+                Checks: '-*,mongo-stringdata-const-ref-check'
+                WarningsAsErrors: '*'
+                """))
+
+        self.expected_output = [
+            "",
+        ]
+
+        self.run_clang_tidy()
+
+
 if __name__ == '__main__':
 
     parser = argparse.ArgumentParser()

src/mongo/tools/mongo_tidy_checks/tests/SConscript

@@ -45,6 +45,9 @@ if env.GetOption('ninja') == 'disabled':
         'test_MongoRandCheck.cpp',
         'test_MongoPolyFillCheck.cpp',
         'test_MongoNoUniqueAddressCheck.cpp',
+        "test_MongoStringDataConstRefCheck1.cpp",
+        "test_MongoStringDataConstRefCheck2.cpp",
+        "test_MongoStringDataConstRefCheck3.cpp",
     ]
 
     # So that we can do fast runs, we will generate a separate compilation database file for each

src/mongo/tools/mongo_tidy_checks/tests/test_MongoStringDataConstRefCheck1.cpp

@@ -0,0 +1,7 @@
+namespace mongo {
+class StringData {};
+
+int func(const StringData& sd) {
+    return -1;
+}
+}  // namespace mongo

src/mongo/tools/mongo_tidy_checks/tests/test_MongoStringDataConstRefCheck2.cpp

@@ -0,0 +1,7 @@
+namespace mongo {
+class StringData {};
+
+int func(const StringData&& sd) {
+    return -1;
+}
+}  // namespace mongo

src/mongo/tools/mongo_tidy_checks/tests/test_MongoStringDataConstRefCheck3.cpp

@@ -0,0 +1,75 @@
+namespace mongo {
+class StringData {};
+
+int func0(StringData sd) {
+    return 0;
+}
+
+int func1(StringData& sd) {
+    return 0;
+}
+
+int func2(StringData&& sd) {
+    return 0;
+}
+
+int func3(const StringData* sd) {
+    return 0;
+}
+}  // namespace mongo
+
+namespace testNS {
+class StringData;
+
+int func4(const testNS::StringData& sd) {
+    return 0;
+}
+
+int func5(testNS::StringData& sd) {
+    return 0;
+}
+
+}  // namespace testNS
+
+
+template <typename T>
+int func6(const T& sd) {
+    return 0;
+}
+
+template <typename T>
+int func7(const T&& sd) {
+    return 0;
+}
+
+template <typename T>
+int func8(T& sd) {
+    return 0;
+}
+
+template <typename T>
+int func9(T&& sd) {
+    return 0;
+}
+
+#define SD_FUNCDEF(T)         \
+    int func10(const T& sd) { \
+        return 0;             \
+    }
+
+namespace mongo {
+SD_FUNCDEF(StringData);
+
+int TestSDCheckMain() {
+    StringData sd;
+    func6(sd);
+    func7(StringData{});
+    func8(sd);
+    func9(StringData{});
+
+    const StringData& sdRef = sd;
+    func6(sdRef);
+
+    return 0;
+}
+}  // namespace mongo