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.

pymtl3/pymtl3/datatypes/bitstructs.py

Lines 744 to 747 in 4b3bc18

	if not isinstance( name, str ) or not name.isidentifier():
	raise TypeError( f'Field name {name!r} is not a valid identifier!' )
	if keyword.iskeyword( name ):
	raise TypeError( f'Field name {name!r} is a keyword!' )

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 ...