jeremiah-c-leary / vhdl-style-guide

Style guide enforcement for VHDL
GNU General Public License v3.0
191 stars 39 forks source link

Detect and give hint for unused signals, which are declared in a record #1000

Closed ungultig1 closed 12 months ago

ungultig1 commented 1 year ago

Hello,

VSG could be able to detect and give a hint for unused signals, which are declared in a record.

By any chance, do you plan to include this feature?

Thank you.

greetings

jeremiah-c-leary commented 1 year ago

Good Evening @ungultig1 ,

Unfortunately VSG is more of a code formatting than a static checker, so there are currently no plans to provide those types of checks.

I know there are other tools out there already, Spyglass and QLint, which will perform the type of checking you are asking for.

Regards,

--Jeremy

ungultig1 commented 1 year ago

Good Morning @jeremiah-c-leary ,

thank you for your answer.

I understand, but I'm wondering, because vsg won't work as there are incomplete code-snippets inside the architectures in a vhdl-file. During the development, it is handy to paste some code-snippets, but still keep the vsg-Formatter-functionality.

Here is a example of an Error-outcome in that case:

image

That's why i thought, you have an in-build style checker.

Maybe I should open a new issue, where I describe this behaviour - It would be a benefit, if the vsg-formatter would keep up its work and format the code, until reaching this line of error (?).

Thank you. Greetings.

jeremiah-c-leary commented 1 year ago

Good Morning @ungultig1 ,

During the development, it is handy to paste some code-snippets, but still keep the vsg-Formatter-functionality

I think understand your request now. Instead of requiring code that will compile, VSG should be able to operate, as best as it can, on code that is incomplete or malformed. It should be robust enough to attempt to format what it can.

Would it be acceptable to stop the formatting update at the point where the failure occurs? So in your example Error, everything before line 1019 would be formatted, but everything after would not be formatted.

That could be a first step at least.

Regards,

--Jeremy

ungultig1 commented 1 year ago

Hello Jeremy,

yes, this would be very handy in that case. Looking forward to it.

Thank you. Greetins

jeremiah-c-leary commented 1 year ago

Good Morning @ungultig1,

I pushed an alpha version of an implementation to the issue-1000 branch and was wondering if you had time to check it out and give some feedback.

I added the --force_fix command line option which works in conjunction with the --fix option. By using this option I was able to take this file:

  1
  2    architecture    RTL    of     FIFO    is
  3
  4      Signal     a : std_logic;
  5      siGnal b_something:            std_logic;
  6      SIGNAL c_something_else :std_logic;
  7
  8         componEnt rAm iS
  9         end component;
 10
 11         COMPONENT RAM
 12         END;
 13
 14         signal a : std_logic;
 15 signal b_something : std_logic;
 16       signal c_something_else : std_logic;
 17
 18     BEGIN
 19
 20   PROCESS BEGIN END;
 21
 22   PROCESS BEGIN END PROCESS;
 23
 24 end     rtl;

...which has a syntax error at line 12 because of the missing component keyword...and produce this file:

  1
  2 architecture rtl of fifo is
  3
  4   signal     a : std_logic;
  5   signal b_something : std_logic;
  6   signal c_something_else : std_logic;
  7
  8   component ram is
  9   end component;
 10
 11   component ram
 12   end;
 13
 14         signal a : std_logic;
 15 signal b_something : std_logic;
 16       signal c_something_else : std_logic;
 17
 18     BEGIN
 19
 20   PROCESS BEGIN END;
 21
 22   PROCESS BEGIN END PROCESS;
 23
 24 end     rtl;

So everything after line 12 is not fixed, which is what we would expect.

However, there will be certain types of rules which may not work, such as alignment rules. This will depend on where the syntax error is located.

For example, the signal identifiers, colon, etc... defined on lines 4 through 6 are not aligned. This is due to not finding the architecture begin keyword on line 18 because of the syntax error on line 12.

If lines 11 and 12 were commented out, then more lines would be fixed:

  1
  2 architecture rtl of fifo is
  3
  4   signal a                : std_logic;
  5   signal b_something      : std_logic;
  6   signal c_something_else : std_logic;
  7
  8   component ram is
  9   end component;
 10
 11   --        COMPONENT RAM
 12   --        END;
 13
 14   signal a                : std_logic;
 15   signal b_something      : std_logic;
 16   signal c_something_else : std_logic;
 17
 18 begin
 19
 20   process is
 21   begin end;
 22
 23   PROCESS BEGIN END PROCESS;
 24
 25 end     rtl;

So now the fixing stops at line 21 due to the missing process keyword and the signals are now aligned because the begin keyword was found by the parser.

Could you give this try and let me know how well it works on your end. I do not have any testing implemented for this feature yet, and still wondering how much I should test.

Any feedback on the implementation would be appreciated. I used `-force_fix as an option, but maybe there is a better name or maybe it shouldn't be a command line option?

I like the command line option because it does not change the normal behavior of VSG so it limits the impact to other users. I suppose I could make it configurable via a configuration file. At some point there will be too many command line options. I just do not know if I have hit that point yet.

Regards,

--Jeremy

jeremiah-c-leary commented 1 year ago

Afternoon @ungultig1 ,

Just wanted to ping you to see if you had a chance to check out the update for this issue.

--Jeremy

ungultig1 commented 1 year ago

Good morning @jeremiah-c-leary ,

sorry, I had too much stuff going on lately. I will try it out this week.

Thank you for your effort.

Greetings,

ungultig1 commented 12 months ago

Good Afternoon @jeremiah-c-leary ,

i just checked the new command line "--force_fix" (issue-1000-branch).

It works as accepted, and formats the code until a error appears.

Thank you for this feature.

Does the option "--force_fix" somehow impact the "--fix" option, when there are no errors? I did not notice any, so I assume it wont.

Greetings,

jeremiah-c-leary commented 12 months ago

Good afternoon @ungultig1 ,

Does the option "--force_fix" somehow impact the "--fix" option, when there are no errors?

The only affect --force_fix has is to not exit early if a classification error occurs and the --fix option was used.

112        try:
113           design_file.tokenize(self.lAllObjects)
114        except exceptions.ClassifyError as e:
115            if self.commandLineArguments.force_fix and self.commandLineArguments.fix:
116                print(e.message)
117                print('')
118                print('INFO:  The --force_fix option was enabled.')
119                print('       Proceeding to analyze and apply fixes.')
120                print('')
121            else:
122               raise e

So all the tokens are passed into the top level classifier starting at the design_file production at line 113. If the classifier fails, it will raise a ClassifyError exception. Line 115 checks for the --fix and the --force_fix options. If both are True then VSG exits the exception handler and continues with the analysis and then applies fixes. If either option is not present then line 122 will re-raise the ClassifyError exception and no analysis will be performed.

Hopefully that answered you question in a round about way.

I will merge this to master.

Please let me know if there are any other issues with the option.

Thanks,

--Jeremy