tidy: add clang-tidy

clang-tidy is a static analyzer, and can find many different issues. So far I have integrated a few simple checks, and disabled the remaining ones. They will be enabled in future PRs. Closes #2152. Ref #2148.
kuzudb · Oct 6, 2023 · bf7790d · bf7790d
1 parent 3cf3539
commit bf7790d
Show file tree

Hide file tree

Showing 30 changed files with 141 additions and 92 deletions.
diff --git a/.clang-tidy b/.clang-tidy
@@ -0,0 +1,16 @@
+# Bugprone exception escape is super unhelpful.
+Checks:
+  -*,
+
+  bugprone-*,
+  -bugprone-easily-swappable-parameters,
+  -bugprone-exception-escape,
+  -bugprone-forward-declaration-namespace,
+  -bugprone-implicit-widening-of-multiplication-result,
+  -bugprone-narrowing-conversions,
+  -bugprone-signed-char-misuse,
+
+  clang-analyzer-deadcode.DeadStores,
+  concurrency-*,
+HeaderFilterRegex: src/include|tools/benchmark/include|tools/nodejs_api/src_cpp/include|tools/python_api/src_cpp/include|tools/rust_api/include|tools/shell/include
+WarningsAsErrors: "*"
diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml
@@ -20,7 +20,7 @@ jobs:
         run: |
           pip install torch~=2.0.0 --extra-index-url https://download.pytorch.org/whl/cpu &&\
           pip install --user -r tools/python_api/requirements_dev.txt -f https://data.pyg.org/whl/torch-2.0.0+cpu.html
-      
+
       - name: Ensure Node.js dependencies
         run: npm install --include=dev
         working-directory: tools/nodejs_api
@@ -30,7 +30,7 @@ jobs:
 
       - name: Python test
         run: CC=gcc CXX=g++ make pytest NUM_THREADS=32
-      
+
       - name: Node.js test
         run: CC=gcc CXX=g++ make nodejstest NUM_THREADS=32
 
@@ -117,7 +117,7 @@ jobs:
         shell: bash
 
   clang-build-test:
-    name: clang build & test
+    name: clang build, test & tidy
     needs: [clang-formatting-check, header-include-guard-check]
     runs-on: kuzu-self-hosted-testing
     steps:
@@ -140,13 +140,26 @@ jobs:
 
       - name: Python test
         run: CC=clang-14 CXX=clang++-14 make pytest NUM_THREADS=32
-      
+
       - name: Node.js test
         run: CC=clang-14 CXX=clang++-14 make nodejstest NUM_THREADS=32
-      
+
       - name: Java test
         run: CC=clang-14 CXX=clang++-14 make javatest NUM_THREADS=32
 
+      - name: Create compilation commands database
+        run: make clangd NUM_THREADS=32
+
+      - name: Run clang-tidy on src
+        run: run-clang-tidy -p build/release -quiet -j 32 "^$(realpath src)"
+
+      - name: Run clang-tidy on tools
+        run: run-clang-tidy -p build/release -quiet -j 32 "^$(realpath tools)/(?!shell/linenoise.cpp)"
+
+      - name: Run clang-tidy on examples
+        run: run-clang-tidy -p build/release -quiet -j 32 "^$(realpath examples)"
+
+
   msvc-build-test:
     name: msvc build & test
     needs: [clang-formatting-check, header-include-guard-check]
@@ -226,12 +239,12 @@ jobs:
     runs-on: kuzu-self-hosted-benchmarking
     steps:
       - uses: actions/checkout@v3
-      
+
       - name: Ensure Python dependencies
         run: |
           pip install torch~=1.13 --extra-index-url https://download.pytorch.org/whl/cpu &&\
           pip install --user -r tools/python_api/requirements_dev.txt -f https://data.pyg.org/whl/torch-1.13.0+cpu.html
-      
+
       - name: Ensure Node.js dependencies
         run: npm install
         working-directory: tools/nodejs_api

diff --git a/Makefile b/Makefile
@@ -103,6 +103,11 @@ lcov:
 	cd $(ROOT_DIR)/build/release/test && \
 	ctest --output-on-failure -j ${TEST_JOBS}
 
+clangd:
+	$(call mkdirp,build/release) && cd build/release && \
+	cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DCMAKE_BUILD_TYPE=Release -DBUILD_EXAMPLES=TRUE \
+		-DBUILD_BENCHMARK=TRUE -DBUILD_JAVA=TRUE -DBUILD_NODEJS=TRUE -DBUILD_TESTS=TRUE ../..
+
 pytest:
 	$(MAKE) release
 	cd $(ROOT_DIR)/tools/python_api/test && \

diff --git a/src/common/types/date_t.cpp b/src/common/types/date_t.cpp
@@ -274,7 +274,7 @@ bool Date::tryConvertDate(const char* buf, uint64_t len, uint64_t& pos, date_t&
     int32_t day = 0;
     int32_t month = -1;
     int32_t year = 0;
-    int sep;
+    char sep;
 
     // skip leading spaces
     while (pos < len && StringUtils::CharacterIsSpace(buf[pos])) {

diff --git a/src/common/types/dtime_t.cpp b/src/common/types/dtime_t.cpp
@@ -62,7 +62,7 @@ bool Time::TryConvertTime(const char* buf, uint64_t len, uint64_t& pos, dtime_t&
         return false;
     }
 
-    int sep;
+    char sep;
 
     // skip leading spaces
     while (pos < len && isspace(buf[pos])) {

diff --git a/src/common/types/types.cpp b/src/common/types/types.cpp
@@ -448,12 +448,10 @@ LogicalTypeID LogicalTypeUtils::dataTypeIDFromString(const std::string& dataType
         return LogicalTypeID::INTERNAL_ID;
     } else if ("INT64" == upperDataTypeIDString) {
         return LogicalTypeID::INT64;
-    } else if ("INT32" == upperDataTypeIDString) {
+    } else if ("INT32" == upperDataTypeIDString || "INT" == upperDataTypeIDString) {
         return LogicalTypeID::INT32;
     } else if ("INT16" == upperDataTypeIDString) {
         return LogicalTypeID::INT16;
-    } else if ("INT" == upperDataTypeIDString) {
-        return LogicalTypeID::INT32;
     } else if ("INT8" == upperDataTypeIDString) {
         return LogicalTypeID::INT8;
     } else if ("UINT64" == upperDataTypeIDString) {

diff --git a/src/function/base_lower_upper_operation.cpp b/src/function/base_lower_upper_operation.cpp
@@ -1,5 +1,9 @@
+#include "common/assert.h"
+#include "common/exception/runtime.h"
+#include "common/string_utils.h"
 #include "function/string/functions/base_lower_upper_function.h"
 
+using namespace kuzu::common;
 using namespace kuzu::utf8proc;
 
 namespace kuzu {
@@ -32,10 +36,16 @@ void BaseLowerUpperFunction::convertCase(char* result, uint32_t len, char* input
         if (input[i] & 0x80) {
             int size = 0, newSize = 0;
             int codepoint = utf8proc_codepoint(input + i, size);
+            if (codepoint < 0) {
+                // TODO(Xiyang): We shouldn't allow invalid UTF-8 to enter a string column.
+                std::string funcName = toUpper ? "UPPER" : "LOWER";
+                throw RuntimeException(common::StringUtils::string_format(
+                    "Failed calling {}: Invalid UTF-8", funcName));
+            }
             int convertedCodepoint =
                 toUpper ? utf8proc_toupper(codepoint) : utf8proc_tolower(codepoint);
-            auto success = utf8proc_codepoint_to_utf8(convertedCodepoint, newSize, result);
-            assert(success);
+            bool success = utf8proc_codepoint_to_utf8(convertedCodepoint, newSize, result);
+            KU_ASSERT(success);
             result += newSize;
             i += size;
         } else {

diff --git a/src/include/common/system_message.h b/src/include/common/system_message.h
@@ -0,0 +1,16 @@
+#pragma once
+
+#include <cerrno>
+#include <string>
+#include <system_error>
+
+namespace kuzu {
+namespace common {
+inline std::string system_err_message(int code) {
+    return std::system_category().message(code);
+}
+inline std::string posix_err_message() {
+    return system_err_message(errno);
+}
+} // namespace common
+} // namespace kuzu
diff --git a/src/include/common/types/cast_helpers.h b/src/include/common/types/cast_helpers.h
@@ -122,7 +122,7 @@ struct DateToStringCast {
         }
         // optionally add BC to the end of the date
         if (add_bc) {
-            memcpy(ptr, " (BC)", 5);
+            strcpy(ptr, " (BC)");
         }
     }
 };
@@ -282,7 +282,7 @@ struct IntervalToStringCast {
             }
         } else if (length == 0) {
             // empty interval: default to 00:00:00
-            memcpy(buffer, "00:00:00", 8);
+            strcpy(buffer, "00:00:00");
             return 8;
         }
         return length;

diff --git a/src/include/function/arithmetic/arithmetic_functions.h b/src/include/function/arithmetic/arithmetic_functions.h
@@ -206,7 +206,8 @@ struct Gamma {
 struct Lgamma {
     template<class T>
     static inline void operation(T& input, double_t& result) {
-        result = lgamma(input);
+        result =
+            lgamma(input); // NOLINT(concurrency-mt-unsafe): We don't use the thread-unsafe signgam.
     }
 };
 

diff --git a/src/include/function/boolean/boolean_functions.h b/src/include/function/boolean/boolean_functions.h
@@ -38,9 +38,7 @@ const uint8_t NULL_BOOL = 2;
 struct And {
     static inline void operation(
         bool left, bool right, uint8_t& result, bool isLeftNull, bool isRightNull) {
-        if (!left && !isLeftNull) {
-            result = false;
-        } else if (!right && !isRightNull) {
+        if ((!left && !isLeftNull) || (!right && !isRightNull)) {
             result = false;
         } else if (isLeftNull || isRightNull) {
             result = NULL_BOOL;
@@ -68,9 +66,7 @@ struct And {
 struct Or {
     static inline void operation(
         bool left, bool right, uint8_t& result, bool isLeftNull, bool isRightNull) {
-        if (left && !isLeftNull) {
-            result = true;
-        } else if (right && !isRightNull) {
+        if ((left && !isLeftNull) || (right && !isRightNull)) {
             result = true;
         } else if (isLeftNull || isRightNull) {
             result = NULL_BOOL;

diff --git a/src/include/processor/operator/scan_node_id.h b/src/include/processor/operator/scan_node_id.h
@@ -43,13 +43,7 @@ class ScanNodeIDSharedState {
     inline uint32_t getNumTableStates() const { return tableStates.size(); }
     inline NodeTableScanState* getTableState(uint32_t idx) const { return tableStates[idx].get(); }
 
-    inline void initialize(transaction::Transaction* transaction) {
-        auto numMask = tableStates[0]->getSemiMask()->getNumMasks();
-        for (auto& tableState : tableStates) {
-            assert(tableState->getSemiMask()->getNumMasks() == numMask);
-            tableState->initializeMaxOffset(transaction);
-        }
-    }
+    void initialize(transaction::Transaction* transaction);
 
     std::tuple<NodeTableScanState*, common::offset_t, common::offset_t> getNextRangeToRead();
 

diff --git a/src/main/plan_printer.cpp b/src/main/plan_printer.cpp
@@ -150,6 +150,10 @@ void OpProfileTree::printOpProfileBoxUpperFrame(uint32_t rowIdx, std::ostringstr
     oss << std::endl;
 }
 
+static std::string dashed_line_account_for_index(uint32_t width, uint32_t indent) {
+    return std::string(width - (1 + indent) * 2, '-');
+}
+
 void OpProfileTree::printOpProfileBoxes(uint32_t rowIdx, std::ostringstream& oss) const {
     auto height = calculateRowHeight(rowIdx);
     auto halfWayPoint = height / 2;
@@ -164,12 +168,13 @@ void OpProfileTree::printOpProfileBoxes(uint32_t rowIdx, std::ostringstream& oss
                 unsigned int numParams = opProfileBox->getNumParams();
                 if (i == 0) {
                     textToPrint = opProfileBox->getOpName();
-                } else if (i == 1) {
-                    textToPrint = std::string(opProfileBoxWidth - (1 + INDENT_WIDTH) * 2, '-');
+                } else if (i == 1) { // NOLINT(bugprone-branch-clone): Merging these branches is a
+                                     // logical error, and this conditional chain is pleasant.
+                    textToPrint = dashed_line_account_for_index(opProfileBoxWidth, INDENT_WIDTH);
                 } else if (i <= numParams + 1) {
                     textToPrint = opProfileBox->getParamsName(i - 2);
                 } else if ((i - numParams - 1) % 2) {
-                    textToPrint = std::string(opProfileBoxWidth - (1 + INDENT_WIDTH) * 2, '-');
+                    textToPrint = dashed_line_account_for_index(opProfileBoxWidth, INDENT_WIDTH);
                 } else {
                     textToPrint = opProfileBox->getAttribute((i - numParams - 1) / 2 - 1);
                 }

diff --git a/src/main/query_result.cpp b/src/main/query_result.cpp
@@ -91,7 +91,7 @@ std::vector<std::unique_ptr<DataTypeInfo>> QueryResult::getColumnTypesInfo() con
             columnTypeInfo->childrenTypesInfo.push_back(
                 DataTypeInfo::getInfoForDataType(LogicalType(LogicalTypeID::STRING), "_label"));
             auto numProperties = NodeVal::getNumProperties(value);
-            for (auto j = 0u; i < numProperties; j++) {
+            for (auto j = 0u; j < numProperties; j++) {
                 auto name = NodeVal::getPropertyName(value, j);
                 auto val = NodeVal::getPropertyVal(value, j);
                 columnTypeInfo->childrenTypesInfo.push_back(
@@ -104,7 +104,7 @@ std::vector<std::unique_ptr<DataTypeInfo>> QueryResult::getColumnTypesInfo() con
             columnTypeInfo->childrenTypesInfo.push_back(
                 DataTypeInfo::getInfoForDataType(LogicalType(LogicalTypeID::INTERNAL_ID), "_dst"));
             auto numProperties = RelVal::getNumProperties(value);
-            for (auto j = 0u; i < numProperties; j++) {
+            for (auto j = 0u; j < numProperties; j++) {
                 auto name = NodeVal::getPropertyName(value, j);
                 auto val = NodeVal::getPropertyVal(value, j);
                 columnTypeInfo->childrenTypesInfo.push_back(

diff --git a/src/optimizer/agg_key_dependency_optimizer.cpp b/src/optimizer/agg_key_dependency_optimizer.cpp
@@ -67,16 +67,17 @@ AggKeyDependencyOptimizer::resolveKeysAndDependentKeys(const binder::expression_
     for (auto& expression : keys) {
         if (expression->expressionType == PROPERTY) {
             auto propertyExpression = (binder::PropertyExpression*)expression.get();
-            if (propertyExpression->isPrimaryKey() || propertyExpression->isInternalID()) {
+            if (propertyExpression->isPrimaryKey() ||
+                propertyExpression->isInternalID()) { // NOLINT(bugprone-branch-clone): Collapsing
+                                                      // is a logical error.
                 groupExpressions.push_back(expression);
             } else if (primaryVarNames.contains(propertyExpression->getVariableName())) {
                 dependentExpressions.push_back(expression);
             } else {
                 groupExpressions.push_back(expression);
             }
-        } else if (ExpressionUtil::isNodeVariable(*expression)) {
-            dependentExpressions.push_back(expression);
-        } else if (ExpressionUtil::isRelVariable(*expression)) {
+        } else if (ExpressionUtil::isNodeVariable(*expression) ||
+                   ExpressionUtil::isRelVariable(*expression)) {
             dependentExpressions.push_back(expression);
         } else {
             groupExpressions.push_back(expression);

diff --git a/src/processor/operator/persistent/copy_rel_columns.cpp b/src/processor/operator/persistent/copy_rel_columns.cpp
@@ -26,6 +26,7 @@ void CopyRelColumns::executeInternal(ExecutionContext* context) {
             numBwdRows = countRelLists(sharedState->bwdRelData.get(), info.nbrOffsetPos);
         }
         assert(numFwdRows == numBwdRows);
+        (void)numBwdRows; // For clang-tidy, value calculated for debug assert.
         numRows += numFwdRows;
     }
     sharedState->incrementNumRows(numRows);

diff --git a/src/processor/operator/persistent/reader/csv/base_csv_reader.cpp b/src/processor/operator/persistent/reader/csv/base_csv_reader.cpp
@@ -5,6 +5,7 @@
 #include "common/data_chunk/data_chunk.h"
 #include "common/exception/copy.h"
 #include "common/string_utils.h"
+#include "common/system_message.h"
 #include "processor/operator/persistent/reader/csv/driver.h"
 
 using namespace kuzu::common;
@@ -23,8 +24,8 @@ BaseCSVReader::BaseCSVReader(const std::string& filePath, const common::ReaderCo
 #endif
     );
     if (fd == -1) {
-        throw CopyException(
-            StringUtils::string_format("Could not open file {}: {}", filePath, strerror(errno)));
+        throw CopyException(StringUtils::string_format(
+            "Could not open file {}: {}", filePath, posix_err_message()));
     }
 }
 
@@ -196,7 +197,7 @@ bool BaseCSVReader::readBuffer(uint64_t* start) {
     uint64_t readCount = read(fd, buffer.get() + remaining, bufferReadSize);
     if (readCount == -1) {
         throw CopyException(StringUtils::string_format(
-            "Could not read from file {}: {}", filePath, strerror(errno)));
+            "Could not read from file {}: {}", filePath, posix_err_message()));
     }
 
     bufferSize = remaining + readCount;
@@ -414,7 +415,7 @@ uint64_t BaseCSVReader::getFileOffset() const {
     if (offset == -1) {
         // LCOV_EXCL_START
         throw CopyException(StringUtils::string_format(
-            "Could not get current file position for file {}: {}", filePath, strerror(errno)));
+            "Could not get current file position for file {}: {}", filePath, posix_err_message()));
         // LCOV_EXCL_END
     }
     assert(offset >= bufferSize);
@@ -429,7 +430,7 @@ uint64_t BaseCSVReader::getLineNumber() {
     if (lseek(fd, 0, SEEK_SET) == -1) {
         // LCOV_EXCL_START
         throw CopyException(StringUtils::string_format(
-            "Could not seek to beginning of file {}: {}", filePath, strerror(errno)));
+            "Could not seek to beginning of file {}: {}", filePath, posix_err_message()));
         // LCOV_EXCL_END
     }
 
@@ -440,16 +441,13 @@ uint64_t BaseCSVReader::getLineNumber() {
         if (bytesRead == -1) {
             // LCOV_EXCL_START
             throw CopyException(StringUtils::string_format(
-                "Could not read from file {}: {}", filePath, strerror(errno)));
+                "Could not read from file {}: {}", filePath, posix_err_message()));
             // LCOV_EXCL_END
         }
         totalBytes += bytesRead;
 
         for (uint64_t i = 0; i < bytesRead; i++) {
-            if (buf[i] == '\n') {
-                lineNumber++;
-                carriageReturn = false;
-            } else if (carriageReturn) {
+            if (buf[i] == '\n' || carriageReturn) {
                 lineNumber++;
                 carriageReturn = false;
             }