Add error checking and support for nested configs with param overrides

include/slang/ast/symbols/CompilationUnitSymbols.h

@@ -264,9 +264,6 @@ struct SLANG_EXPORT ResolvedConfig {
     /// A list of libraries to use to look up definitions.
     std::span<const SourceLibrary* const> liblist;
 
-    /// The original rule that was resolved.
-    const ConfigRule* configRule = nullptr;
-
     ResolvedConfig(const ConfigBlockSymbol& useConfig, const InstanceSymbol& rootInstance);
 };
 

scripts/diagnostics.txt

@@ -1019,6 +1019,8 @@ error MissingExternModuleImpl "missing implementation for extern {} '{}'"
 error ExternDeclMismatchImpl "extern {} '{}' does not match implementation"
 error ExternDeclMismatchPrev "extern {} '{}' does not match previous extern definition"
 error UnknownLibrary "unknown library '{}'"
+error NestedConfigMultipleTops "non-top config '{}' has more than one top cell specified"
+error ConfigParamsIgnored "parameter overrides provided by config rule are ignored because the target instance is using a different nested config '{}'"
 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

@@ -2349,11 +2349,24 @@ std::pair<Compilation::DefinitionLookupResult, bool> Compilation::resolveConfigR
                 if (result) {
                     auto topCells = result->getTopCells();
                     if (topCells.size() != 1) {
-                        // TODO: error
+                        auto syntax = result->getSyntax();
+                        SLANG_ASSERT(syntax);
+
+                        auto range = syntax->as<ConfigDeclarationSyntax>().topCells.sourceRange();
+                        auto& diag = scope.addDiag(diag::NestedConfigMultipleTops, range);
+                        diag << result->name;
+                        diag.addNote(diag::NoteConfigRule, rule->sourceRange);
+
                         return {{}, true};
                     }
 
-                    return {{&topCells[0].definition, result, nullptr}, true};
+                    if (rule->paramOverrides) {
+                        scope.addDiag(diag::ConfigParamsIgnored,
+                                      rule->paramOverrides->sourceRange())
+                            << result->name;
+                    }
+
+                    return {{&topCells[0].definition, result, topCells[0].rule}, true};
                 }
             }
 

source/ast/symbols/CompilationUnitSymbols.cpp

@@ -445,8 +445,13 @@ void ConfigBlockSymbol::resolve() const {
                 curRule.paramOverrides = newRule.paramOverrides;
             if (newRule.liblist)
                 curRule.liblist = newRule.liblist;
-            if (!newRule.useCell.name.empty())
+            if (!newRule.useCell.name.empty()) {
+                // Note: prefer the source range to come from the rule that
+                // supplies a use cell name, to make other reported errors easier
+                // to understand.
                 curRule.useCell = newRule.useCell;
+                curRule.sourceRange = newRule.sourceRange;
+            }
         }
     };
 

source/ast/symbols/InstanceSymbols.cpp

@@ -59,7 +59,7 @@ class InstanceBuilder {
                     std::span<const AttributeInstanceSyntax* const> attributes,
                     const ResolvedConfig* resolvedConfig, const ConfigBlockSymbol* newConfigRoot,
                     bool isFromBind) :
-        compilation(context.getCompilation()),
+        comp(context.getCompilation()),
         context(context), definition(definition), paramBuilder(paramBuilder),
         parentOverrideNode(parentOverrideNode), attributes(attributes),
         resolvedConfig(resolvedConfig), newConfigRoot(newConfigRoot), isFromBind(isFromBind) {}
@@ -92,7 +92,7 @@ class InstanceBuilder {
 private:
     using DimIterator = std::span<VariableDimensionSyntax*>::iterator;
 
-    Compilation& compilation;
+    Compilation& comp;
     const ASTContext& context;
     const DefinitionSymbol& definition;
     SmallVector<int32_t> path;
@@ -107,22 +107,17 @@ class InstanceBuilder {
                            const HierarchyOverrideNode* overrideNode) {
         paramBuilder.setOverrides(overrideNode);
         auto [name, loc] = getNameLoc(syntax);
-        auto inst = compilation.emplace<InstanceSymbol>(compilation, name, loc, definition,
-                                                        paramBuilder, /* isUninstantiated */ false,
-                                                        isFromBind);
-        inst->arrayPath = path.copy(compilation);
+        auto inst = comp.emplace<InstanceSymbol>(comp, name, loc, definition, paramBuilder,
+                                                 /* isUninstantiated */ false, isFromBind);
+        inst->arrayPath = path.copy(comp);
         inst->setSyntax(syntax);
         inst->setAttributes(*context.scope, attributes);
 
         if (resolvedConfig) {
-            if (newConfigRoot) {
-                auto rc = compilation.emplace<ResolvedConfig>(*newConfigRoot, *inst);
-                rc->configRule = resolvedConfig->configRule;
-                inst->resolvedConfig = rc;
-            }
-            else {
+            if (newConfigRoot)
+                inst->resolvedConfig = comp.emplace<ResolvedConfig>(*newConfigRoot, *inst);
+            else
                 inst->resolvedConfig = resolvedConfig;
-            }
         }
 
         return inst;
@@ -136,10 +131,10 @@ class InstanceBuilder {
         SLANG_ASSERT(syntax.decl);
         auto nameToken = syntax.decl->name;
         auto createEmpty = [&]() {
-            return compilation.emplace<InstanceArraySymbol>(compilation, nameToken.valueText(),
-                                                            nameToken.location(),
-                                                            std::span<const Symbol* const>{},
-                                                            ConstantRange());
+            return comp.emplace<InstanceArraySymbol>(comp, nameToken.valueText(),
+                                                     nameToken.location(),
+                                                     std::span<const Symbol* const>{},
+                                                     ConstantRange());
         };
 
         auto& dimSyntax = **it;
@@ -153,9 +148,9 @@ class InstanceBuilder {
             return createEmpty();
 
         ConstantRange range = dim.range;
-        if (range.width() > compilation.getOptions().maxInstanceArray) {
+        if (range.width() > comp.getOptions().maxInstanceArray) {
             auto& diag = context.addDiag(diag::MaxInstanceArrayExceeded, dimSyntax.sourceRange());
-            diag << definition.getKindString() << compilation.getOptions().maxInstanceArray;
+            diag << definition.getKindString() << comp.getOptions().maxInstanceArray;
             return createEmpty();
         }
 
@@ -176,9 +171,9 @@ class InstanceBuilder {
             elements.push_back(symbol);
         }
 
-        auto result = compilation.emplace<InstanceArraySymbol>(compilation, nameToken.valueText(),
-                                                               nameToken.location(),
-                                                               elements.copy(compilation), range);
+        auto result = comp.emplace<InstanceArraySymbol>(comp, nameToken.valueText(),
+                                                        nameToken.location(), elements.copy(comp),
+                                                        range);
         result->setSyntax(syntax);
 
         for (auto element : elements)
@@ -528,7 +523,6 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
         if (confRule) {
             SLANG_ASSERT(resolvedConfig);
             auto rc = comp.emplace<ResolvedConfig>(*resolvedConfig);
-            rc->configRule = confRule;
             if (confRule->liblist)
                 rc->liblist = *confRule->liblist;
             localConfig = rc;

tests/unittests/ast/ConfigTests.cpp

@@ -511,7 +511,7 @@ endmodule
     CHECK(barA.getDefinition().name == "m2");
 }
 
-TEST_CASE("Config warning cases") {
+TEST_CASE("Config warning / error cases") {
     auto tree = SyntaxTree::fromText(R"(
 config cfg1;
     design top top;
@@ -521,9 +521,25 @@ config cfg1;
     instance top.foo use lib1.c;
     instance top.foo use d;
     instance foo.bar use e;
+    instance top.blah1 use qq;
+    instance top.blah2 use rr;
+    instance top.blah2 use #(.A(1));
+endconfig
+
+module a; endmodule
+module b; endmodule
+
+config qq;
+    design a b;
+endconfig
+
+config rr;
+    design a;
 endconfig
 
 module top;
+    asdf blah1();
+    asdf blah2();
 endmodule
 )");
     CompilationOptions options;
@@ -533,12 +549,14 @@ endmodule
     compilation.addSyntaxTree(tree);
 
     auto& diags = compilation.getAllDiagnostics();
-    REQUIRE(diags.size() == 5);
+    REQUIRE(diags.size() == 7);
     CHECK(diags[0].code == diag::ConfigDupTop);
     CHECK(diags[1].code == diag::WarnUnknownLibrary);
     CHECK(diags[2].code == diag::DupConfigRule);
     CHECK(diags[3].code == diag::DupConfigRule);
     CHECK(diags[4].code == diag::ConfigInstanceWrongTop);
+    CHECK(diags[5].code == diag::ConfigParamsIgnored);
+    CHECK(diags[6].code == diag::NestedConfigMultipleTops);
 }
 
 TEST_CASE("Config rules with param overrides") {
@@ -667,3 +685,37 @@ endmodule
     CHECK(getParam("top.A").integer() == 1);
     CHECK(getParam("top.foo.B").integer() == 2);
 }
+
+TEST_CASE("Config param overrides with nested config path") {
+    auto tree = SyntaxTree::fromText(R"(
+config cfg1;
+    design top;
+    cell f use cfg2;
+endconfig
+
+config cfg2;
+    design f;
+    cell f use #(.B(5));
+endconfig
+
+module f #(parameter int B);
+endmodule
+
+module top;
+    f foo();
+endmodule
+)");
+    CompilationOptions options;
+    options.topModules.emplace("cfg1");
+
+    Compilation compilation(options);
+    compilation.addSyntaxTree(tree);
+    NO_COMPILATION_ERRORS;
+
+    auto getParam = [&](std::string_view name) {
+        auto& param = compilation.getRoot().lookupName<ParameterSymbol>(name);
+        return param.getValue();
+    };
+
+    CHECK(getParam("top.foo.B").integer() == 5);
+}