diff --git a/bindings/python/CompBindings.cpp b/bindings/python/CompBindings.cpp index 2e78dad3c..db929c145 100644 --- a/bindings/python/CompBindings.cpp +++ b/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(&Compilation::getDefinition, - py::const_), - byrefint, "syntax"_a) .def("getDefinitions", &Compilation::getDefinitions, byrefint) .def("getPackage", &Compilation::getPackage, byrefint, "name"_a) .def("getStdPackage", &Compilation::getStdPackage, byrefint) diff --git a/include/slang/ast/Compilation.h b/include/slang/ast/Compilation.h index aff891f95..6f01698f6 100644 --- a/include/slang/ast/Compilation.h +++ b/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 definitionFromSyntax; + flat_hash_map> + 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. diff --git a/include/slang/ast/symbols/InstanceSymbols.h b/include/slang/ast/symbols/InstanceSymbols.h index 73485366e..b43b2b426 100644 --- a/include/slang/ast/symbols/InstanceSymbols.h +++ b/include/slang/ast/symbols/InstanceSymbols.h @@ -90,7 +90,8 @@ class SLANG_EXPORT InstanceSymbol : public InstanceSymbolBase { const syntax::HierarchyInstantiationSyntax& syntax, const ASTContext& context, SmallVectorBase& results, SmallVectorBase& 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, diff --git a/source/ast/Compilation.cpp b/source/ast/Compilation.cpp index 9158c3316..e07112208 100644 --- a/source/ast/Compilation.cpp +++ b/source/ast/Compilation.cpp @@ -683,7 +683,7 @@ Compilation::DefinitionLookupResult Compilation::getDefinition( case SyntaxKind::InterfaceDeclaration: case SyntaxKind::ProgramDeclaration: { auto& mds = bindInfo.instantiationDefSyntax->as(); - 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 scopes; + + SmallMap 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); diff --git a/source/ast/Scope.cpp b/source/ast/Scope.cpp index a46253027..f69553ba1 100644 --- a/source/ast/Scope.cpp +++ b/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); } diff --git a/source/ast/symbols/InstanceSymbols.cpp b/source/ast/symbols/InstanceSymbols.cpp index 832007f9f..a6bbf3ce5 100644 --- a/source/ast/symbols/InstanceSymbols.cpp +++ b/source/ast/symbols/InstanceSymbols.cpp @@ -91,10 +91,11 @@ class InstanceBuilder { public: InstanceBuilder(const ASTContext& context, SmallVectorBase& implicitNets, const HierarchyOverrideNode* parentOverrideNode, - std::span attributes, bool isFromBind) : + std::span 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& implicitNets; SmallVector path; std::span 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 instances; + auto& header = *syntax.header; + auto loc = header.name.location(); + auto instName = comp.emplace(header.name, + std::span()); + auto instance = comp.emplace( + instName, missing(TokenKind::OpenParenthesis, loc), std::span(), + missing(TokenKind::CloseParenthesis, loc)); + + instances.push_back(instance); + + auto instantiation = comp.emplace( + std::span(), header.name, nullptr, instances.copy(comp), + header.semi); + + ASTContext context(scope, LookupLocation::max); + SmallVector results; + SmallVector 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(*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& results, SmallVectorBase& 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. diff --git a/source/ast/symbols/MemberSymbols.cpp b/source/ast/symbols/MemberSymbols.cpp index d3d0a8432..9693e2beb 100644 --- a/source/ast/symbols/MemberSymbols.cpp +++ b/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; } diff --git a/tests/unittests/ast/AssertionTests.cpp b/tests/unittests/ast/AssertionTests.cpp index 5752ee8b5..cec47d966 100644 --- a/tests/unittests/ast/AssertionTests.cpp +++ b/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") { diff --git a/tests/unittests/ast/HierarchyTests.cpp b/tests/unittests/ast/HierarchyTests.cpp index cd58fb84a..2134124c2 100644 --- a/tests/unittests/ast/HierarchyTests.cpp +++ b/tests/unittests/ast/HierarchyTests.cpp @@ -1592,7 +1592,7 @@ endmodule auto& top = compilation.getRoot().lookupName("top").body; auto instances = top.membersOfType(); - 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); +} diff --git a/tests/unittests/parsing/VisitorTests.cpp b/tests/unittests/parsing/VisitorTests.cpp index 3d99d39c1..d8be86cee 100644 --- a/tests/unittests/parsing/VisitorTests.cpp +++ b/tests/unittests/parsing/VisitorTests.cpp @@ -28,7 +28,12 @@ class SemanticModel { else if (syntax.kind == SyntaxKind::ModuleDeclaration || syntax.kind == SyntaxKind::InterfaceDeclaration || syntax.kind == SyntaxKind::ProgramDeclaration) { - auto def = compilation.getDefinition(syntax.as()); + auto [parentScope, parentSym] = getParent(syntax); + if (!parentScope) + parentScope = &compilation.getRoot(); + + auto def = compilation.getDefinition(*parentScope, + syntax.as()); if (!def) return nullptr; @@ -39,13 +44,13 @@ class SemanticModel { } // Otherwise try to find the parent symbol first. - auto parent = syntax.parent ? getDeclaredSymbol(*syntax.parent) : nullptr; - if (!parent) + auto [parentScope, parentSym] = getParent(syntax); + if (!parentSym) return nullptr; // If this is a type alias, unwrap its target type to look at the syntax node. - if (parent->kind == SymbolKind::TypeAlias) { - auto& target = parent->as().targetType.getType(); + if (parentSym->kind == SymbolKind::TypeAlias) { + auto& target = parentSym->as().targetType.getType(); if (target.getSyntax() == &syntax) { symbolCache.emplace(&syntax, &target); return ⌖ @@ -53,13 +58,11 @@ class SemanticModel { return nullptr; } - if (parent->kind == SymbolKind::Instance) - parent = &parent->as().body; - else if (!parent->isScope()) + if (!parentScope) return nullptr; // Search among the parent's children to see if we can find ourself. - for (auto& child : parent->as().members()) { + for (auto& child : parentScope->members()) { if (child.getSyntax() == &syntax) { // We found ourselves, hurray. symbolCache.emplace(&syntax, &child); @@ -116,8 +119,20 @@ class SemanticModel { } private: - Compilation& compilation; + std::pair getParent(const SyntaxNode& syntax) { + auto parent = syntax.parent ? getDeclaredSymbol(*syntax.parent) : nullptr; + if (!parent) + return {nullptr, nullptr}; + + if (parent->kind == SymbolKind::Instance) + parent = &parent->as().body; + else if (!parent->isScope()) + return {nullptr, parent}; + + return {&parent->as(), parent}; + } + Compilation& compilation; flat_hash_map symbolCache; };