From e342f51565c8869b60847ee734b9808f027d286f Mon Sep 17 00:00:00 2001
From: MikePopoloski <mike@popoloski.com>
Date: Sun, 19 May 2024 15:36:03 -0400
Subject: [PATCH] Add Wunused-import and Wunused-wildcard-import

---
 scripts/diagnostics.txt              |  4 +++-
 scripts/warning_docs.txt             | 24 ++++++++++++++++++++++++
 source/ast/ElabVisitors.h            | 14 ++++++++++++++
 source/ast/Lookup.cpp                |  3 +++
 tests/unittests/ast/WarningTests.cpp | 24 ++++++++++++++++++++++++
 5 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt
index a76dd5980..562bbcab9 100644
--- a/scripts/diagnostics.txt
+++ b/scripts/diagnostics.txt
@@ -1107,6 +1107,8 @@ warning unused-type-parameter UnusedTypeParameter "unused type parameter '{}'"
 warning unused-typedef UnusedTypedef "unused typedef '{}'"
 warning unused-genvar UnusedGenvar "unused genvar '{}'"
 warning unused-assertion-decl UnusedAssertionDecl "unused {} '{}'"
+warning unused-import UnusedImport "unused import '{}'"
+warning unused-wildcard-import UnusedWildcardImport "unused wildcard import"
 warning missing-top NoTopModules "no top-level modules found in design"
 warning duplicate-defparam DuplicateDefparam "parameter already has a defparam override applied; ignoring this one"
 
@@ -1149,4 +1151,4 @@ group conversion = { width-trunc width-expand port-width-trunc port-width-expand
 group unused = { unused-def unused-net unused-implicit-net unused-variable undriven-net unassigned-variable
                  unused-but-set-net unused-but-set-variable unused-port undriven-port unused-argument
                  unused-parameter unused-type-parameter unused-typedef unused-genvar unused-assertion-decl
-                 unused-but-set-port unused-config-cell unused-config-instance }
+                 unused-but-set-port unused-config-cell unused-config-instance unused-import unused-wildcard-import }
diff --git a/scripts/warning_docs.txt b/scripts/warning_docs.txt
index acce14054..d2ed196e7 100644
--- a/scripts/warning_docs.txt
+++ b/scripts/warning_docs.txt
@@ -1013,6 +1013,30 @@ module m;
 endmodule
 ```
 
+-Wunused-import
+A package import directive is never used.
+```
+package p;
+    int a;
+endpackage
+
+module m;
+    import p::a;
+endmodule
+```
+
+-Wunused-wildcard-import
+A wildcard package import directive is never used.
+```
+package p;
+    int a;
+endpackage
+
+module m;
+    import p::*;
+endmodule
+```
+
 -Wbad-procedural-force
 According to the Verilog/SystemVerilog standard, it is illegal to procedurally force a
 bit-select or range select of a variable (you can do that with a net, or with a plain
diff --git a/source/ast/ElabVisitors.h b/source/ast/ElabVisitors.h
index 76596e590..5285fd86d 100644
--- a/source/ast/ElabVisitors.h
+++ b/source/ast/ElabVisitors.h
@@ -675,6 +675,20 @@ struct PostElabVisitor : public ASTVisitor<PostElabVisitor, false, false> {
     void handle(const LetDeclSymbol& symbol) { checkAssertionDeclUnused(symbol, "let"sv); }
     void handle(const CheckerSymbol& symbol) { checkAssertionDeclUnused(symbol, "checker"sv); }
 
+    void handle(const ExplicitImportSymbol& symbol) { checkUnused(symbol, diag::UnusedImport); }
+
+    void handle(const WildcardImportSymbol& symbol) {
+        auto syntax = symbol.getSyntax();
+        if (!syntax)
+            return;
+
+        auto [used, _] = compilation.isReferenced(*syntax);
+        if (!used) {
+            if (shouldWarn(symbol))
+                symbol.getParentScope()->addDiag(diag::UnusedWildcardImport, symbol.location);
+        }
+    }
+
 private:
     void checkValueUnused(const ValueSymbol& symbol, DiagCode unusedCode,
                           std::optional<DiagCode> unsetCode, std::optional<DiagCode> unreadCode) {
diff --git a/source/ast/Lookup.cpp b/source/ast/Lookup.cpp
index 3255dfa80..726e5160f 100644
--- a/source/ast/Lookup.cpp
+++ b/source/ast/Lookup.cpp
@@ -1767,6 +1767,7 @@ void Lookup::unqualifiedImpl(const Scope& scope, std::string_view name, LookupLo
                 case SymbolKind::ExplicitImport:
                     result.found = symbol->as<ExplicitImportSymbol>().importedSymbol();
                     result.flags |= LookupResultFlags::WasImported;
+                    scope.getCompilation().noteReference(*symbol);
                     break;
                 case SymbolKind::ForwardingTypedef:
                     // If we find a forwarding typedef, the actual typedef was never defined.
@@ -1886,6 +1887,8 @@ void Lookup::unqualifiedImpl(const Scope& scope, std::string_view name, LookupLo
 
                 result.flags |= LookupResultFlags::WasImported;
                 result.found = imports[0].imported;
+                scope.getCompilation().noteReference(*imports[0].import);
+
                 wildcardImportData->importedSymbols.try_emplace(result.found->name, result.found);
                 return;
             }
diff --git a/tests/unittests/ast/WarningTests.cpp b/tests/unittests/ast/WarningTests.cpp
index a1c60b279..17dc18a4a 100644
--- a/tests/unittests/ast/WarningTests.cpp
+++ b/tests/unittests/ast/WarningTests.cpp
@@ -611,6 +611,30 @@ endmodule
     CHECK(diags[2].code == diag::UnusedAssertionDecl);
 }
 
+TEST_CASE("Unused imports") {
+    auto tree = SyntaxTree::fromText(R"(
+package p;
+    int a;
+endpackage
+
+module m;
+    import p::a;
+    import p::*;
+endmodule
+)");
+
+    CompilationOptions coptions;
+    coptions.flags = CompilationFlags::None;
+
+    Compilation compilation(coptions);
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 2);
+    CHECK(diags[0].code == diag::UnusedImport);
+    CHECK(diags[1].code == diag::UnusedWildcardImport);
+}
+
 TEST_CASE("Implicit conversions with constants") {
     auto tree = SyntaxTree::fromText(R"(
 module m;