Slang incorrectly claims that a parameter is recursively defined

[udif]

Describe the bug
In some places when calling the AST to get a parameter value, it triggers an erroneous error message.

To Reproduce

`define PARAM_DECL parameter X=1,\
                           parameter I=3,\
                           parameter W=I+1\


module t
#(// Parameters
  `PARAM_DECL,
  parameter J=2,
  parameter Y=I+1,
  parameter Z=J+1
)
  (
output [W-1:0]w,
output [Y-1:0]y,
output [X-1:0]z
);
assign w = {W{1'b0}};
endmodule

Run the following with:
slang-hier test.v
Which produces:

Module="t" Instance="t" 
Top level design units:
    t

Build succeeded: 0 errors, 0 warnings

Then run again with:
slang-hier --params test.v
Which produces:

Module="t" Instance="t" Parameters: X=1, I=3, W=4, J=2, Y=4, Z=3
Top level design units:
    t

test.v:14:9: error: 'W' recursively depends on its own definition
output [W-1:0]w,
        ^
test13.v:8:3: note: declared here
  `PARAM_DECL,
  ^
test.v:3:38: note: expanded from macro 'PARAM_DECL'
                           parameter W=I+1\
                                     ^

Build failed: 1 error, 0 warnings

Notice that the error appears only when 2 parameters are both defined in a macro and one depends on the other.
All other test cases, where only one is defined in the macro, do not trigger this.

The error is triggered by the following lines in slang-hier.v on lines 93-94:

                            if (p->symbol.kind == SymbolKind::Parameter)
                                v = p->symbol.as<ParameterSymbol>().getValue().toString();

The assign is in the test because this was an effort to get a minimum sized example to another bug where a parameter on the same code was incorrectly inferred as 0, but instead it triggered this bug I already ran into earlier but didn't have a clue as how to recreate.

[udif]

For the sake of completeness, I edited the original issue to include the expected output when the bug is not triggered, which I forgot to include earlier.

[MikePopoloski]

This is essentially an API bug; the API should not allow you to access the parameter list before the instance body has been elaborated, but currently it does. If you do something like call .members() prior to accessing the parameters things will work correctly. I'll add an accessor that enforces this.

[udif]

I am confused.
I was using the code below, and assumed that by the time driver.createCompilation() returns, everything is elaborated, and I'm merely traversing an already elaborated tree.

    bool ok = driver.parseAllSources();
    auto compilation = driver.createCompilation();
    auto instances = compilation->getRoot().topInstances;
    for (auto& i : instances) {
        i->visit(makeVisitor([&](auto& visitor, const InstanceSymbol& type) {...}));
    }
...

The truth is, I couldn't find any document/tutorial that explains how to use slang as a parser. I could only rely on the tools directory as examples, but apparently that's not enoug.

In sv-lang.com, you have several sections:
User Manual, which more-or-less only talks about using the slang executable, perhaps as a simple syntax checker.
Developer Guide, that has 5 sub-sections. The first is a technical build/install section, the next deal with some subclasses you don't need to know about if you use the driver. only in the last sub-section you mention the SyntaxTree class.
And finally, the API Reference which is way too low level and only serves as a reference material if you already knows what each class does and just needs a reference.

I'm left with so many questions:

1. What is the different between a SyntaxTree and the AST Classes? When do I use one or the other?
2. How do I just parse source, where everything is only compiled and checked for syntax correctness but not fully elaborated yet?
3. And in contrast to the previous section, what do I need to do to start parsing an elaborated tree? Looking at the AST JSON output from slang I see that a fully elaborated design includes both final parameter values, and the expression used to derive it. How does this work for other stuff such as if/for generate? do I see both code paths, those covered by if/generate and the else branch that is not?

[MikePopoloski]

Yeah, it's on my list to write some higher level documentation for the AST constructs. The key points to get you going:

• SyntaxTree represents a "parse tree" or a "concrete syntax tree". It wraps up a lexer / preprocessor / parser for a single compilation unit. Parsing does not involve name resolution, just creating a data structure that mirrors the syntactic structure of the source.
• The AST / "abstract syntax tree" is represented by the Compilation object, symbols, expressions, statements, etc.
• AST nodes are lazily elaborated, so if you just call getRoot() it constructs the top level and returns immediately without creating all child nodes. In order to get all diagnostics you have to visit the whole design, which is what getAllDiagnostics() does. At that point the design is fully elaborated and subsequent visits just reuse the elaborated tree. I don't consider this step parsing; it's doing name resolution, type checking, elaborating generate loops, etc.

[udif]

I just independently confirmed this.

For the sake of the test, I added an additional silent driver.reportCompilation(*compilation, /* quiet */ true); call before my code, and the problems went away (I left the original reportCompilation at the end.
When you look at repotCompilation, you see that when it is in quiet mode, all it does is this code:

    for (auto& diag : compilation.getAllDiagnostics())
        diagEngine.issue(diag);

    bool succeeded = diagEngine.getNumErrors() == 0;

    std::string diagStr = diagClient->getString();
    OS::printE(fmt::format("{}", diagStr));

Apparently, as you wrote, it does more work as it gets the diagnostics messages.
My permanent solution is simply to call it once before my code.

[MikePopoloski]

Fixed in b9d5423