MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
558 stars 122 forks source link

Allow post-elaboration diagnostics with missing modules #875

Closed suzizecat closed 2 months ago

suzizecat commented 5 months ago

Is your feature request related to a problem? Please describe. As of today, in order to get unused/undriven/... warnings, it is mandatory to have no error during compilation/elaboration. Therefore, when a module is missing a choice must be made between:

  1. Not having undriven warnings as long as a module is missing.
  2. Having the undriven warnings without the missing module error (using the slang::ast::CompilationFlags::IgnoreUnknownModules flag).

Describe the solution you'd like I would like to have both the undriven reports where relevant while keeping the missing module error in the report.

Describe alternatives you've considered I considered running the PostElabVisitor manually, but there might be elaboration error which would make it fail and I am not aware of what to look for.

I could also use solution 2 and add a custom visitor which lookup unbound modules and manually emit a custom "missing module" error.

For now, I settled on solution 1 but it is sub-optimal IMHO.

Additional context This is used in the context of a language server. It is always surprising when fixing an error makes more warnings appear. Moreover, the missing module doesn't seem to hinder the behavior of the PostElabVisitor.

Note, as the condition preventing the PostElabVisitor from running is

if (!elabVisitor.hierarchyProblem && numErrors == 0)

(from Compilation.cpp:1411) An this feature may require separating "critical" errors (that prevent the post-elaboration) from the simple errors that are not an issue.

MikePopoloski commented 5 months ago

This is doable, but my thinking was that the missing module may be the thing that provides drivers for the otherwise unused variables. For example:

module m;
    int foo;
    n n1();
endmodule
module n;
    assign m.foo = 1;
endmodule

If you don't provide the definition of n then you'd get a warning about it being undriven. Perhaps it's unlikely and you'd rather just see those warnings anyway.

suzizecat commented 5 months ago

It's only my opinion, but it wouldn't surprise me to see an undriven for m.foo in your example. Moreover, I never encountered such code structure (which is beside the point, but still). I almost never drive an internal net of a sub-module directly (m.foo) let alone the net of a parent. (I would always go through ports). This aside, I am working on design and not in verification, so I might be unaware of some use cases.

However it seems to me that the error about missing module is fairly important while it shouldn't impact diagnostic of a file that is totally unrelated.

In example:

module top()
a a1(); // Missing definition
b b1();
endmodule
module b()
logic foo;
endmodule

I would expect to see the undriven status of b.foo regardless of what is happening in top (and would expect a message about missing definition of a)

MikePopoloski commented 4 months ago

Yeah, I think that's fair. I'll consider it further.

MikePopoloski commented 2 months ago

I ended up just removing the restriction in 093dc45769d90fc50dbc02aed97f99c0ac225621

We'll see if I get issue reports for the opposite problem now, where people are annoyed about follow up warnings after errors.