jeremiah-c-leary / vhdl-style-guide

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

`report_statement_` rules do not act within assertions, nor are there equivalent rules that do act within assertions #1082

Closed JHertz5 closed 6 months ago

JHertz5 commented 7 months ago

Environment ''' $ ./bin/vsg -v VHDL Style Guide (VSG) version: 3.20.0 Git commit SHA: 22beaa35 '''

Describe the bug There are a handful of rules that act upon standalone report statements, but do not act when the report is within an assertion, and there is no equivalent rule that does act within assertions. This is true of the following rules:

To Reproduce Steps to reproduce the behavior:

  1. Create test.vhd with the following contents:
    
    entity test is
    end entity test;

architecture rtl of test is

procedure my_procedure is begin

REPORT          "hello"
  SEVERITY      warning;

assert true
  REPORT        "hello"
  SEVERITY       warning;

end procedure;

begin

end architecture rtl;

2. Run `vsg -f test.vhd --fix`
3. Observe the changes to the file:
```vhdl
entity test is
end entity test;

architecture rtl of test is

  procedure my_procedure is
  begin

    report "hello"
      severity warning;

    assert true
      REPORT        "hello"
      SEVERITY       warning;

  end procedure;

begin

end architecture rtl;
  1. Note that the standalone report has been corrected, but not the one within the assertion.

Expected behavior I would expect to see the fixes applied to the assertion as well.

JHertz5 commented 7 months ago

It seems to me that it is more within the spirit of the existing rules to create new rules to act on assertions rather making the existing report rules apply in to assertions. I will have a go at making these changes.

jeremiah-c-leary commented 6 months ago

Good Afternoon @JHertz5 ,

It seems to me that it is more within the spirit of the existing rules to create new rules to act on assertions rather making the existing report rules apply in to assertions.

I would agree with this. The assertion production is defined as:

assertion ::=
    **assert** condition
  [ **report** expression ]
  [ **severity** expression ]

while the report_statement production is defined as:

report_statement ::=
    [ label : ]
        **report** expression
            [ **severity** expression ] ;

so while they look similar they are different productions so it would be best to have different rules for them, although it is hard to imagine someone would want different rules.

Corresponding assert_1.. rules would make the most sense.

I will have a go at making these changes.

Awesome. I appreciate the effort.

I find for rules that essentially already exist that is takes longer to write the documentation and the tests than it takes to implement the rule itself.

Regards,

--Jeremy

JHertz5 commented 6 months ago

Hi @jeremiah-c-leary. Happy new year! Thanks very much for your response. I forgot to link it here, but I have already raised PR #1083 for this. In addition to replicating the rules listed in the description for the report and severity keywords, I also added equivalent rules for the assert keyword. Obviously, any feedback you have is welcome!

jeremiah-c-leary commented 6 months ago

Evening @JHertz5 ,

Everything looks really good.

Hopefully it wasn't too difficult to implement the rules, tests and update the documentation.

As you can see, to implement the rules just required changing 1 file and adding 6. The remaining 28 files were for tests and documentation.

Were there any issues you ran into while implementing the rules. Anything unclear or something you struggled with?

Again, thanks for the contribution.

I will get this merged to master.

Regards,

--Jeremy

JHertz5 commented 6 months ago

Hi @jeremiah-c-leary.

Everything looks really good.

Excellent, thanks very much!

Hopefully it wasn't too difficult to implement the rules, tests and update the documentation.

Yes, it was easier than I expected to implement, although this was helped by the fact that the rules are simple and the fact that I could copy most of the code from existing rules.

Were there any issues you ran into while implementing the rules. Anything unclear or something you struggled with?

I don't think so, again most of the code was copied from existing rules which made it very easy. For a more complex and/or original rule, I'm not sure where I would begin but I think this will become clearer as I look at other rules and become more familiar with the codebase.

Again, thanks for the contribution.

No problem at all, it was a pleasure! I plan to make more contributions if/when I see any more changes that I would like and think I could fix.