New tidy checks (#857)

This commit adds two new checks for port connections:

NoDotStarInPortConnection: Checks if the syntax .* has been used to
connect ports

NoImplicitPortNameInPortConnection: Checks if the syntax .port_name has
been used to connect ports

tools/tidy/CMakeLists.txt

@@ -18,7 +18,9 @@ add_library(
   src/style/EnforceModuleInstantiationPrefix.cpp
   src/style/OnlyANSIPortDecl.cpp
   src/synthesis/XilinxDoNotCareValues.cpp
-  src/synthesis/CastSignedIndex.cpp)
+  src/synthesis/CastSignedIndex.cpp
+  src/style/NoDotStarInPortConnection.cpp
+  src/style/NoImplicitPortNameInPortConnection.cpp)
 
 target_include_directories(slang_tidy_obj_lib PUBLIC include ../../include)
 target_link_libraries(slang_tidy_obj_lib PUBLIC slang::slang)

tools/tidy/include/TidyDiags.h

@@ -22,5 +22,7 @@ inline constexpr DiagCode EnforceModuleInstantiationPrefix(DiagSubsystem::Tidy,
 inline constexpr DiagCode OnlyANSIPortDecl(DiagSubsystem::Tidy, 8);
 inline constexpr DiagCode XilinxDoNotCareValues(DiagSubsystem::Tidy, 9);
 inline constexpr DiagCode CastSignedIndex(DiagSubsystem::Tidy, 10);
+inline constexpr DiagCode NoDotStarInPortConnection(DiagSubsystem::Tidy, 11);
+inline constexpr DiagCode NoImplicitPortNameInPortConnection(DiagSubsystem::Tidy, 12);
 
 } // namespace slang::diag

tools/tidy/src/TidyConfig.cpp

@@ -24,6 +24,8 @@ TidyConfig::TidyConfig() {
     styleChecks.emplace("NoOldAlwaysSyntax", CheckStatus::ENABLED);
     styleChecks.emplace("EnforceModuleInstantiationPrefix", CheckStatus::ENABLED);
     styleChecks.emplace("OnlyANSIPortDecl", CheckStatus::ENABLED);
+    styleChecks.emplace("NoDotStarInPortConnection", CheckStatus::ENABLED);
+    styleChecks.emplace("NoImplicitPortNameInPortConnection", CheckStatus::ENABLED);
     checkKinds.insert({slang::TidyKind::Style, styleChecks});
 
     auto synthesisChecks = std::unordered_map<std::string, CheckStatus>();

tools/tidy/src/style/AlwaysFFBlocking.cpp

@@ -61,7 +61,7 @@ class AlwaysFFBlocking : public TidyCheck {
     std::string diagString() const override {
         return "use of a blocking assignment for a non local variables inside always_ff";
     }
-    std::string name() const override { return "AlwaysCombNonBlocking"; }
+    std::string name() const override { return "AlwaysFFBlocking"; }
     std::string description() const override { return shortDescription(); }
     std::string shortDescription() const override {
         return "Enforces that blocking assignments are not being used inside always_ff "

tools/tidy/src/style/NoDotStarInPortConnection.cpp

@@ -0,0 +1,65 @@
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+
+#include "ASTHelperVisitors.h"
+#include "TidyDiags.h"
+
+#include "slang/syntax/AllSyntax.h"
+#include "slang/syntax/SyntaxVisitor.h"
+
+using namespace slang;
+using namespace slang::ast;
+using namespace slang::syntax;
+
+namespace no_dot_start_in_port_connection {
+
+struct PortConnectionVisitor : public SyntaxVisitor<PortConnectionVisitor> {
+    void handle(const WildcardPortConnectionSyntax&) { found = true; }
+
+public:
+    bool found{false};
+};
+
+struct MainVisitor : public TidyVisitor, ASTVisitor<MainVisitor, true, true> {
+    explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {}
+
+    void handle(const InstanceBodySymbol& symbol) {
+        NEEDS_SKIP_SYMBOL(symbol)
+        PortConnectionVisitor visitor;
+        symbol.getSyntax()->visit(visitor);
+        if (visitor.found)
+            diags.add(diag::NoDotStarInPortConnection, symbol.location);
+    }
+};
+} // namespace no_dot_start_in_port_connection
+
+using namespace no_dot_start_in_port_connection;
+
+class NoDotStarInPortConnection : public TidyCheck {
+public:
+    [[maybe_unused]] explicit NoDotStarInPortConnection(TidyKind kind) : TidyCheck(kind) {}
+
+    bool check(const RootSymbol& root) override {
+        MainVisitor visitor(diagnostics);
+        root.visit(visitor);
+        if (!diagnostics.empty())
+            return false;
+        return true;
+    }
+
+    DiagCode diagCode() const override { return diag::NoDotStarInPortConnection; }
+
+    std::string diagString() const override { return "use of .* in port connection list"; }
+
+    DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; }
+
+    std::string name() const override { return "NoDotStarInPortConnection"; }
+
+    std::string description() const override { return shortDescription(); }
+
+    std::string shortDescription() const override {
+        return "Checks if in a module instantiation any port is connected using .* syntax.";
+    }
+};
+
+REGISTER(NoDotStarInPortConnection, NoDotStarInPortConnection, TidyKind::Style)

tools/tidy/src/style/NoImplicitPortNameInPortConnection.cpp

@@ -0,0 +1,71 @@
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+
+#include "ASTHelperVisitors.h"
+#include "TidyDiags.h"
+#include <iostream>
+
+#include "slang/syntax/AllSyntax.h"
+#include "slang/syntax/SyntaxVisitor.h"
+
+using namespace slang;
+using namespace slang::ast;
+using namespace slang::syntax;
+
+namespace no_implicit_port_name_in_port_connection {
+
+struct PortConnectionVisitor : public SyntaxVisitor<PortConnectionVisitor> {
+    void handle(const PortConnectionSyntax& syntax) {
+        if (syntax.toString().find('(') == std::string::npos)
+            found = true;
+    }
+
+public:
+    bool found{false};
+};
+
+struct MainVisitor : public TidyVisitor, ASTVisitor<MainVisitor, true, true> {
+    explicit MainVisitor(Diagnostics& diagnostics) : TidyVisitor(diagnostics) {}
+
+    void handle(const InstanceBodySymbol& symbol) {
+        NEEDS_SKIP_SYMBOL(symbol)
+        PortConnectionVisitor visitor;
+        symbol.getSyntax()->visit(visitor);
+        if (visitor.found)
+            diags.add(diag::NoImplicitPortNameInPortConnection, symbol.location);
+    }
+};
+} // namespace no_implicit_port_name_in_port_connection
+
+using namespace no_implicit_port_name_in_port_connection;
+
+class NoImplicitPortNameInPortConnection : public TidyCheck {
+public:
+    [[maybe_unused]] explicit NoImplicitPortNameInPortConnection(TidyKind kind) : TidyCheck(kind) {}
+
+    bool check(const RootSymbol& root) override {
+        MainVisitor visitor(diagnostics);
+        root.visit(visitor);
+        if (!diagnostics.empty())
+            return false;
+        return true;
+    }
+
+    DiagCode diagCode() const override { return diag::NoImplicitPortNameInPortConnection; }
+
+    std::string diagString() const override {
+        return "port name not specified. Please use .port_name(net) syntax.";
+    }
+
+    DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; }
+
+    std::string name() const override { return "NoImplicitPortNameInPortConnection"; }
+
+    std::string description() const override { return shortDescription(); }
+
+    std::string shortDescription() const override {
+        return "Checks if in a module instantiation any port is connected using .port_name syntax.";
+    }
+};
+
+REGISTER(NoImplicitPortNameInPortConnection, NoImplicitPortNameInPortConnection, TidyKind::Style)

tools/tidy/src/style/NoOldAlwaysSyntax.cpp

@@ -55,11 +55,11 @@ class NoOldAlwaysSyntax : public TidyCheck {
 
     DiagCode diagCode() const override { return diag::NoOldAlwaysSyntax; }
 
-    std::string diagString() const override { return "Use of old always verilog syntax"; }
+    std::string diagString() const override { return "use of old always verilog syntax"; }
 
     DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Warning; }
 
-    std::string name() const override { return "NoOldAlwaysSyntax "; }
+    std::string name() const override { return "NoOldAlwaysSyntax"; }
 
     std::string description() const override { return shortDescription(); }
 

tools/tidy/src/synthesis/NoLatchesOnDesign.cpp

@@ -44,7 +44,7 @@ class NoLatchesOnDesign : public TidyCheck {
 
     DiagCode diagCode() const override { return diag::NoLatchesOnDesign; }
 
-    std::string diagString() const override { return "Latches are not allowed in this design"; }
+    std::string diagString() const override { return "latches are not allowed in this design"; }
 
     DiagnosticSeverity diagSeverity() const override { return DiagnosticSeverity::Error; }
 

tools/tidy/tests/CMakeLists.txt

@@ -18,7 +18,9 @@ add_executable(
   EnforceModuleInstantiationTest.cpp
   OnlyANSIPortDecl.cpp
   XilinxDoNotCareValuesTest.cpp
-  CastSignedIndexTest.cpp)
+  CastSignedIndexTest.cpp
+  NoDotStarInPortConnectionTest.cpp
+  NoImplicitPortNameInPortConnectionTest.cpp)
 
 target_link_libraries(tidy_unittests PRIVATE Catch2::Catch2 slang_tidy_obj_lib)
 target_compile_definitions(tidy_unittests PRIVATE UNITTESTS)

tools/tidy/tests/NoDotStarInPortConnectionTest.cpp

@@ -0,0 +1,53 @@
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+
+#include "Test.h"
+#include "TidyFactory.h"
+
+TEST_CASE("NoDotStarInPortConnection: Use of dot star in module port connection") {
+    auto tree = SyntaxTree::fromText(R"(
+module test (input clk, input rst);
+endmodule
+
+module top ();
+    logic clk, rst;
+    test t (.clk(clk), .*);
+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("NoDotStarInPortConnection");
+    bool result = visitor->check(root);
+    CHECK_FALSE(result);
+}
+
+TEST_CASE("NoDotStarInPortConnection: Module port connection port by port") {
+    auto tree = SyntaxTree::fromText(R"(
+module test (input clk, input rst);
+endmodule
+
+module top ();
+    logic clk, rst;
+    test t (.clk, .rst(rst));
+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("NoDotStarInPortConnection");
+    bool result = visitor->check(root);
+    CHECK(result);
+}

tools/tidy/tests/NoImplicitPortNameInPortConnectionTest.cpp

@@ -0,0 +1,54 @@
+// SPDX-FileCopyrightText: Michael Popoloski
+// SPDX-License-Identifier: MIT
+
+#include "Test.h"
+#include "TidyFactory.h"
+
+TEST_CASE(
+    "NoImplicitPortNameInPortConnection: Only port name specified in module port connection") {
+    auto tree = SyntaxTree::fromText(R"(
+module test (input clk, input rst);
+endmodule
+
+module top ();
+    logic clk, rst;
+    test t (.clk, .rst(rst));
+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("NoImplicitPortNameInPortConnection");
+    bool result = visitor->check(root);
+    CHECK_FALSE(result);
+}
+
+TEST_CASE("NoImplicitPortNameInPortConnection: Module port connection port by port") {
+    auto tree = SyntaxTree::fromText(R"(
+module test (input cl()k, input rst);
+endmodule
+
+module top ();
+    logic clk, rst;
+    test t (.clk(clk), .rst(rst));
+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("NoImplicitPortNameInPortConnection");
+    bool result = visitor->check(root);
+    CHECK(result);
+}