Add new features to slang-tidy (#777)

This PR adds new features to slang-tidy:
* slang-tidy can read its configuration from a .slang-tidy file, closely similar to what .clang-tidy offers.
* Adds new checks to the linter.
* Small refactor in the code of the checks.
* Add option to skip linting checks from specific files.

include/slang/ast/expressions/AssignmentExpressions.h

@@ -29,6 +29,7 @@ class SLANG_EXPORT AssignmentExpression : public Expression {
 
     bool isCompound() const { return op.has_value(); }
     bool isNonBlocking() const { return nonBlocking; }
+    bool isBlocking() const { return !nonBlocking; }
     bool isLValueArg() const;
 
     const Expression& left() const { return *left_; }

include/slang/ast/symbols/ValueSymbol.h

@@ -150,6 +150,9 @@ class SLANG_EXPORT ValueDriver {
     /// Indicates whether the driver is inside an always_ff block.
     bool isInAlwaysFFBlock() const;
 
+    /// Indicates whether the driver is inside an always_latch block.
+    bool isInAlwaysLatchBlock() const;
+
     /// Gets the source range describing the driver as written in the source code.
     SourceRange getSourceRange() const;
 

include/slang/util/CppTypePrinter.h

@@ -0,0 +1,62 @@
+//------------------------------------------------------------------------------
+//! @file TypePrinter.h
+//! @brief Utility function that let's you print the type of a variable
+//
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+//------------------------------------------------------------------------------
+#pragma once
+
+// Courtesy of:
+// https://stackoverflow.com/questions/81870/is-it-possible-to-print-a-variables-type-in-standard-c/64490578#64490578
+
+#include <string_view>
+
+namespace slang {
+
+/// Allows you to print the demangled name of a C++ type.
+template<typename T>
+constexpr std::string_view typeName();
+
+template<>
+constexpr std::string_view typeName<void>() {
+    return "void";
+}
+
+namespace type_printer_inner {
+
+using type_name_prober = void;
+
+template<typename T>
+constexpr std::string_view wrappedTypeName() {
+#ifdef __clang__
+    return __PRETTY_FUNCTION__;
+#elif defined(__GNUC__)
+    return __PRETTY_FUNCTION__;
+#elif defined(_MSC_VER)
+    return __FUNCSIG__;
+#else
+#    error "Unsupported compiler"
+#endif
+}
+
+constexpr std::size_t wrappedTypeNamePrefixLength() {
+    return wrappedTypeName<type_name_prober>().find(typeName<type_name_prober>());
+}
+
+constexpr std::size_t wrappedTypeNameSuffixLength() {
+    return wrappedTypeName<type_name_prober>().length() - wrappedTypeNamePrefixLength() -
+           typeName<type_name_prober>().length();
+}
+
+} // namespace type_printer_inner
+
+template<typename T>
+constexpr std::string_view typeName() {
+    constexpr auto wrapped_name = type_printer_inner::wrappedTypeName<T>();
+    constexpr auto prefix_length = type_printer_inner::wrappedTypeNamePrefixLength();
+    constexpr auto suffix_length = type_printer_inner::wrappedTypeNameSuffixLength();
+    constexpr auto type_name_length = wrapped_name.length() - prefix_length - suffix_length;
+    return wrapped_name.substr(prefix_length, type_name_length);
+}
+} // namespace slang

source/ast/symbols/ValueSymbol.cpp

@@ -404,6 +404,12 @@ bool ValueDriver::isInAlwaysFFBlock() const {
                ProceduralBlockKind::AlwaysFF;
 }
 
+bool ValueDriver::isInAlwaysLatchBlock() const {
+    return containingSymbol->kind == SymbolKind::ProceduralBlock &&
+           containingSymbol->as<ProceduralBlockSymbol>().procedureKind ==
+               ProceduralBlockKind::AlwaysLatch;
+}
+
 SourceRange ValueDriver::getSourceRange() const {
     if (procCallExpression)
         return procCallExpression->sourceRange;

tools/tidy/CMakeLists.txt

@@ -2,12 +2,27 @@
 # SPDX-FileCopyrightText: Michael Popoloski
 # SPDX-License-Identifier: MIT
 # ~~~
-add_executable(slang_tidy tidy.cpp synthesis/OnlyAssignedOnReset.cpp
-                          synthesis/RegisterHasNoReset.cpp)
+
+add_library(
+  slang_tidy_obj_lib OBJECT
+  src/TidyConfig.cpp
+  src/TidyConfigParser.cpp
+  src/ASTHelperVisitors.cpp
+  src/synthesis/OnlyAssignedOnReset.cpp
+  src/synthesis/RegisterHasNoReset.cpp
+  src/style/EnforcePortSuffix.cpp
+  src/synthesis/NoLatchesOnDesign.cpp
+  src/style/NoOldAlwaysSyntax.cpp
+  src/style/AlwaysCombNonBlocking.cpp
+  src/style/AlwaysFFBlocking.cpp)
+
+target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include)
+target_link_libraries(slang_tidy_obj_lib PUBLIC slang::slang fmt::fmt)
+
+add_executable(slang_tidy src/tidy.cpp)
 add_executable(slang::tidy ALIAS slang_tidy)
 
-target_link_libraries(slang_tidy PRIVATE slang::slang fmt::fmt)
-target_include_directories(slang_tidy PRIVATE include ../../include)
+target_link_libraries(slang_tidy PRIVATE slang_tidy_obj_lib)
 set_target_properties(slang_tidy PROPERTIES OUTPUT_NAME "slang-tidy")
 
 if(SLANG_INCLUDE_INSTALL)

tools/tidy/README.md

@@ -0,0 +1,51 @@
+# Slang Tidy
+
+A SystemVerilog linter
+
+## Configuration File
+
+Slang Tidy can be configured using a `.slang-tidy`. This file can be provided using the `--config-file` argument or the
+`slang-tidy` tool will search for it from the path where it has been called until the root directory.
+
+### Configuration file grammar
+
+The grammar of the config file is the following
+
+```
+config                ::= section+
+section               ::= checks_config_section | checks_section
+checks_section        ::= "Checks:" [EOL] rule+
+rule                  ::= ["-"] ((check_or_all ",")* | check_or_all) END
+check_or_all          ::= "*" | check_group "-" check_name_or_all
+check_group           ::= identifier
+check_name_or_all     ::= "*" | identifier
+checks_config_section ::= "CheckConfigs:" [EOL] config+
+config                ::= ((config_tuple ",")* | config_tuple) END
+config_tuple          ::= config_name ":" config_value
+config_name           ::= identifier
+config_value          ::= identifier
+identifier            ::= {A-Za-z}+
+EOL                   ::= '\n' | '\r' | '\r\n'
+END                   ::= EOL* | EOF
+```
+
+### Configuration file example
+
+```
+Checks:
+    -synthesis-only-assigned-on-reset,
+    -ports-enforce-port-suffix
+CheckConfigs:
+    clkName: clk,
+    resetIsActiveHigh: false,
+    inputPortSuffix: _k,
+    inputPortSuffix: _p
+```
+
+## How to add a new check
+  1. Create a new `cpp` file with the name of the check in CamelCase format inside the check kind folder.
+  2. Inside the new `cpp` file create a class that inherits from `TidyChecks`. Use the `check` function to implement
+     the code that will perform the check in the AST.
+  3. Use the `REGISTER` macro to register the new check in the factory.
+  4. Create the new tidy diagnostic in the `TidyDiags.h` file.
+  5. Add the new check to the corresponding map in the `TidyConfig` constructor.

tools/tidy/include/ASTHelperVisitors.h

@@ -1,14 +1,42 @@
 //------------------------------------------------------------------------------
 //! @file ASTHelperVisitors.h
-//! @brief Reusable AST visitors
+//! @brief Reusable AST visitors and functions
 //
 // SPDX-FileCopyrightText: Michael Popoloski
 // SPDX-License-Identifier: MIT
 //------------------------------------------------------------------------------
 #pragma once
 
+#include "TidyConfig.h"
+#include "TidyFactory.h"
+#include <filesystem>
+
 #include "slang/ast/ASTVisitor.h"
 
+#define NEEDS_SKIP_SYMBOL(__symbol)                            \
+    if (skip(sourceManager->getFileName((__symbol).location))) \
+        return;
+
+// Function that tries to get the name of the variable in an expression
+std::optional<std::string_view> getIdentifier(const slang::ast::Expression& expr);
+
+struct TidyVisitor {
+protected:
+    explicit TidyVisitor(slang::Diagnostics& diagnostics) :
+        sourceManager(Registry::getSourceManager()), diags(diagnostics),
+        config(Registry::getConfig()) {}
+
+    [[nodiscard]] bool skip(std::string_view path) const {
+        auto file = std::filesystem::path(path).filename();
+        const auto& skipFiles = config.getSkipFiles();
+        return std::find(skipFiles.begin(), skipFiles.end(), file) != skipFiles.end();
+    }
+
+    slang::not_null<const slang::SourceManager*> sourceManager;
+    slang::Diagnostics& diags;
+    const TidyConfig& config;
+};
+
 /// ASTVisitor that will collect all identifiers under a node and store them in the identifiers
 /// internal variable so they can be retrieved later
 struct CollectIdentifiers : public slang::ast::ASTVisitor<CollectIdentifiers, false, true> {
@@ -31,17 +59,9 @@ struct LookupLhsIdentifier : public slang::ast::ASTVisitor<LookupLhsIdentifier,
     // Checks if the symbol on the LHS of the expression has the provided name
     static bool hasIdentifier(const std::string_view& name,
                               const slang::ast::AssignmentExpression& expression) {
-        const slang::ast::Symbol* symbol = nullptr;
-        if (slang::ast::MemberAccessExpression::isKind(expression.left().kind)) {
-            auto& memberAccess = expression.left().as<slang::ast::MemberAccessExpression>();
-            if (slang::ast::NamedValueExpression::isKind(memberAccess.value().kind))
-                symbol = &memberAccess.value().as<slang::ast::NamedValueExpression>().symbol;
-        }
-        else {
-            symbol = expression.left().getSymbolReference();
-        }
+        auto identifier = getIdentifier(expression.left());
 
-        if (symbol && symbol->name == name) {
+        if (identifier && identifier.value() == name) {
             return true;
         }
         return false;
@@ -54,6 +74,6 @@ struct LookupLhsIdentifier : public slang::ast::ASTVisitor<LookupLhsIdentifier,
     void reset() { _found = false; }
 
 private:
-    const std::string_view& name;
+    const std::string_view name;
     bool _found = false;
 };

tools/tidy/include/TidyConfig.h

@@ -0,0 +1,112 @@
+//------------------------------------------------------------------------------
+//! @file TidyConfig.h
+//! @brief Configuration of the tidy tool
+//
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+//------------------------------------------------------------------------------
+#pragma once
+
+#include "TidyKind.h"
+#include <algorithm>
+#include <fmt/format.h>
+#include <slang/util/CppTypePrinter.h>
+#include <string>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+
+class TidyConfig {
+    friend class TidyConfigParser;
+
+public:
+    /// Configuration values of checks
+    struct CheckConfigs {
+        std::string clkName;
+        std::string resetName;
+        bool resetIsActiveHigh;
+        std::string inputPortSuffix;
+        std::string outputPortSuffix;
+        std::string inoutPortSuffix;
+    };
+
+    /// Default TidyConfig constructor which will set the default check's configuration values
+    TidyConfig();
+
+    /// Returns whether a check is enabled or not
+    [[nodiscard]] bool isCheckEnabled(slang::TidyKind kind, const std::string& checkName) const;
+
+    /// Returns the check config object
+    inline const CheckConfigs& getCheckConfigs() const { return checkConfigs; }
+
+    // Returns a vector containing the file names that won't be checked by slang-tidy
+    inline const std::vector<std::string>& getSkipFiles() const { return skipFiles; }
+
+    // Adds a new file into the list of skipped files
+    void addSkipFile(const std::string& path);
+    void addSkipFile(std::vector<std::string> paths);
+
+private:
+    CheckConfigs checkConfigs;
+
+    /// Possible status of the checks
+    enum class CheckStatus { ENABLED, DISABLED };
+
+    std::unordered_map<slang::TidyKind, std::unordered_map<std::string, CheckStatus>> checkKinds;
+
+    // List of files that won't be checked by slang-tidy
+    std::vector<std::string> skipFiles;
+
+    /// Enables or disables all the implemented checks based on status
+    void toggleAl(CheckStatus status);
+
+    /// Enables or disables all the checks implemented in the TidyKind provided based on status
+    void toggleGroup(slang::TidyKind kind, CheckStatus status);
+
+    /// Disables or enables a particular check implemented in the TidyKind provided based on status.
+    /// It will return false if the check do not exist.
+    [[nodiscard]] bool toggleCheck(slang::TidyKind kind, const std::string& checkName,
+                                   CheckStatus status);
+
+    /// Sets the value of a check config. Will throw an invalid_argument exception if the type of
+    /// value doesn't match the config type
+    template<typename T>
+    void setConfig(const std::string& configName, T value) {
+        if (configName == "clkName") {
+            innerSetConfig(checkConfigs.clkName, value);
+            return;
+        }
+        else if (configName == "resetName") {
+            innerSetConfig(checkConfigs.resetName, value);
+            return;
+        }
+        else if (configName == "resetIsActiveHigh") {
+            innerSetConfig(checkConfigs.resetIsActiveHigh, value);
+            return;
+        }
+        else if (configName == "inputPortSuffix") {
+            innerSetConfig(checkConfigs.inputPortSuffix, value);
+            return;
+        }
+        else if (configName == "outputPortSuffix") {
+            innerSetConfig(checkConfigs.outputPortSuffix, value);
+            return;
+        }
+        else if (configName == "inoutPortSuffix") {
+            innerSetConfig(checkConfigs.inoutPortSuffix, value);
+            return;
+        }
+
+        SLANG_THROW(std::invalid_argument(fmt::format("The check: {} does not exist", configName)));
+    }
+
+    template<typename T, typename U>
+    void innerSetConfig(T& config, U value) {
+        if constexpr (std::is_same_v<T, U>)
+            config = value;
+        else
+            SLANG_THROW(
+                std::invalid_argument(fmt::format("check config expected a {} found {}",
+                                                  slang::typeName<T>(), slang::typeName<U>())));
+    }
+};