Enforce missing rules for virtual interfaces

1. Virtual interfaces can't have hierarchical references to objects outside the interface
2. Virtual interfaces can't have interface ports

include/slang/ast/Compilation.h

@@ -34,6 +34,7 @@ class ConfigBlockSymbol;
 class DefinitionSymbol;
 class Expression;
 class GenericClassDefSymbol;
+class InstanceSymbol;
 class InterfacePortSymbol;
 class MethodPrototypeSymbol;
 class ModportSymbol;
@@ -328,6 +329,9 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
     /// Gets all of the diagnostics produced during compilation.
     const Diagnostics& getAllDiagnostics();
 
+    /// Queries if any errors have been issued on any scope within this compilation.
+    bool hasIssuedErrors() const { return numErrors > 0; };
+
     /// @}
     /// @name Utility and convenience methods
     /// @{
@@ -588,6 +592,13 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
                       const Expression& firstExpr, const Symbol& secondSym,
                       DriverBitRange secondRange, const Expression& secondExpr);
 
+    /// Notes the existence of the given hierarchical reference, which is used,
+    /// among other things, to ensure we perform instance caching correctly.
+    void noteHierarchicalReference(const Scope& scope, const HierarchicalReference& ref);
+
+    /// Notes the existence of a virtual interface type declaration for the given instance.
+    void noteVirtualIfaceInstance(const InstanceSymbol& instance);
+
     /// Adds a set of diagnostics to the compilation's list of semantic diagnostics.
     void addDiagnostics(const Diagnostics& diagnostics);
 
@@ -723,14 +734,12 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
     /// Creates an empty ImplicitTypeSyntax object.
     const syntax::ImplicitTypeSyntax& createEmptyTypeSyntax(SourceLocation loc);
 
-    /// Queries if any errors have been issued on any scope within this compilation.
-    bool hasIssuedErrors() const { return numErrors > 0; };
-
     /// @}
 
 private:
     friend class Lookup;
     friend class Scope;
+    friend class DiagnosticVisitor;
 
     // Collected information about a resolved bind directive.
     struct ResolvedBind {
@@ -764,6 +773,7 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
                             ResolvedBind& resolvedBind);
     void checkBindTargetParams(const syntax::BindDirectiveSyntax& syntax, const Scope& scope,
                                const ResolvedBind& resolvedBind);
+    void checkVirtualIfaceInstance(const InstanceSymbol& instance);
     std::pair<DefinitionLookupResult, bool> resolveConfigRule(const Scope& scope,
                                                               const ConfigRule& rule) const;
     std::pair<DefinitionLookupResult, bool> resolveConfigRules(
@@ -839,6 +849,9 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
     // Map from pointers (to symbols, statements, expressions) to their associated attributes.
     flat_hash_map<const void*, std::span<const AttributeSymbol* const>> attributeMap;
 
+    // Map from instance bodies to hierarchical references that extend up through them.
+    flat_hash_map<const Symbol*, std::vector<const HierarchicalReference*>> hierRefMap;
+
     struct SyntaxMetadata {
         const syntax::SyntaxTree* tree = nullptr;
         const NetType* defaultNetType = nullptr;
@@ -860,6 +873,9 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
     // A list of libraries that control the order in which we search for cell bindings.
     std::vector<const SourceLibrary*> defaultLiblist;
 
+    // A list of instances that have been created by virtual interface type declarations.
+    std::vector<const InstanceSymbol*> virtualInterfaceInstances;
+
     // A map from class name + decl name + scope to out-of-block declarations. These get
     // registered when we find the initial declaration and later get used when we see
     // the class prototype. The value also includes a boolean indicating whether anything

include/slang/ast/expressions/MiscExpressions.h

@@ -75,8 +75,8 @@ class SLANG_EXPORT HierarchicalValueExpression : public ValueExpressionBase {
     /// Information about the hierarchical reference.
     HierarchicalReference ref;
 
-    HierarchicalValueExpression(const ValueSymbol& symbol, const HierarchicalReference& ref,
-                                SourceRange sourceRange);
+    HierarchicalValueExpression(const Scope& scope, const ValueSymbol& symbol,
+                                const HierarchicalReference& ref, SourceRange sourceRange);
 
     ConstantValue evalImpl(EvalContext& context) const;
 
@@ -138,7 +138,7 @@ class SLANG_EXPORT ArbitrarySymbolExpression : public Expression {
     /// if this expression was created via hierarchical reference.
     HierarchicalReference hierRef;
 
-    ArbitrarySymbolExpression(const Symbol& symbol, const Type& type,
+    ArbitrarySymbolExpression(const Scope& scope, const Symbol& symbol, const Type& type,
                               const HierarchicalReference* hierRef, SourceRange sourceRange);
 
     ConstantValue evalImpl(EvalContext&) const { return nullptr; }

scripts/diagnostics.txt

@@ -1126,8 +1126,11 @@ error ConfigParamsIgnored "parameter overrides provided by config rule are ignor
 error MultipleTopDupName "more than one top module specified with the name '{}'"
 error NetAliasSelf "cannot alias a net to itself"
 error MultipleNetAlias "cannot specify an alias between the same bits of the same nets more than once"
+error VirtualIfaceHierRef "interface containing hierarchical reference to object outside its body cannot be used in a virtual interface"
+error VirtualIfaceIfacePort "interface with ports that reference other interfaces cannot be used in a virtual interface"
 note NoteAliasedTo "which is aliased to"
 note NoteAliasDeclaration "previous alias declaration here"
+note NoteHierarchicalRef "hierarchical reference here"
 warning unused-def UnusedDefinition "{} definition is unused"
 warning unused-net UnusedNet "unused net '{}'"
 warning unused-implicit-net UnusedImplicitNet "implicit net '{}' is unused elsewhere"

source/ast/Compilation.cpp

@@ -1254,6 +1254,26 @@ void Compilation::noteNetAlias(const Scope& scope, const Symbol& firstSym,
     }
 }
 
+void Compilation::noteHierarchicalReference(const Scope& initialScope,
+                                            const HierarchicalReference& ref) {
+    // For now, we're only interested in upward names that cross
+    // through instance boundaries.
+    SLANG_ASSERT(ref.expr);
+    auto currScope = &initialScope;
+    for (size_t i = 0; i < ref.upwardCount; i++) {
+        auto& sym = currScope->asSymbol();
+        if (sym.kind == SymbolKind::InstanceBody)
+            hierRefMap[&sym].push_back(&ref);
+
+        currScope = sym.getHierarchicalParent();
+        SLANG_ASSERT(currScope);
+    }
+}
+
+void Compilation::noteVirtualIfaceInstance(const InstanceSymbol& symbol) {
+    virtualInterfaceInstances.push_back(&symbol);
+}
+
 const Expression* Compilation::getDefaultDisable(const Scope& scope) const {
     auto curr = &scope;
     while (true) {
@@ -2197,6 +2217,27 @@ void Compilation::checkBindTargetParams(const syntax::BindDirectiveSyntax& synta
     }
 }
 
+void Compilation::checkVirtualIfaceInstance(const InstanceSymbol& instance) {
+    auto body = instance.getCanonicalBody();
+    if (!body)
+        body = &instance.body;
+
+    if (auto it = hierRefMap.find(body); it != hierRefMap.end()) {
+        auto& diag = body->addDiag(diag::VirtualIfaceHierRef, instance.location);
+        for (auto ref : it->second)
+            diag.addNote(diag::NoteHierarchicalRef, ref->expr->sourceRange);
+    }
+
+    Diagnostic* portDiag = nullptr;
+    for (auto port : body->getPortList()) {
+        if (port->kind == SymbolKind::InterfacePort) {
+            if (!portDiag)
+                portDiag = &body->addDiag(diag::VirtualIfaceIfacePort, instance.location);
+            portDiag->addNote(diag::NoteDeclarationHere, port->location);
+        }
+    }
+}
+
 void Compilation::resolveDefParamsAndBinds() {
     TimeTraceScope timeScope("resolveDefParamsAndBinds"sv, ""sv);
 

source/ast/ElabVisitors.h

@@ -380,12 +380,11 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
         // in other instances to facilitate downstream consumers in not needing to recreate
         // this duplication detection logic again.
         SLANG_ASSERT(symbol.getCanonicalBody() == nullptr);
-        if (!compilation.hasFlag(CompilationFlags::DisableInstanceCaching)) {
-            auto [it, inserted] = instanceCache.try_emplace(symbol, &symbol.body);
-            if (!inserted) {
-                symbol.setCanonicalBody(*it->second);
+        auto [it, inserted] = instanceCache.try_emplace(symbol, &symbol.body);
+        if (!inserted) {
+            symbol.setCanonicalBody(*it->second);
+            if (!compilation.hasFlag(CompilationFlags::DisableInstanceCaching))
                 return;
-            }
         }
 
         if (visitInstances)
@@ -474,9 +473,19 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
 
     void finalize() {
         // Once everything has been visited, go back over and check things that might
-        // have been influenced by visiting later symbols. Unfortunately visiting
-        // a specialization can trigger more specializations to be made for the
-        // same or other generic class, so we need to be careful here when iterating.
+        // have been influenced by visiting later symbols.
+        while (!compilation.virtualInterfaceInstances.empty()) {
+            auto vii = compilation.virtualInterfaceInstances;
+            compilation.virtualInterfaceInstances.clear();
+
+            for (auto inst : vii) {
+                inst->visit(*this);
+                compilation.checkVirtualIfaceInstance(*inst);
+            }
+        }
+
+        // Visiting a specialization can trigger more specializations to be made for the
+        // same or other generic classes, so we need to be careful here when iterating.
         SmallSet<const Type*, 8> visitedSpecs;
         SmallVector<const Type*> toVisit;
         bool didSomething;

source/ast/Expression.cpp

@@ -1423,8 +1423,9 @@ Expression* Expression::tryBindInterfaceRef(const ASTContext& context,
         if (symbol->kind == SymbolKind::UninstantiatedDef ||
             (symbol->kind == SymbolKind::Variable &&
              symbol->as<VariableSymbol>().getType().isError())) {
-            return comp.emplace<ArbitrarySymbolExpression>(*origSymbol, comp.getErrorType(),
-                                                           nullptr, syntax.sourceRange());
+            return comp.emplace<ArbitrarySymbolExpression>(*context.scope, *origSymbol,
+                                                           comp.getErrorType(), nullptr,
+                                                           syntax.sourceRange());
         }
 
         if (isInterfacePort && !origSymbol->name.empty()) {
@@ -1498,7 +1499,8 @@ Expression* Expression::tryBindInterfaceRef(const ASTContext& context,
     }
 
     auto hierRef = HierarchicalReference::fromLookup(comp, result);
-    return comp.emplace<ArbitrarySymbolExpression>(*origSymbol, *type, &hierRef, sourceRange);
+    return comp.emplace<ArbitrarySymbolExpression>(*context.scope, *origSymbol, *type, &hierRef,
+                                                   sourceRange);
 }
 
 void Expression::findPotentiallyImplicitNets(

source/ast/expressions/MiscExpressions.cpp

@@ -137,7 +137,8 @@ Expression& ValueExpressionBase::fromSymbol(const ASTContext& context, const Sym
             (symbol.kind == SymbolKind::ConstraintBlock && constraintAllowed) ||
             (symbol.kind == SymbolKind::Coverpoint && flags.has(ASTFlags::AllowCoverpoint))) {
             // Special case for event expressions and constraint block built-in methods.
-            return *comp.emplace<ArbitrarySymbolExpression>(symbol, comp.getVoidType(), hierRef,
+            return *comp.emplace<ArbitrarySymbolExpression>(*context.scope, symbol,
+                                                            comp.getVoidType(), hierRef,
                                                             sourceRange);
         }
 
@@ -174,10 +175,13 @@ Expression& ValueExpressionBase::fromSymbol(const ASTContext& context, const Sym
     }
 
     Expression* result;
-    if (hierRef && hierRef->target)
-        result = comp.emplace<HierarchicalValueExpression>(value, *hierRef, sourceRange);
-    else
+    if (hierRef && hierRef->target) {
+        result = comp.emplace<HierarchicalValueExpression>(*context.scope, value, *hierRef,
+                                                           sourceRange);
+    }
+    else {
         result = comp.emplace<NamedValueExpression>(value, sourceRange);
+    }
 
     if (isUnbounded)
         result->type = &comp.getUnboundedType();
@@ -476,12 +480,15 @@ bool NamedValueExpression::checkConstant(EvalContext& context) const {
     return true;
 }
 
-HierarchicalValueExpression::HierarchicalValueExpression(const ValueSymbol& symbol,
+HierarchicalValueExpression::HierarchicalValueExpression(const Scope& scope,
+                                                         const ValueSymbol& symbol,
                                                          const HierarchicalReference& ref,
                                                          SourceRange sourceRange) :
     ValueExpressionBase(ExpressionKind::HierarchicalValue, symbol, sourceRange), ref(ref) {
     SLANG_ASSERT(ref.target == &symbol);
     this->ref.expr = this;
+
+    scope.getCompilation().noteHierarchicalReference(scope, this->ref);
 }
 
 ConstantValue HierarchicalValueExpression::evalImpl(EvalContext& context) const {
@@ -545,14 +552,16 @@ void TypeReferenceExpression::serializeTo(ASTSerializer& serializer) const {
     serializer.write("targetType", targetType);
 }
 
-ArbitrarySymbolExpression::ArbitrarySymbolExpression(const Symbol& symbol, const Type& type,
+ArbitrarySymbolExpression::ArbitrarySymbolExpression(const Scope& scope, const Symbol& symbol,
+                                                     const Type& type,
                                                      const HierarchicalReference* hierRef,
                                                      SourceRange sourceRange) :
     Expression(ExpressionKind::ArbitrarySymbol, type, sourceRange), symbol(&symbol) {
 
     if (hierRef && hierRef->target) {
         this->hierRef = *hierRef;
         this->hierRef.expr = this;
+        scope.getCompilation().noteHierarchicalReference(scope, this->hierRef);
     }
 }
 
@@ -572,8 +581,8 @@ Expression& ArbitrarySymbolExpression::fromSyntax(Compilation& comp, const NameS
     comp.noteReference(*symbol, context.flags.has(ASTFlags::LValue));
 
     auto hierRef = HierarchicalReference::fromLookup(comp, result);
-    return *comp.emplace<ArbitrarySymbolExpression>(*symbol, comp.getVoidType(), &hierRef,
-                                                    syntax.sourceRange());
+    return *comp.emplace<ArbitrarySymbolExpression>(*context.scope, *symbol, comp.getVoidType(),
+                                                    &hierRef, syntax.sourceRange());
 }
 
 void ArbitrarySymbolExpression::serializeTo(ASTSerializer& serializer) const {

source/ast/symbols/InstanceSymbols.cpp

@@ -1305,7 +1305,9 @@ static const AssertionExpr* bindUnknownPortConn(const ASTContext& context,
 
                             auto hierRef = HierarchicalReference::fromLookup(comp, result);
                             auto hre = comp.emplace<ArbitrarySymbolExpression>(
-                                *symbol, comp.getVoidType(), &hierRef, syntax.sourceRange());
+                                *context.scope, *symbol, comp.getVoidType(), &hierRef,
+                                syntax.sourceRange());
+
                             return comp.emplace<SimpleAssertionExpr>(*hre, std::nullopt);
                         }
                     }

source/ast/types/AllTypes.cpp

@@ -1144,6 +1144,7 @@ const Type& VirtualInterfaceType::fromSyntax(const ASTContext& context,
     auto loc = syntax.name.location();
     auto& iface = InstanceSymbol::createVirtual(context, loc, def->as<DefinitionSymbol>(),
                                                 syntax.parameters);
+    comp.noteVirtualIfaceInstance(iface);
 
     const ModportSymbol* modport = nullptr;
     std::string_view modportName = syntax.modport ? syntax.modport->member.valueText() : ""sv;