Allow redefining modules using a new --allow-redefinition flag

[udif]

Here is my first trial of a PR to handle https://github.com/MikePopoloski/slang/issues/600#issuecomment-1232131424 Other than the boilerplate flags code, I'm not sure this code is 100%, even though it works for my toy example:

Given the 3 test files from #600 :

// a.v
module a;
b b_inst (.x(1'b0));
endmodule

// b0.v
module b;
endmodule

// b1.v
module b(input x);
endmodule

Without the new flag:

:~/git/slang/build$ bin/slang  b0.v a.v b1.v
Top level design units:
    a

a.v:2:12: error: port 'x' does not exist in 'b'
b b_inst (.x(1'b0));
           ^
b1.v:1:8: error: redefinition of 'b'
module b(input x);
       ^
b0.v:1:8: note: previous definition here
module b;
       ^

Build failed: 2 errors, 0 warnings

With the new flag:

~/git/slang/build$ bin/slang --allow-redefinition b0.v a.v b1.v
Top level design units:
    a

Build succeeded: 0 errors, 0 warnings

Comparing AST's reveals the output from bin/slang --allow-redefinition b0.v a.v b1.v and bin/slang a.v b1.v is almost identical: Other than address changes, we have 1 extra compilationUnit , but other that, the JSONs are identical.

[udif]

I left this as draft PR because I feel less confident about this PR. Too many places where this might break things by mistake.

[codecov[bot]]

Codecov Report

Merging #601 (6b08997) into master (39a6be7) will decrease coverage by 0.00%. The diff coverage is 100.00%.

Additional details and impacted files

[](https://codecov.io/gh/MikePopoloski/slang/pull/601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) ```diff @@ Coverage Diff @@ ## master #601 +/- ## ========================================== - Coverage 89.37% 89.36% -0.01% ========================================== Files 187 187 Lines 45317 45313 -4 ========================================== - Hits 40500 40496 -4 Misses 4817 4817 ``` | [Impacted Files](https://codecov.io/gh/MikePopoloski/slang/pull/601?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) | Coverage Δ | | |---|---|---| | [source/compilation/Compilation.cpp](https://codecov.io/gh/MikePopoloski/slang/pull/601/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski#diff-c291cmNlL2NvbXBpbGF0aW9uL0NvbXBpbGF0aW9uLmNwcA==) | `95.28% <100.00%> (-0.03%)` | :arrow_down: | | [source/driver/Driver.cpp](https://codecov.io/gh/MikePopoloski/slang/pull/601/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski#diff-c291cmNlL2RyaXZlci9Ecml2ZXIuY3Bw) | `99.41% <100.00%> (+<0.01%)` | :arrow_up: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/MikePopoloski/slang/pull/601?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/MikePopoloski/slang/pull/601?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). Last update [39a6be7...6b08997](https://codecov.io/gh/MikePopoloski/slang/pull/601?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski).

[udif]

The new commit changes the behavior so that while redefinition is allowed on --allow-redefinition , it is still issued as a warning, and not completely silent. This warning can be turned off by -Wno-redefinition, but no hint about that is given.

In order to achieve this commit , I had to remove the const on optionMap generated by diagnostic_gen.py . You probably won't like it, but I wanted to have a message that is an error if redefinition is not allowed (default), a warning if redefinition is allowed, and if it is a warning, it can also be turned off completely.

You will probably have comments on this PR, which is why it was issued as a DRAFT in the first place, and no tests were written yet.

[MikePopoloski]

You're right, I don't love the changes to the optionsMap. I think you should do what I recently did in https://github.com/MikePopoloski/slang/commit/84060b2aa63a184d93065c0f23ff4b9aa4a51e76 for out of bounds selections. You make the diagnostic a warning in diagnostics.txt, inside Driver you promote the severity to an Error unless some other option (in this case allow-redefinition) says otherwise.

Other than that I think this change is fairly straightforward and unlikely to cause problems. It might be possible for people using this feature to have some confusing things happen, if some code creates a definition and immediately looks it up, and then later someone overwrites the definition. Nothing will crash, they are separate module definitions in memory, but they will have the same name and might not be otherwise distinguishable to the user. But that seems par for the course if you're using this feature in the first place.

[udif]

You're right, I don't love the changes to the optionsMap. I think you should do what I recently did in 84060b2 for out of bounds selections. You make the diagnostic a warning in diagnostics.txt, inside Driver you promote the severity to an Error unless some other option (in this case allow-redefinition) says otherwise.

I did this. changed the error to warning in diagnostics.txt, and it works . However, the error is accompanied by a warning flag option ( [-Wredefinition] ):

:~/git/slang/build$ bin/slang  b0.v a.v b1.v
Top level design units:
    a

a.v:2:12: error: port 'x' does not exist in 'b'
b b_inst (.x(1'b0));
           ^
b1.v:1:8: error: redefinition of 'b' [-Wredefinition]
module b(input x);
       ^
b0.v:1:8: note: previous definition here
module b;
       ^

Build failed: 2 errors, 0 warnings

Adding -Wno-redefinition removes the warning even though this is an error and not a warning.

~/git/slang/build$ bin/slang -Wno-redefinition b0.v a.v b1.v
Top level design units:
    a

a.v:2:12: error: port 'x' does not exist in 'b'
b b_inst (.x(1'b0));
           ^

Build failed: 1 error, 0 warnings

I would expect an error class message not to have a -W option.

[MikePopoloski]

Warnings can be promoted to error severity with -Werror= and demoted from error severity with -Wno-error= so it makes sense to me that it would show the warning option name, as it's a valid target for changing the severity. It does occur to me though that we should separate this out to a separate warning specifically for module/interface/program redefinition, as that error is used for other kinds of redefinition that we don't want suppressible. The naming here is tough as I call these things "Definitions" internally but there isn't a good standard name for them.

[MikePopoloski]

In fact if you make the handling in createDefinition always take the newer rather than the older definition in the case of a conflict then you don't even need a separate option. It's sufficient for the "warning" to be an error by default and downgradable by the user if desired.

[udif]

Warnings can be promoted to error severity with -Werror= and demoted from error severity with -Wno-error= so it makes sense to me that it would show the warning option name, as it's a valid target for changing the severity

The problem above is that an error is removed with -Wno-redefinition, not with -Wno-error= . I think -Wno-XXX should not demote an error, only a warning. maybe change findAndSet() to take an additional parameter specifying the existing severity.

Also, I think Wno-error might be broken:

        else if (startsWith(arg, "error=")) {
            findAndSet(arg.substr(6), DiagnosticSeverity::Error, "-Werror=");
        }
        else if (startsWith(arg, "no-error=")) {
            findAndSet(arg.substr(9), DiagnosticSeverity::Error, "-Wno-error=");
        }
        else if (startsWith(arg, "no-")) {
            findAndSet(arg.substr(3), DiagnosticSeverity::Ignored, "-Wno-");
        }

I would expect:

-Wxxx	set xxx to be a warning.
-Werror=xxx	set xxx to be an error.
-Wno-error=xxx	set xxx to be ignored.
-Wno-xxx	sets xxx to be ignored if it is already a warning

[MikePopoloski]

Yes, I discovered and fixed that issue yesterday. Please pull the latest changes.

I guess it's arguable whether a command line like -Werror=foo -Wno-foo should still report the foo error or not. C++ compilers (i.e. gcc and clang) take the opinion that it should work the way it does with slang, i.e. the no-foo will suppress the warning even if its severity has been upgraded.

[udif]

In fact if you make the handling in createDefinition always take the newer rather than the older definition in the case of a conflict then you don't even need a separate option. It's sufficient for the "warning" to be an error by default and downgradable by the user if desired.

ok, let's try it.

Yes, I discovered and fixed that issue yesterday. Please pull the latest changes.

I took a look at the new code and it makes sense now. You can guess I'm not playing much with GCC errors and warnings, as I wasn't aware of their convention.

BTW, clang-tidy has some warnings on the autogenerated DiagCode.cpp, and I even managed to totally break clang-tidy (core dump or internal error) in one of my earlier changes. something about \<eof>, followed by a few hundred lines of clang-tidy stack trace.

[udif]

Whooops... A failed unit test. But not failing on my local tree? Might be related to an uninitialized variable, because other macos and windows pass. Running the content of the test locally yields:

x.v:4:8: error: redefinition of 'top' [-Wredefinition]
module top #(parameter real r, int i) ();
       ^
x.v:1:8: note: previous definition here
module top #(parameter int foo = 3) ();
       ^
warning: no top-level modules found in design [-Wmissing-top]

Build failed: 1 error, 1 warning

old code:

~/git/slang/build$ slang x.v
Top level design units:
    top

x.v:4:8: error: redefinition of 'top'
module top #(parameter real r, int i) ();
       ^
x.v:1:8: note: previous definition here
module top #(parameter int foo = 3) ();
       ^

Build failed: 1 error, 0 warnings

Unit test checks for exactly 1 message.

    REQUIRE(diags.size() == 1);
    CHECK(diags[0].code == diag::Redefinition);

Still not sure why this is happenning:

/home/runner/work/slang/slang/tests/unittests/HierarchyTests.cpp:71: FAILED:
due to unexpected exception with message:
  Assertion '!topDef' failed
    in file /home/runner/work/slang/slang/source/compilation/Compilation.cpp,
  line 476
    function: void slang::Compilation::createDefinition(const slang::Scope&,
  slang::LookupLocation, const slang::ModuleDeclarationSyntax&)

[MikePopoloski]

Looks like you figured out the topDef assert -- basically the code was saying we should never have an existing entry in topDefinitions because if there was a redefinition we would have returned early from the function above, but after your change that's no longer true.

I think my comment above about defining a new warning for this case got missed above. The Redefinition diagnostic gets used in a bunch of other places that should not be able to be demoted, so we want a separate warning that applies to just to duplicate Definitions (modules / interfaces / programs). Otherwise your code seems like it's on the right track.

[MikePopoloski]

Maybe the new warning could be called something like -Wduplicate-definition

[MikePopoloski]

Perfect, thanks!