[slang] Add virtual interface hierarchical references usage checks.

include/slang/ast/symbols/InstanceSymbols.h

@@ -10,6 +10,7 @@
 #include "slang/ast/ASTContext.h"
 #include "slang/ast/Scope.h"
 #include "slang/ast/Symbol.h"
+#include "slang/ast/expressions/MiscExpressions.h"
 #include "slang/numeric/ConstantValue.h"
 #include "slang/syntax/SyntaxFwd.h"
 
@@ -164,6 +165,10 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope {
     /// Flags that describe properties of the instance.
     bitmask<InstanceFlags> flags;
 
+    /// Collected source ranges of hierarchical identifiers used inside instance body
+    /// for later diagnostic.
+    mutable std::optional<std::span<SourceRange>> hierIdentifiers;
+
     InstanceBodySymbol(Compilation& compilation, const DefinitionSymbol& definition,
                        const HierarchyOverrideNode* hierarchyOverrideNode,
                        bitmask<InstanceFlags> flags);
@@ -184,6 +189,9 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope {
 
     bool hasSameType(const InstanceBodySymbol& other) const;
 
+    /// Fill hierIdentifiers list.
+    void collectHierIdents(Compilation& comp) const;
+
     static InstanceBodySymbol& fromDefinition(
         Compilation& compilation, const DefinitionSymbol& definition, SourceLocation instanceLoc,
         bitmask<InstanceFlags> flags, const HierarchyOverrideNode* hierarchyOverrideNode,
@@ -199,9 +207,28 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope {
 
     static bool isKind(SymbolKind kind) { return kind == SymbolKind::InstanceBody; }
 
+    static void processHierIdent(const HierarchicalValueExpression& hVE,
+                                 const InstanceBodySymbol& ifaceBody,
+                                 SmallVector<SourceRange, 4>& hierIdents) {
+        const auto& sym = hVE.symbol;
+        const auto* parentScope = sym.getParentScope();
+        // Checking whether a symbol belongs to the interface definition body
+        while (parentScope->asSymbol().kind != SymbolKind::CompilationUnit) {
+            if (const auto symScope = parentScope->asSymbol().as_if<InstanceBodySymbol>();
+                symScope && symScope == &ifaceBody)
+                break;
+            parentScope = parentScope->asSymbol().getParentScope();
+        }
+
+        if (parentScope->asSymbol().kind == SymbolKind::CompilationUnit)
+            hierIdents.push_back(hVE.sourceRange);
+    }
+
 private:
     friend class Scope;
 
+    class HierIdentsVisitor;
+
     void setPorts(std::span<const Symbol* const> ports) const { portList = ports; }
 
     const DefinitionSymbol& definition;

scripts/diagnostics.txt

@@ -489,6 +489,7 @@ error TimingCheckEventEdgeRequired "'{}' event argument requires an edge trigger
 error NoChangeEdgeRequired "trigger for first argument to '$nochange' must be posedge or negedge only"
 error PathPulseInvalidPathName "PATHPULSE$ path '{}' must be of the form 'source$dest'"
 error VirtualInterfaceIfaceMember "virtual interfaces cannot be used as the type for interface items"
+error VirtualInterfaceHierRef "Declaration of virtual interface should not contain hierarchical references to objects outside its body"
 error DefparamBadHierarchy "defparam in a hierarchy in or under a generate block, array of instances, or bind instantiation cannot change a parameter value outside that hierarchy"
 error BindTargetPrimitive "cannot bind a primitive"
 error BindUnderBind "cannot bind an instance underneath the scope of another bind instance"

source/ast/ElabVisitors.h

@@ -18,7 +18,7 @@ using namespace syntax;
 
 // This visitor is used to touch every node in the AST to ensure that all lazily
 // evaluated members have been realized and we have recorded every diagnostic.
-struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
+struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, true> {
     DiagnosticVisitor(Compilation& compilation, const size_t& numErrors, uint32_t errorLimit) :
         compilation(compilation), numErrors(numErrors), errorLimit(errorLimit) {}
 
@@ -336,8 +336,25 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
             return;
         }
 
-        if (visitInstances)
+        if (visitInstances) {
+            // Collect hierarchical identifiers for current instance
+            SmallVector<SourceRange, 4> hierIdents;
+            instBodiesHierIdents.push_back(std::make_pair(&symbol.body, hierIdents));
             visit(symbol.body);
+            symbol.body.hierIdentifiers = instBodiesHierIdents.back().second.copy(compilation);
+            instBodiesHierIdents.pop_back();
+        }
+    }
+
+    void handle(const HierarchicalValueExpression& hVE) {
+        if (instBodiesHierIdents.empty()) {
+            visitDefault(hVE);
+            return;
+        }
+
+        InstanceBodySymbol::processHierIdent(hVE, *instBodiesHierIdents.back().first,
+                                             instBodiesHierIdents.back().second);
+        visitDefault(hVE);
     }
 
     void handle(const SubroutineSymbol& symbol) {
@@ -466,6 +483,10 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
     SmallVector<const MethodPrototypeSymbol*> externIfaceProtos;
     SmallVector<std::pair<const InterfacePortSymbol*, const ModportSymbol*>> modportsWithExports;
     TimingPathMap timingPathMap;
+
+private:
+    SmallVector<std::pair<const InstanceBodySymbol*, SmallVector<SourceRange, 4>>, 4>
+        instBodiesHierIdents;
 };
 
 // This visitor is for finding all defparam directives in the hierarchy.

source/ast/symbols/InstanceSymbols.cpp

@@ -1126,6 +1126,28 @@ bool InstanceBodySymbol::hasSameType(const InstanceBodySymbol& other) const {
     return true;
 }
 
+class InstanceBodySymbol::HierIdentsVisitor : public ASTVisitor<HierIdentsVisitor, true, true> {
+public:
+    SmallVector<SourceRange, 4> hierIdentsSR;
+
+    HierIdentsVisitor(const InstanceBodySymbol& ifaceBody) : ifaceBody(ifaceBody){};
+
+    void handle(const HierarchicalValueExpression& hVE) {
+        processHierIdent(hVE, ifaceBody, hierIdentsSR);
+    }
+
+private:
+    const InstanceBodySymbol& ifaceBody;
+};
+
+void InstanceBodySymbol::collectHierIdents(Compilation& comp) const {
+    if (!hierIdentifiers.has_value()) {
+        HierIdentsVisitor hIdentsVisitor(*this);
+        hIdentsVisitor.visit(*this);
+        hierIdentifiers = hIdentsVisitor.hierIdentsSR.copy(comp);
+    }
+}
+
 void InstanceBodySymbol::serializeTo(ASTSerializer& serializer) const {
     serializer.writeLink("definition", definition);
 }

source/ast/types/AllTypes.cpp

@@ -1128,9 +1128,48 @@ const Type& VirtualInterfaceType::fromSyntax(const ASTContext& context,
     }
 
     auto loc = syntax.name.location();
+    auto& defSym = def->as<DefinitionSymbol>();
     auto& iface = InstanceSymbol::createVirtual(context, loc, def->as<DefinitionSymbol>(),
                                                 syntax.parameters);
 
+    // To control duplicates
+    SmallSet<const DefinitionSymbol*, 4> visited;
+    // Find cached instance hierarchical identifiers
+    std::function<std::optional<const InstanceSymbol*>(const Scope& scope)> findInst =
+        [&defSym, &findInst, &visited](const Scope& scope) -> std::optional<const InstanceSymbol*> {
+        for (auto& name : scope.getNameMap()) {
+            if (auto inst = name.second->as_if<InstanceSymbol>()) {
+                auto instDef = &inst->getDefinition();
+                if (instDef == &defSym && inst->body.hierIdentifiers.has_value())
+                    return inst;
+
+                if (visited.contains(instDef))
+                    continue;
+                visited.insert(instDef);
+
+                if (auto founded = findInst(inst->body); founded.has_value())
+                    return founded;
+            }
+        }
+        return std::nullopt;
+    };
+
+    auto& scope = comp.getRoot();
+    auto founded = findInst(scope);
+    const InstanceSymbol* toReport = nullptr;
+    if (!founded.has_value()) {
+        iface.body.collectHierIdents(comp);
+        toReport = &iface;
+    }
+    else {
+        toReport = founded.value();
+    }
+
+    for (auto& sR : toReport->body.hierIdentifiers.value()) {
+        auto& diag = context.addDiag(diag::VirtualInterfaceHierRef, sR);
+        diag.addNote(diag::NoteDeclarationHere, iface.location);
+    }
+
     const ModportSymbol* modport = nullptr;
     std::string_view modportName = syntax.modport ? syntax.modport->member.valueText() : ""sv;
     if (!modportName.empty() && syntax.modport) {

tests/unittests/ast/InterfaceTests.cpp

@@ -724,3 +724,127 @@ endmodule
     REQUIRE(diags.size() == 1);
     CHECK(diags[0].code == diag::UndeclaredIdentifier);
 }
+
+TEST_CASE("Virtual interface declaration errors") {
+    auto tree = SyntaxTree::fromText(R"(
+localparam type requestType = byte;
+localparam type responseType = int;
+
+module testMod#(N=16);
+
+  wire clk, rst;
+
+  allIfc#(N) allInst(clk, rst);
+  sliceIfc sliceInst();
+  virtual sliceIfc sliceInst1;
+
+endmodule:testMod
+
+module testMod1#(N=16);
+
+  wire clk, rst;
+
+  allIfc#(N) allInst(clk, rst);
+  virtual sliceIfc1 sliceInst;
+
+endmodule:testMod1
+
+interface automatic allIfc#(N=1)(input clk, rst);
+
+  var requestType Requests[N];
+  var responseType Responses[N];
+
+  function requestType requestRead(int index);
+    return Requests[index];
+  endfunction
+
+  function void responseWrite(int index, responseType response);
+    Responses[index] <= response;
+  endfunction
+
+  modport clientMp(output Requests, input Responses,
+                   input clk, rst);
+
+  modport serverMp(input Requests, output Responses,
+                   import requestRead, responseWrite,
+                   input clk, rst);
+
+endinterface:allIfc
+
+interface automatic sliceIfc#(I=0)();
+  interface II();
+      logic reset;
+  endinterface
+  II ii();  // invalid
+  wire reset = ii.reset;  // valid
+
+  allIfc allInst(.clk(), .rst());
+  var requestType request;
+  var responseType response;
+
+  assign allInst.Requests[I] = request;    // invalid
+  assign response = allInst.Responses[I];  // invalid
+
+  function void requestWrite(requestType req);
+    request <= req;
+  endfunction
+
+  function responseType responseRead();
+    return response;
+  endfunction
+
+  wire clk = allInst.clk;  // invalid
+  wire rst = allInst.rst;  // invalid
+
+  modport clientMp(output request, input response,
+                   import requestWrite, responseRead,
+                   input clk, rst);
+
+endinterface:sliceIfc
+
+interface automatic sliceIfc1#(I=0)();
+  interface II();
+      logic reset;
+  endinterface
+  II ii();  // invalid
+  wire reset = ii.reset;  // valid
+
+  allIfc allInst(.clk(), .rst());
+  var requestType request;
+  var responseType response;
+
+  assign allInst.Requests[I] = request;    // invalid
+  assign response = allInst.Responses[I];  // invalid
+
+  function void requestWrite(requestType req);
+    request <= req;
+  endfunction
+
+  function responseType responseRead();
+    return response;
+  endfunction
+
+  wire clk = allInst.clk;  // invalid
+  wire rst = allInst.rst;  // invalid
+
+  modport clientMp(output request, input response,
+                   import requestWrite, responseRead,
+                   input clk, rst);
+
+endinterface:sliceIfc1
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 8);
+    CHECK(diags[0].code == diag::VirtualInterfaceHierRef);
+    CHECK(diags[1].code == diag::VirtualInterfaceHierRef);
+    CHECK(diags[2].code == diag::VirtualInterfaceHierRef);
+    CHECK(diags[3].code == diag::VirtualInterfaceHierRef);
+    CHECK(diags[4].code == diag::VirtualInterfaceHierRef);
+    CHECK(diags[5].code == diag::VirtualInterfaceHierRef);
+    CHECK(diags[6].code == diag::VirtualInterfaceHierRef);
+    CHECK(diags[7].code == diag::VirtualInterfaceHierRef);
+}