RTL gen pass fail to catch all System Verilog keyword

[KelvinChung2000]

VerilogTranslationPass fail with the following example.

def mk_aType(a: int = 4, b: int = 6):

    return mk_bitstruct(
        "aType",
        {
            "a": mk_bits(a),
            "b": mk_bits(b),
            "const": mk_bits(32),
        },
        namespace={},
    )

class test(Component):
    def construct(s, t):
        s.in_ = InPort(t)

if __name__ == "__main__":
    m = test(mk_aType())
    m.elaborate()
    m.set_metadata(VerilogTranslationPass.enable, True)
    m.apply(VerilogTranslationPass())

When running verilator --lint-only ./test__t_aType__a_4__b_6__const_32__pickled.v will give the following error.

%Error: test__t_aType__a_4__b_6__const_32__pickled.v:10:16: syntax error, unexpected const
   10 |   logic [31:0] const;
      |                ^~~~~
%Error: Exiting due to 1 error(s)

Fixing is easy as I can just not name things with the System Verilog key. But I will still report it as you have checks in place to prevent this, and the check fails to detect it.

[cbatten]

Hmmm ... "const" is on the keyword list here:

• https://github.com/pymtl/pymtl3/blob/master/pymtl3/passes/backends/verilog/util/utility.py#L112

So I think we would catch if you name a signal const but maybe we don't check field names?

[KelvinChung2000]

This is likely the case. My guess is the bitstruct field is not checked.

[cbatten]

Would make a great fix for a pull request if you are interested!

[yo96]

It is intended that we do not check for Verilog keywords when making a new bitstruct class, since it still works for Python simulation. We only check if the bitstruct fields are Python keywords or valid Python identifiers. https://github.com/pymtl/pymtl3/blob/4b3bc183b14ffacd069f48dc5997b00045aea53f/pymtl3/datatypes/bitstructs.py#L744-L747 I think it would be more appropriate to check if the field names are Verilog keywords in the translation pass.

[cbatten]

Right ... I think our point was we currently have a list of verilog keywords that get checked at translation although currently it looks like we don't check if bitstruct fields are verilog keywords at translation ...