Autofix prefix/suffix rules

[imd1]

Is your feature request related to a problem? Please describe. It would be really good for VSG to be able to automatically fix rules such as constant_015 i.e. prefix or suffix renaming rules. For legacy code bases, it's a lot of manual changes required

Describe the solution you'd like In the constant_015 example, one could configure a list of existing prefixes...if VSG detects one of them, it removes the prefix before adding the desired one, otherwise it just adds the prefix. Dealing with the combination of prefixes and suffixes, this idea could be extended by using the "list of existing" to detect use as suffixes and remove the suffix.

[jeremiah-c-leary]

Hey @imd1,

It would be really good for VSG to be able to automatically fix rules such as constant_015 i.e. prefix or suffix renaming rules. For legacy code bases, it's a lot of manual changes required

I think the updates for the other issues you have submitted would allow me to do this.

It will work if there is a single prefix/suffix defined though. Otherwise VSG would have no method to determine which to use, unless we could define rules that dictate what prefix should be used. Then maybe I could determine the prefix based on it's usage and then flag the declaration and usage accordingly.

One rule I have at work is signals connected to output ports should start with w_. I could use that as a basis for setting the prefix. In this case I would need to know how the port is declared on the entity first, which could be in a different file.

hmmm...I would require some restructuring of the code to be able to reference multiple files. This would also require that you include all the files so the correct ones could be referenced.

[imd1]

I think I'm asking for something slightly simpler than your comments are suggesting i.e. I'm not worried about differentiating different categories of signals for example. I want all my signals to start with s_, say. Same applies to constants (C_), generics (G_) etc etc

The idea I have is that it should be possible to prefix constants with C_. The slight complication arises if the constant already starts with a prefix (CONST_) or suffix (_C, _CONST), so I thought you could create a list of existing prefixes and suffixes (CONST_, _C, _CONST) in the configuration file, so VSG would attempt to strip the constant of these prior to adding C_.

The same principle could be applied to all items where there is a single prefix/suffix, which is my general use case (apart from ports)

With ports, it might be possible to specify _i for input ports, _o for output ports etc and then apply the above techniques. This would requires individual config for the port direction, maybe something VSG could parse?

If you want to split this up into several layers of changes, and I get some auto-fixing earlier as a result, that's fine. Anything will help when trying to get large legacy code sets updated

[imd1]

My suggestion is to trial auto fixing something simple e.g. constants,and let me see if my request and your understanding matches

[imd1]

I would be happy to progress this under your guidance....

First step would be to blindly autofix the rule e.g. for constant_015, modify the constant to add the prefix or suffix. The second step would be to add the capability of stripping the existing constant of prefixes and suffixes supplied by configuration before applying the correct one. Can you offer some guidance?

[imd1]

I guess this is harder that I thought: as well as changing the name of the constant in the definition, you'd have to change it everywhere in the file...mmmm

[imd1]

On further reflection, this might be better actioned by writing some separate custom scripting to do the renaming...so once that is written, the task is to trigger the scripting to apply the auto fixing?

[jeremiah-c-leary]

I guess this is harder that I thought: as well as changing the name of the constant in the definition, you'd have to change it everywhere in the file...mmmm

Agree. You also have the issue of constants defined in packages that VSG does not have visibility of.

Well, I do have a consistent_token_case rule that could be modified somehow. It is currently very expensive to run due to how I coded it, but could be significantly improved.

It would be nice to identify tokens as constants, variables, types or signals. For example this code:

  constant c_const : std_logic := '0';
  begin
  a <= c_const;

could look something like this:

<constant_keyword><whitespace><constant_identifier><ws><colon><ws><??><ws><assignment_operator><ws><literal><semicolon><return>
<begin_keyword><return>
<port><ws><assignment_operator><ws><constant_usage><semicolon>

Then the existing prefix/suffix rule could be used. We would just pass and to it. This would only work if a single allowable prefix/suffix was defined.

I do not currently have defined. However, there is another ticket that is forcing me to further refine my parsing of expressions and conditions. If I had a list of constant identifiers, then I could search for locations where they are used and assign the token.

This would only work in a file that has the constants defined in the same file where they are being used.

To handle the case where constants can be fixed over multiple files would require ensuring they are all read in, and then keeping a list of constants on a per file basis. Then ensuring you can reference the correct list depending on which package is pulled in.

That might be something for a 4.0 release, as that would be a major change to how VSG currently works. It would also give me an opportunity to improve main.py.

[imd1]

I've just spent a considerable time having to manually change to variables to add the project standard _v suffix...

All I did was open the file in Notepad++, search for the offending variable (whole word, case sensitive), enter the replacement string, and replace each one stepwise to ensure e.g. strings in comments were not changed.

Surely this is something ideal for automation within VSG. It is simple to do if you perform this as a text file manipulation etc etc, just as I had to manually

Adding this idea to fix prefixes and suffixes would be a game changer

[imd1]

I wonder whether I should just write a script to parse VSG log files and perform the find and replace?

[imd1]

I'm going to have a go locally at writing a Python script that parses the VSG .json output file and applies fixes for the prefix and suffix rules. Once happy, I can send it to you for possible integration with VSG or similar.

[jeremiah-c-leary]

I would check out the rule signal_014, which checks for consistent case of signal names. This would be similar in scope, but the fix would be different.

Looks like there are two base rules that handled consistent_case: https://github.com/jeremiah-c-leary/vhdl-style-guide/blob/master/vsg/rules/consistent_case_of_tokens_from_between_tokens_applied_to_region.py https://github.com/jeremiah-c-leary/vhdl-style-guide/blob/master/vsg/rules/consistent_token_case.py

It looks like you just might need to slightly change the analyze to check for prefixes versus case.

 36     def analyze(self, oFile):
 37         lTargetTypes = oFile.get_tokens_matching(self.lTokens)
 38         lTargetValues = []
 39         lTargetValuesLower = []
 40         for oTargetType in lTargetTypes:
 41             oToken = oTargetType.get_tokens()[0]
 42             lTargetValues.append(oToken.get_value())
 43             lTargetValuesLower.append(oToken.get_value().lower())
 44
 45         oToi = oFile.get_all_tokens()
 46         iLine, lTokens = utils.get_toi_parameters(oToi)
 47
 48         for iToken, oToken in enumerate(lTokens):
 49
 50             iLine = utils.increment_line_number(iLine, oToken)
 51
 52             if is_token_in_ignore_token_list(oToken, self.lIgnoreTokens):
 53                 continue
 54
 55             sTokenValue = oToken.get_value()
 56             sTokenValueLower = sTokenValue.lower()
 57             for sTargetValue, sTargetValueLower in zip(lTargetValues, lTargetValuesLower):
 58                 if sTokenValueLower == sTargetValueLower:
 59                     if sTokenValue != sTargetValue:
 60                         sSolution = 'Change "' + sTokenValue + '" to "' + sTargetValue + '"'
 61                         oNewToi = oToi.extract_tokens(iToken, iToken)
 62                         oViolation = violation.New(iLine, oNewToi, sSolution)
 63                         dAction = {}
 64                         dAction['constant'] = sTargetValue
 65                         dAction['found'] = sTokenValue
 66                         oViolation.set_action(dAction)
 67                         self.add_violation(oViolation)

So lines 43 and 56 probably.

You will need to check for the prefix/suffix first though.

[imd1]

FYI I am still working on the Python to autocorrect these rules - it is embedded in my local scripting outside the scope of VSG. It parses the json results file to find where rules are violated and attempts to correct them. It uses a data base of existing prefixes and suffixes that it will remove before applying the requested one (legacy code tends to contain a complete mixture of styles!)

[jeremiah-c-leary]

You might want to look at how architecture_601 was implemented. One of the problems with the existing consistent case rules is they are greedy. They can end up checking into procedures and functions. After implementing the rule, I feel much more confident that fixing the prefix/suffix rules could be done.

[jeremiah-c-leary]

Hey @imd1,

Where do we stand on this issue? Is this something we should still persue?

--Jeremy

[imd1]

Hey @imd1,

Where do we stand on this issue? Is this something we should still persue?

--Jeremy

It's definitely worth pursuing. I have completed my scripting solution which runs outside VSG which is OK but it suffers because it uses regex to find and replace labels rather than parsing like VSG. Do you want more details of what I've done so you can see what is required?

[jeremiah-c-leary]

Do you want more details of what I've done so you can see what is required?

Did you take scoping into account, like issue #551?

Section 12 of the LRM covers scope and visibility. I am still trying to digest the impacts to VSG and how I can properly handle it. I think I need to solve that issue first before I can reliably implement this feature.