Updates to slang-tidy (#785)

* slang-tidy fix NoOldAlwaysSyntax check when dealing with assertions

The NoOldAlwaysSyntax check was miss triggering with the property clock,
this commit checks if block is an assertion to not trigger the check.

* slang-tidy add new check EnforceModuleInstantiationPrefix

This new check will enforce a prefix string in module instantiation

* slang-tidy minor syntax fixes

* style: pre-commit fixes

* slang-tidy remove unnecessary include

tools/tidy/CMakeLists.txt

@@ -14,7 +14,8 @@ add_library(
   src/synthesis/NoLatchesOnDesign.cpp
   src/style/NoOldAlwaysSyntax.cpp
   src/style/AlwaysCombNonBlocking.cpp
-  src/style/AlwaysFFBlocking.cpp)
+  src/style/AlwaysFFBlocking.cpp
+  src/style/EnforceModuleInstantiationPrefix.cpp)
 
 target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include)
 target_link_libraries(slang_tidy_obj_lib PUBLIC slang::slang)

tools/tidy/include/TidyConfig.h

@@ -28,6 +28,7 @@ class TidyConfig {
         std::string inputPortSuffix;
         std::string outputPortSuffix;
         std::string inoutPortSuffix;
+        std::string moduleInstantiationPrefix;
     };
 
     /// Default TidyConfig constructor which will set the default check's configuration values

tools/tidy/include/TidyDiags.h

@@ -18,5 +18,6 @@ inline constexpr DiagCode NoLatchesOnDesign(DiagSubsystem::Tidy, 3);
 inline constexpr DiagCode NoOldAlwaysSyntax(DiagSubsystem::Tidy, 4);
 inline constexpr DiagCode AlwaysCombNonBlocking(DiagSubsystem::Tidy, 5);
 inline constexpr DiagCode AlwaysFFBlocking(DiagSubsystem::Tidy, 6);
+inline constexpr DiagCode EnforceModuleInstantiationPrefix(DiagSubsystem::Tidy, 7);
 
 } // namespace slang::diag

tools/tidy/src/TidyConfig.cpp

@@ -14,12 +14,14 @@ TidyConfig::TidyConfig() {
     checkConfigs.inputPortSuffix = "_i";
     checkConfigs.outputPortSuffix = "_o";
     checkConfigs.inoutPortSuffix = "_io";
+    checkConfigs.moduleInstantiationPrefix = "i_";
 
     auto styleChecks = std::unordered_map<std::string, CheckStatus>();
     styleChecks.emplace("AlwaysCombNonBlocking", CheckStatus::ENABLED);
     styleChecks.emplace("AlwaysFFBlocking", CheckStatus::ENABLED);
     styleChecks.emplace("EnforcePortSuffix", CheckStatus::ENABLED);
     styleChecks.emplace("NoOldAlwaysSyntax", CheckStatus::ENABLED);
+    styleChecks.emplace("EnforceModuleInstantiationPrefix", CheckStatus::ENABLED);
     checkKinds.insert({slang::TidyKind::Style, styleChecks});
 
     auto synthesisChecks = std::unordered_map<std::string, CheckStatus>();

tools/tidy/src/TidyConfigParser.cpp

@@ -5,7 +5,6 @@
 
 #include "fmt/format.h"
 #include <fstream>
-#include <iostream>
 
 #include "slang/util/OS.h"
 
@@ -352,7 +351,7 @@ void TidyConfigParser::setCheckConfig(const std::string& configName, std::string
     std::transform(configValue.begin(), configValue.end(), configValue.begin(), ::tolower);
 
     if (configValue == "true" || configValue == "false") {
-        set_config(configValue == "true" ? true : false);
+        set_config(configValue == "true");
     }
     else {
         set_config(configValue);

tools/tidy/src/style/EnforceModuleInstantiationPrefix.cpp

@@ -0,0 +1,63 @@
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+
+#include "ASTHelperVisitors.h"
+#include "TidyDiags.h"
+#include "fmt/color.h"
+
+#include "slang/syntax/AllSyntax.h"
+
+using namespace slang;
+using namespace slang::ast;
+
+namespace enforce_module_instantiation_prefix {
+struct MainVisitor : public TidyVisitor, ASTVisitor<MainVisitor, true, false> {
+    explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {}
+
+    void handle(const InstanceSymbol& instance) {
+        NEEDS_SKIP_SYMBOL(instance)
+
+        if (!instance.isModule())
+            return;
+
+        std::string_view prefix = config.getCheckConfigs().moduleInstantiationPrefix;
+        for (auto& member : instance.body.members()) {
+            if (!member.name.starts_with(prefix)) {
+                diags.add(diag::EnforcePortSuffix, member.location) << member.name << prefix;
+            }
+        }
+    }
+};
+} // namespace enforce_module_instantiation_prefix
+
+using namespace enforce_module_instantiation_prefix;
+class EnforceModuleInstantiationPrefix : public TidyCheck {
+public:
+    [[maybe_unused]] explicit EnforceModuleInstantiationPrefix(TidyKind kind) : TidyCheck(kind) {}
+
+    bool check(const ast::RootSymbol& root) override {
+        MainVisitor visitor(diagnostics);
+        root.visit(visitor);
+        if (!diagnostics.empty())
+            return false;
+        return true;
+    }
+
+    DiagCode diagCode() const override { return diag::EnforceModuleInstantiationPrefix; }
+    DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; }
+    std::string diagString() const override {
+        return "module instantiation '{}' is not correctly suffixed with prefix: '{}'";
+    }
+    std::string name() const override { return "EnforceModuleInstantiationPrefix"; }
+    std::string description() const override {
+        return "Enforces that module instantiations in the design follow the code guidelines "
+               "provided in the configuration file by the config " +
+               fmt::format(fmt::emphasis::italic, "moduleInstantiationPrefix");
+    }
+    std::string shortDescription() const override {
+        return "Enforces that module instantiations in the design follows the code guidelines "
+               "provided in the configuration file";
+    }
+};
+
+REGISTER(EnforceModuleInstantiationPrefix, EnforceModuleInstantiationPrefix, TidyKind::Style)

tools/tidy/src/style/NoOldAlwaysSyntax.cpp

@@ -16,6 +16,9 @@ struct MainVisitor : public TidyVisitor, ASTVisitor<MainVisitor, true, true> {
     void handle(const ast::ProceduralBlockSymbol& symbol) {
         NEEDS_SKIP_SYMBOL(symbol)
 
+        if (symbol.isFromAssertion)
+            return;
+
         if (symbol.procedureKind == ProceduralBlockKind::Always) {
             diags.add(diag::NoOldAlwaysSyntax, symbol.location);
         }

tools/tidy/src/tidy.cpp

@@ -16,7 +16,6 @@
 #include "slang/ast/Compilation.h"
 #include "slang/diagnostics/TextDiagnosticClient.h"
 #include "slang/driver/Driver.h"
-#include "slang/text/SourceManager.h"
 #include "slang/util/Version.h"
 
 /// Performs a search for the .slang-tidy file on the current directory. If the file is not found,
@@ -85,8 +84,8 @@ int main(int argc, char** argv) {
         Registry::setSourceManager(&sm);
 
         bool first = true;
-        for (const auto& check_name : Registry::getRegisteredChecks()) {
-            const auto check = Registry::create(check_name);
+        for (const auto& checkName : Registry::getRegisteredChecks()) {
+            const auto check = Registry::create(checkName);
             if (first)
                 first = false;
             else
@@ -104,11 +103,11 @@ int main(int argc, char** argv) {
         return 1;
 
     std::unique_ptr<ast::Compilation> compilation;
-    bool compilation_ok;
+    bool compilationOk;
     SLANG_TRY {
-        compilation_ok = driver.parseAllSources();
+        compilationOk = driver.parseAllSources();
         compilation = driver.createCompilation();
-        compilation_ok &= driver.reportCompilation(*compilation, true);
+        compilationOk &= driver.reportCompilation(*compilation, true);
     }
     SLANG_CATCH(const std::exception& e) {
 #if __cpp_exceptions
@@ -117,7 +116,7 @@ int main(int argc, char** argv) {
         return 1;
     }
 
-    if (!compilation_ok) {
+    if (!compilationOk) {
         slang::OS::print("slang-tidy: errors found during compilation\n");
         return 1;
     }
@@ -127,19 +126,19 @@ int main(int argc, char** argv) {
     // Set the sourceManager to the Registry so checks can access it
     Registry::setSourceManager(compilation->getSourceManager());
 
-    int ret_code = 0;
+    int retCode = 0;
 
     // Check all enabled checks
-    for (const auto& check_name : Registry::getEnabledChecks()) {
-        const auto check = Registry::create(check_name);
+    for (const auto& checkName : Registry::getEnabledChecks()) {
+        const auto check = Registry::create(checkName);
         OS::print(fmt::format("[{}]", check->name()));
 
         driver.diagEngine.setMessage(check->diagCode(), check->diagString());
         driver.diagEngine.setSeverity(check->diagCode(), check->diagSeverity());
 
         auto checkOk = check->check(compilation->getRoot());
         if (!checkOk) {
-            ret_code = 1;
+            retCode = 1;
             OS::print(fmt::emphasis::bold | fmt::fg(fmt::color::red), " FAIL\n");
             for (const auto& diag : check->getDiagnostics())
                 driver.diagEngine.issue(diag);
@@ -151,7 +150,7 @@ int main(int argc, char** argv) {
         }
     }
 
-    return ret_code;
+    return retCode;
 }
 
 std::optional<std::filesystem::path> project_slang_tidy_config() {

tools/tidy/tests/CMakeLists.txt

@@ -14,7 +14,8 @@ add_executable(
   EnforcePortSuffixTest.cpp
   NoOldAlwaysSyntaxTest.cpp
   AlwaysCombNonBlockingTest.cpp
-  AlwaysFFBlockingTest.cpp)
+  AlwaysFFBlockingTest.cpp
+  EnforceModuleInstantiationTest.cpp)
 
 target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib)
 target_compile_definitions(tidy_unittests PRIVATE UNITTESTS)

tools/tidy/tests/EnforceModuleInstantiationTest.cpp

@@ -0,0 +1,51 @@
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+
+#include "Test.h"
+#include "TidyFactory.h"
+
+TEST_CASE("EnforceModuleInstantiationPrefix: Incorrect module instantiation prefix") {
+    auto tree = SyntaxTree::fromText(R"(
+module test ();
+endmodule
+
+module top();
+    test test();
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+    compilation.getAllDiagnostics();
+    auto& root = compilation.getRoot();
+
+    TidyConfig config;
+    Registry::setConfig(config);
+    Registry::setSourceManager(compilation.getSourceManager());
+    auto visitor = Registry::create("EnforceModuleInstantiationPrefix");
+    bool result = visitor->check(root);
+    CHECK_FALSE(result);
+}
+
+TEST_CASE("EnforceModuleInstantiationPrefix: Correct module instantiation prefix") {
+    auto tree = SyntaxTree::fromText(R"(
+module test ();
+endmodule
+
+module top();
+    test i_test();
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+    compilation.getAllDiagnostics();
+    auto& root = compilation.getRoot();
+
+    TidyConfig config;
+    Registry::setConfig(config);
+    Registry::setSourceManager(compilation.getSourceManager());
+    auto visitor = Registry::create("EnforceModuleInstantiationPrefix");
+    bool result = visitor->check(root);
+    CHECK(result);
+}

tools/tidy/tests/NoOldAlwaysSyntaxTest.cpp

@@ -80,3 +80,75 @@ endmodule
     bool result = visitor->check(root);
     CHECK(result);
 }
+
+TEST_CASE("NoOldAlwaysSyntax: Assertion") {
+    auto tree = SyntaxTree::fromText(R"(
+module top(input logic clk, input logic rst);
+    logic a, b;
+
+    test : assert property (
+        @(posedge clk) disable iff (rst) a |-> b) else $error("error");
+    )
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+    compilation.getAllDiagnostics();
+    auto& root = compilation.getRoot();
+
+    TidyConfig config;
+    Registry::setConfig(config);
+    Registry::setSourceManager(compilation.getSourceManager());
+    auto visitor = Registry::create("NoOldAlwaysSyntax");
+    bool result = visitor->check(root);
+    CHECK(result);
+}
+
+TEST_CASE("NoOldAlwaysSyntax: Sequence") {
+    auto tree = SyntaxTree::fromText(R"(
+module top(input logic clk);
+    logic a, b;
+
+    sequence;
+        @(posedge clk) a |-> b
+    endsequence
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+    compilation.getAllDiagnostics();
+    auto& root = compilation.getRoot();
+
+    TidyConfig config;
+    Registry::setConfig(config);
+    Registry::setSourceManager(compilation.getSourceManager());
+    auto visitor = Registry::create("NoOldAlwaysSyntax");
+    bool result = visitor->check(root);
+    CHECK(result);
+}
+
+TEST_CASE("NoOldAlwaysSyntax: Covergroup") {
+    auto tree = SyntaxTree::fromText(R"(
+module top(input logic clk, input logic rst);
+    logic a;
+
+    covergroup cg @(posedge clk);
+        coverpoint a;
+    endgroup
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+    compilation.getAllDiagnostics();
+    auto& root = compilation.getRoot();
+
+    TidyConfig config;
+    Registry::setConfig(config);
+    Registry::setSourceManager(compilation.getSourceManager());
+    auto visitor = Registry::create("NoOldAlwaysSyntax");
+    bool result = visitor->check(root);
+    CHECK(result);
+}