Fix handling of nested modules, fix bind directives underneath uninst…

…antiated generate blocks

bindings/python/CompBindings.cpp

@@ -105,10 +105,6 @@ void registerCompilation(py::module_& m) {
         .def("getCompilationUnits", &Compilation::getCompilationUnits, byrefint)
         .def("getSourceLibrary", &Compilation::getSourceLibrary, byrefint, "name"_a)
         .def("tryGetDefinition", &Compilation::tryGetDefinition, byrefint, "name"_a, "scope"_a)
-        .def("getDefinition",
-             py::overload_cast<const ModuleDeclarationSyntax&>(&Compilation::getDefinition,
-                                                               py::const_),
-             byrefint, "syntax"_a)
         .def("getDefinitions", &Compilation::getDefinitions, byrefint)
         .def("getPackage", &Compilation::getPackage, byrefint, "name"_a)
         .def("getStdPackage", &Compilation::getStdPackage, byrefint)

include/slang/ast/Compilation.h

@@ -389,7 +389,8 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
                                          const BindDirectiveInfo& bindInfo) const;
 
     /// Gets the definition for the given syntax node, or nullptr if it does not exist.
-    const DefinitionSymbol* getDefinition(const syntax::ModuleDeclarationSyntax& syntax) const;
+    const DefinitionSymbol* getDefinition(const Scope& scope,
+                                          const syntax::ModuleDeclarationSyntax& syntax) const;
 
     /// Gets the definition indicated by the given config and cell ID, or nullptr
     /// if it does not exist. If no definition is found an appropriate diagnostic will be issued.
@@ -903,7 +904,8 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
 
     // A map from syntax node to the definition it represents. Used much less frequently
     // than other ways of looking up definitions which is why it's lower down here.
-    flat_hash_map<const syntax::ModuleDeclarationSyntax*, DefinitionSymbol*> definitionFromSyntax;
+    flat_hash_map<const syntax::ModuleDeclarationSyntax*, std::vector<DefinitionSymbol*>>
+        definitionFromSyntax;
 
     // A set of all instantiated names in the design; used for determining whether a given
     // module has ever been instantiated to know whether it should be considered top-level.

include/slang/ast/symbols/InstanceSymbols.h

@@ -90,7 +90,8 @@ class SLANG_EXPORT InstanceSymbol : public InstanceSymbolBase {
                            const syntax::HierarchyInstantiationSyntax& syntax,
                            const ASTContext& context, SmallVectorBase<const Symbol*>& results,
                            SmallVectorBase<const Symbol*>& implicitNets,
-                           const BindDirectiveInfo* bindInfo = nullptr);
+                           const BindDirectiveInfo* bindInfo = nullptr,
+                           const syntax::SyntaxNode* overrideSyntax = nullptr);
 
     static void fromFixupSyntax(Compilation& compilation, const DefinitionSymbol& definition,
                                 const syntax::DataDeclarationSyntax& syntax,
@@ -109,6 +110,10 @@ class SLANG_EXPORT InstanceSymbol : public InstanceSymbolBase {
         const ASTContext& context, SourceLocation loc, const DefinitionSymbol& definition,
         const syntax::ParameterValueAssignmentSyntax* paramAssignments);
 
+    /// Creates a default-instantiated instance of a nested definition in the provided scope.
+    static Symbol& createDefaultNested(const Scope& scope,
+                                       const syntax::ModuleDeclarationSyntax& syntax);
+
     /// Creates an intentionally invalid instance by forcing all parameters to null values.
     /// This allows type checking instance members as long as they don't depend on any parameters.
     static InstanceSymbol& createInvalid(Compilation& compilation,

source/ast/Compilation.cpp

@@ -683,7 +683,7 @@ Compilation::DefinitionLookupResult Compilation::getDefinition(
             case SyntaxKind::InterfaceDeclaration:
             case SyntaxKind::ProgramDeclaration: {
                 auto& mds = bindInfo.instantiationDefSyntax->as<ModuleDeclarationSyntax>();
-                result.definition = getDefinition(mds);
+                result.definition = getDefinition(scope, mds);
                 if (!result.definition)
                     errorMissingDef(name, scope, sourceRange, diag::UnknownModule);
                 break;
@@ -710,12 +710,32 @@ Compilation::DefinitionLookupResult Compilation::getDefinition(
     return result;
 }
 
-const DefinitionSymbol* Compilation::getDefinition(const ModuleDeclarationSyntax& syntax) const {
+const DefinitionSymbol* Compilation::getDefinition(const Scope& scope,
+                                                   const ModuleDeclarationSyntax& syntax) const {
     if (auto it = definitionFromSyntax.find(&syntax); it != definitionFromSyntax.end()) {
+        SmallSet<const Scope*, 4> scopes;
+
+        SmallMap<const Scope*, const DefinitionSymbol*, 4> scopeMap;
+        for (auto def : it->second) {
+            auto insertScope = def->getParentScope();
+            if (insertScope && insertScope->asSymbol().kind == SymbolKind::CompilationUnit)
+                insertScope = root.get();
+
+            scopeMap[insertScope] = def;
+        }
+
+        auto lookupScope = &scope;
+        do {
+            if (auto scopeIt = scopeMap.find(lookupScope); scopeIt != scopeMap.end())
+                return scopeIt->second;
+
+            lookupScope = lookupScope->asSymbol().getParentScope();
+        } while (lookupScope);
+
         // If this definition is no longer referenced by the definitionMap
         // it probably got booted by an (illegal) duplicate definition.
         // We don't want to return anything in that case.
-        auto def = it->second;
+        /*auto def = it->second;
         auto& defScope = *def->getParentScope();
         auto targetScope = defScope.asSymbol().kind == SymbolKind::CompilationUnit ? root.get()
                                                                                    : &defScope;
@@ -724,7 +744,7 @@ const DefinitionSymbol* Compilation::getDefinition(const ModuleDeclarationSyntax
         if (dmIt != definitionMap.end() &&
             std::ranges::find(dmIt->second.first, def) != dmIt->second.first.end()) {
             return def;
-        }
+        }*/
     }
     return nullptr;
 }
@@ -825,7 +845,7 @@ void Compilation::createDefinition(const Scope& scope, LookupLocation location,
                        scope, location, syntax, *metadata.defaultNetType, metadata.unconnectedDrive,
                        metadata.timeScale, metadata.tree))
                    .get();
-    definitionFromSyntax[&syntax] = def;
+    definitionFromSyntax[&syntax].push_back(def);
 
     insertDefinition(*def, scope);
 
@@ -1115,7 +1135,8 @@ const Symbol* Compilation::findPackageExportCandidate(const PackageSymbol& packa
 }
 
 void Compilation::noteBindDirective(const BindDirectiveSyntax& syntax, const Scope& scope) {
-    bindDirectives.emplace_back(&syntax, &scope);
+    if (!scope.isUninstantiated())
+        bindDirectives.emplace_back(&syntax, &scope);
 }
 
 void Compilation::noteInstanceWithDefBind(const Symbol& instance) {
@@ -2216,9 +2237,10 @@ void Compilation::resolveDefParamsAndBinds() {
 
         for (auto& entry : binds) {
             if (entry.definitionTarget) {
+                // TODO: fix when there are multiple defs
                 auto it = c.definitionFromSyntax.find(entry.definitionTarget);
-                SLANG_ASSERT(it != c.definitionFromSyntax.end());
-                it->second->bindDirectives.push_back(entry.info);
+                SLANG_ASSERT(it != c.definitionFromSyntax.end() && !it->second.empty());
+                it->second.front()->bindDirectives.push_back(entry.info);
             }
             else {
                 auto node = getNodeFor(entry.path, c);

source/ast/Scope.cpp

@@ -1342,7 +1342,7 @@ void Scope::handleUserDefinedNet(const UserDefinedNetDeclarationSyntax& syntax)
 }
 
 void Scope::handleNestedDefinition(const ModuleDeclarationSyntax& syntax) const {
-    // We implicitly instantiated this definition if it has no parameters
+    // We implicitly instantiate this definition if it has no parameters
     // and no ports and hasn't been instantiated elsewhere.
     auto& header = *syntax.header;
     if (header.parameters && !header.parameters->declarations.empty())
@@ -1362,13 +1362,11 @@ void Scope::handleNestedDefinition(const ModuleDeclarationSyntax& syntax) const
         }
     }
 
-    // This can return nullptr if we had more than one nested declaration
-    // with the same name (so only one got stored in the compilation's map).
-    auto def = compilation.getDefinition(syntax);
+    auto def = compilation.getDefinition(*this, syntax);
     if (!def || def->isInstantiated())
         return;
 
-    auto& inst = InstanceSymbol::createDefault(compilation, *def);
+    auto& inst = InstanceSymbol::createDefaultNested(*this, syntax);
     insertMember(&inst, lastMember, /* isElaborating */ true, /* incrementIndex */ true);
 }
 

source/ast/symbols/InstanceSymbols.cpp

@@ -91,10 +91,11 @@ class InstanceBuilder {
 public:
     InstanceBuilder(const ASTContext& context, SmallVectorBase<const Symbol*>& implicitNets,
                     const HierarchyOverrideNode* parentOverrideNode,
-                    std::span<const AttributeInstanceSyntax* const> attributes, bool isFromBind) :
+                    std::span<const AttributeInstanceSyntax* const> attributes, bool isFromBind,
+                    const SyntaxNode* overrideSyntax) :
         netType(context.scope->getDefaultNetType()), comp(context.getCompilation()),
-        context(context), parentOverrideNode(parentOverrideNode), implicitNets(implicitNets),
-        attributes(attributes), isFromBind(isFromBind) {}
+        context(context), parentOverrideNode(parentOverrideNode), overrideSyntax(overrideSyntax),
+        implicitNets(implicitNets), attributes(attributes), isFromBind(isFromBind) {}
 
     // Resets the builder to be ready to create more instances with different settings.
     // Must be called at least once prior to creating instances.
@@ -119,7 +120,8 @@ class InstanceBuilder {
 
         const HierarchyOverrideNode* overrideNode = nullptr;
         if (parentOverrideNode) {
-            if (auto sit = parentOverrideNode->childNodes.find(syntax);
+            if (auto sit = parentOverrideNode->childNodes.find(overrideSyntax ? *overrideSyntax
+                                                                              : syntax);
                 sit != parentOverrideNode->childNodes.end()) {
                 overrideNode = &sit->second;
             }
@@ -142,6 +144,7 @@ class InstanceBuilder {
     const HierarchyOverrideNode* parentOverrideNode = nullptr;
     const ResolvedConfig* resolvedConfig = nullptr;
     const ConfigBlockSymbol* newConfigRoot = nullptr;
+    const SyntaxNode* overrideSyntax;
     SmallVectorBase<const Symbol*>& implicitNets;
     SmallVector<int32_t> path;
     std::span<const AttributeInstanceSyntax* const> attributes;
@@ -456,6 +459,48 @@ InstanceSymbol& InstanceSymbol::createVirtual(
     return result;
 }
 
+Symbol& InstanceSymbol::createDefaultNested(const Scope& scope,
+                                            const ModuleDeclarationSyntax& syntax) {
+    auto& comp = scope.getCompilation();
+    auto missing = [&](TokenKind tk, SourceLocation loc) {
+        return Token::createMissing(comp, tk, loc);
+    };
+
+    // Fabricate a fake instantiation syntax to let us reuse all of the real logic
+    // with this nested module case as well.
+    SmallVector<TokenOrSyntax, 4> instances;
+    auto& header = *syntax.header;
+    auto loc = header.name.location();
+    auto instName = comp.emplace<InstanceNameSyntax>(header.name,
+                                                     std::span<VariableDimensionSyntax*>());
+    auto instance = comp.emplace<HierarchicalInstanceSyntax>(
+        instName, missing(TokenKind::OpenParenthesis, loc), std::span<TokenOrSyntax>(),
+        missing(TokenKind::CloseParenthesis, loc));
+
+    instances.push_back(instance);
+
+    auto instantiation = comp.emplace<HierarchyInstantiationSyntax>(
+        std::span<AttributeInstanceSyntax*>(), header.name, nullptr, instances.copy(comp),
+        header.semi);
+
+    ASTContext context(scope, LookupLocation::max);
+    SmallVector<const Symbol*> results;
+    SmallVector<const Symbol*> implicitNets;
+    fromSyntax(comp, *instantiation, context, results, implicitNets, nullptr, &syntax);
+    SLANG_ASSERT(implicitNets.empty());
+    SLANG_ASSERT(results.size() == 1);
+
+    // We just created this symbol; it's safe to const cast it here.
+    auto& result = const_cast<Symbol&>(*results[0]);
+
+    // Swap the syntax back to our original definition syntax
+    // so that it's findable in bind directives via the original
+    // syntax and not one we just made up.
+    if (result.kind == SymbolKind::Instance)
+        result.setSyntax(syntax);
+    return result;
+}
+
 InstanceSymbol& InstanceSymbol::createInvalid(Compilation& compilation,
                                               const DefinitionSymbol& definition) {
     // Give this instance an empty name so that it can't be referenced by name.
@@ -468,7 +513,8 @@ InstanceSymbol& InstanceSymbol::createInvalid(Compilation& compilation,
 void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationSyntax& syntax,
                                 const ASTContext& context, SmallVectorBase<const Symbol*>& results,
                                 SmallVectorBase<const Symbol*>& implicitNets,
-                                const BindDirectiveInfo* bindInfo) {
+                                const BindDirectiveInfo* bindInfo,
+                                const SyntaxNode* overrideSyntax) {
     auto defName = syntax.type.valueText();
     TimeTraceScope timeScope("createInstances"sv, [&] { return std::string(defName); });
 
@@ -550,7 +596,7 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
     }
 
     InstanceBuilder builder(context, implicitNets, parentOverrideNode, syntax.attributes,
-                            isFromBind);
+                            isFromBind, overrideSyntax);
 
     // Creates instance symbols -- if specificInstance is provided then only that
     // instance will be created, otherwise all instances in the original syntax
@@ -807,7 +853,7 @@ void InstanceSymbol::resolvePortConnections() const {
     connectionMap = comp.allocPointerMap();
 
     auto syntax = getSyntax();
-    if (!syntax) {
+    if (!syntax || syntax->kind != SyntaxKind::HierarchicalInstance) {
         // If this is a top level module and we have interface ports, the user has
         // the option of allowing it by automatically instantiating interface instances
         // to connect them to.

source/ast/symbols/MemberSymbols.cpp

@@ -1216,7 +1216,7 @@ PrimitiveSymbol& PrimitiveSymbol::fromSyntax(const Scope& scope,
         }
 
         for (auto port : ports) {
-            if (!port->getSyntax()) {
+            if (!port->getSyntax() && !port->name.empty()) {
                 auto& diag = scope.addDiag(diag::PrimitivePortMissing, port->location);
                 diag << port->name;
             }

tests/unittests/ast/AssertionTests.cpp

@@ -1616,11 +1616,12 @@ endmodule
     compilation.addSyntaxTree(tree);
 
     auto& diags = compilation.getAllDiagnostics();
-    REQUIRE(diags.size() == 4);
+    REQUIRE(diags.size() == 5);
     CHECK(diags[0].code == diag::NotAllowedInChecker);
-    CHECK(diags[1].code == diag::NotAllowedInChecker);
-    CHECK(diags[2].code == diag::InvalidInstanceForParent);
-    CHECK(diags[3].code == diag::UndeclaredIdentifier);
+    CHECK(diags[1].code == diag::InvalidInstanceForParent);
+    CHECK(diags[2].code == diag::NotAllowedInChecker);
+    CHECK(diags[3].code == diag::InvalidInstanceForParent);
+    CHECK(diags[4].code == diag::UndeclaredIdentifier);
 }
 
 TEST_CASE("Upward lookup from checkers") {

tests/unittests/ast/HierarchyTests.cpp

@@ -1592,7 +1592,7 @@ endmodule
 
     auto& top = compilation.getRoot().lookupName<InstanceSymbol>("top").body;
     auto instances = top.membersOfType<InstanceSymbol>();
-    REQUIRE(std::ranges::equal(instances, std::array{"i1", "i3", "ff2"}, {}, &Symbol::name));
+    REQUIRE(std::ranges::equal(instances, std::array{"i1", "i3", "ff2", "ff1"}, {}, &Symbol::name));
 }
 
 TEST_CASE("Instance array size limits") {
@@ -2301,3 +2301,73 @@ endprimitive
     REQUIRE(cus.size() == 1);
     CHECK(cus[0]->members().front().name == "p");
 }
+
+TEST_CASE("Nested modules multi-driven regress") {
+    auto tree = SyntaxTree::fromText(R"(
+module m;
+    int foo;
+    module n;
+        assign foo = 1;
+    endmodule
+endmodule
+
+module top;
+    m m1();
+    m m2();
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+    NO_COMPILATION_ERRORS;
+}
+
+TEST_CASE("Nested modules with infinite recursion regress") {
+    auto tree = SyntaxTree::fromText(R"(
+ interface I;
+     I d;
+     module n;
+        g g;
+     endmodule
+ endinterface
+
+ module top;
+     I i();
+ endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+
+    // Check that it doesn't crash.
+    compilation.getAllDiagnostics();
+}
+
+TEST_CASE("Nested modules with binds, parameterized, info task") {
+    auto tree = SyntaxTree::fromText(R"(
+module m #(parameter P);
+  module n;
+    int i;
+  endmodule
+  if (P == 2) begin
+  	bind n foo f();
+  end
+endmodule
+
+module foo;
+  $info("%m");
+endmodule
+
+module top;
+  m #(1) m1();
+  m #(2) m2();
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 1);
+    CHECK(diags[0].code == diag::InfoTask);
+}