tidy: add clang-tidy

.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: "*"

.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 clang-tidy-ci
+
+      - 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

Makefile

@@ -103,6 +103,16 @@ lcov:
 	cd $(ROOT_DIR)/build/release/test && \
 	ctest --output-on-failure -j ${TEST_JOBS}
 
+clangd:
+	$(call mkdirp,build/clangd) && cd build/clangd && \
+	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 ../..
+
+clang-tidy-ci:
+	# For CI: export compile commands for the built items.
+	$(call mkdirp,build/release) && cd build/release && \
+	cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ../..
+
 pytest:
 	$(MAKE) release
 	cd $(ROOT_DIR)/tools/python_api/test && \

dataset/invalid-utf8/invalid-utf8.csv

@@ -0,0 +1 @@
+ÿ

src/common/arrow/arrow_converter.cpp

@@ -44,7 +44,7 @@ void ArrowConverter::setArrowFormatForStruct(
     ArrowSchemaHolder& rootHolder, ArrowSchema& child, const main::DataTypeInfo& typeInfo) {
     auto& childrenTypesInfo = typeInfo.childrenTypesInfo;
     child.format = "+s";
-    child.name = typeInfo.name.c_str();
+    // name is set by parent.
     child.n_children = (std::int64_t)childrenTypesInfo.size();
     rootHolder.nestedChildren.emplace_back();
     rootHolder.nestedChildren.back().resize(child.n_children);

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])) {

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])) {

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) {

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 {
@@ -13,6 +17,12 @@ uint32_t BaseLowerUpperFunction::getResultLen(char* inputStr, uint32_t inputLen,
         if (inputStr[i] & 0x80) {
             int size = 0;
             int codepoint = utf8proc_codepoint(inputStr + i, size);
+            if (codepoint < 0) {
+                // TODO(Xiyang): We shouldn't allow invalid UTF-8 to enter a string column.
+                std::string funcName = isUpper ? "UPPER" : "LOWER";
+                throw RuntimeException(common::StringUtils::string_format(
+                    "Failed calling {}: Invalid UTF-8.", funcName));
+            }
             int convertedCodepoint =
                 isUpper ? utf8proc_toupper(codepoint) : utf8proc_tolower(codepoint);
             int newSize = utf8proc_codepoint_length(convertedCodepoint);
@@ -32,10 +42,10 @@ 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);
+            assert(codepoint >= 0); // Validity ensured by getResultLen.
             int convertedCodepoint =
                 toUpper ? utf8proc_toupper(codepoint) : utf8proc_tolower(codepoint);
-            auto success = utf8proc_codepoint_to_utf8(convertedCodepoint, newSize, result);
-            assert(success);
+            utf8proc_codepoint_to_utf8(convertedCodepoint, newSize, result);
             result += newSize;
             i += size;
         } else {

src/include/common/system_message.h

@@ -0,0 +1,22 @@
+#pragma once
+
+#include <cerrno>
+#include <string>
+#include <system_error>
+
+namespace kuzu {
+namespace common {
+inline std::string systemErrMessage(int code) {
+    // System errors are unexpected. For anything expected, we should catch it explicitly and
+    // provide a better error message to the user.
+    // LCOV_EXCL_START
+    return std::system_category().message(code);
+    // LCOV_EXCL_END
+}
+inline std::string posixErrMessage() {
+    // LCOV_EXCL_START
+    return systemErrMessage(errno);
+    // LCOV_EXCL_END
+}
+} // namespace common
+} // namespace kuzu

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;

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.
     }
 };
 

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;

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();
 

src/main/plan_printer.cpp

@@ -150,6 +150,10 @@ void OpProfileTree::printOpProfileBoxUpperFrame(uint32_t rowIdx, std::ostringstr
     oss << std::endl;
 }
 
+static std::string dashedLineAccountingForIndex(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 = dashedLineAccountingForIndex(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 = dashedLineAccountingForIndex(opProfileBoxWidth, INDENT_WIDTH);
                 } else {
                     textToPrint = opProfileBox->getAttribute((i - numParams - 1) / 2 - 1);
                 }

src/main/query_result.cpp

@@ -91,7 +91,7 @@
             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,9 +104,9 @@
             columnTypeInfo->childrenTypesInfo.push_back(
                 DataTypeInfo::getInfoForDataType(LogicalType(LogicalTypeID::INTERNAL_ID), "_dst"));
             auto numProperties = RelVal::getNumProperties(value);
-            for (auto j = 0u; i < numProperties; j++) {
-                auto name = NodeVal::getPropertyName(value, j);
-                auto val = NodeVal::getPropertyVal(value, j);
+            for (auto j = 0u; j < numProperties; j++) {
+                auto name = RelVal::getPropertyName(value, j);
+                auto val = RelVal::getPropertyVal(value, j);
                 columnTypeInfo->childrenTypesInfo.push_back(
                     DataTypeInfo::getInfoForDataType(*val->getDataType(), name));
             }

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);

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);