architecture_027 and component_020 want different indents for a comment

[SittingDuc]

Environment Ubuntu 22 Linux, Python 3.10, VSG 3.19.0-dev3 (so issue-1065 branch or Master as of 17 December 2023)

Describe the bug When describing a component within the architecture declaration region, with component_019 enabled, it deletes all comments. With component_019 disabled, then component_020 wants a different count of indents than architecture_027 does, for the same line - the "component fred is" declaration with associated comment. Architecture 027 has the comment vertically aligned, Component 020 has it over-indented.

To Reproduce

duc@nest:~/code/vhdltest$ vsg -c cfg_a027_c020.json -f a027_c020.vhd
================================================================================
File:  a027_c020.vhd
================================================================================
Phase 7 of 7... Reporting
Total Rules Checked: 532
Total Violations:    2
  Error   :     2
  Warning :     0
----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  architecture_027          | Error      |          8 | Move 2 columns
  component_020             | Error      |          8 | Move 4 columns
----------------------------+------------+------------+--------------------------------------
NOTE: Refer to online documentation at https://vhdl-style-guide.readthedocs.io/en/latest/index.html for more information.

using the files attached to the case and observe two violations on line 8 that are contradictory: a027_c020.vhd.txt cfg_a027_c020.json (just to disable component_019)

The issue specifically requires a comment on the component is line, and another comment on a port / generic.

Expected behavior Architecture_027 would give nicely aligned comments, so it is the nicest.

Additional context From what I can tell parsing the python, both vsg/rules/architecture/rule_027.py and vsg/rules/component/rule_020.py subclass align_tokens_in_region_between_tokens_skipping_lines_starting_with_tokens, the former checking content from "architecture .. is" to "begin" (my example file, lines 5-21), and the latter checking from "component .. is" to "end component" (my example file, lines 8-15). The difference is, in the call to align_tokens_in_region_between_tokens_skipping_lines_starting_with_tokens.analyze(), "iToken" is numerically one higher for component_020 than it is for architecture_027, which is probably why component_020 wants to over-indent the output.

My suspicion is the code is not well-handling the detail that the component declaration "belongs" to indent-depth N and the component port description "belongs" to indent-depth N+1, and this is producing the conflicting solutions. And I can't see a clean elegant way to code around that; so I offer a bug-report not a pull-request.

Postscript I am hopeful that now that I am parsing more of the VHDL I can reach, my rate of bug suggestions and feature fixes will decline; the project and the design really are good, I'm just stretching it do do things no one had considered before today, and as it solves more of my problems for me :D I won't find as many little issues.. Thanks again!

[jeremiah-c-leary]

Afternoon @SittingDuc ,

My suspicion is the code is not well-handling the detail that the component declaration "belongs" to indent-depth N and the component port description "belongs" to indent-depth N+1, and this is producing the conflicting solutions. And I can't see a clean elegant way to code around that; so I offer a bug-report not a pull-request.

That was a correct analysis. The base rule align_tokens_in_region_between_tokens_skipping_lines_starting_with_tokens was not including all the tokens on the first list line so it was miscalculating the starting column. That then led to the discrepancy between architecture_027 and component_020.

I pushed an update to the issue-1069 branch. Let me know if it resolves the issue on your end.

I am hopeful that now that I am parsing more of the VHDL I can reach, my rate of bug suggestions and feature fixes will decline; the project and the design really are good, I'm just stretching it do do things no one had considered before today, and as it solves more of my problems for me :D I won't find as many little issues..

VSG is only as good as its test cases, so the more code that is thrown at it the more "corner cases" are revealed. I update my test suite and VSG gets incrementally better.

Take this issue, my tests did not include a comment on the line with the component keyword. Why didn't it include it...I can't really say. Probably a lack of imagination for generating test cases that stress the logic.

Eventually we will work through all the issues you run into and VSG will be better for it.

Regards,

--Jeremy

[SittingDuc]

Afternoon @SittingDuc ,

Hello,

My suspicion is the code is not well-handling the detail that the component declaration "belongs" to indent-depth N and the component port description "belongs" to indent-depth N+1, and this is producing the conflicting solutions. And I can't see a clean elegant way to code around that; so I offer a bug-report not a pull-request.

That was a correct analysis. The base rule align_tokens_in_region_between_tokens_skipping_lines_starting_with_tokens was not including all the tokens on the first list line so it was miscalculating the starting column. That then led to the discrepancy between architecture_027 and component_020.

I pushed an update to the issue-1069 branch. Let me know if it resolves the issue on your end.

Running the branch over the real VHDL and the discrepancy goes away, and the header comment is nicely aligned to the port map comments, so the issue is resolved by me. :+1:

Having a shufti over the commit changes, I see vsg/rules/align_tokens_in_region_between_tokens_skipping_lines_starting_with_tokens.py got a new token in the call.. Since this is common code, I hope your regression tests are good, or else you may have broken things elsewhere while fixing this corner :grinning:

I am hopeful that now that I am parsing more of the VHDL I can reach, my rate of bug suggestions and feature fixes will decline; the project and the design really are good, I'm just stretching it do do things no one had considered before today, and as it solves more of my problems for me :D I won't find as many little issues..

VSG is only as good as its test cases, so the more code that is thrown at it the more "corner cases" are revealed. I update my test suite and VSG gets incrementally better.

:+1:

Take this issue, my tests did not include a comment on the line with the component keyword. Why didn't it include it...I can't really say. Probably a lack of imagination for generating test cases that stress the logic.

(wandering off topic ..)

I suspect this instance actually comes back to philosophy - the documentation for component_019 points out comments next to port lists inevitably become out-of-date and become a source of confusion, and confusion makes bugs. So your own code correctly follows the philsophy, deletes comments next to components and thus, you don't automatically think about testing if comments on components break - because you just don't do that.

And I am definitely guilty of overlooking use-cases that are not my own. (Only human fowl, after all) For this instance, I applaud the philosophy, and I have been caught-out by stale comments on a component no longer matching the behaviour, causing pain in debugging; but I have also had pain from the inverse: a component that does not well describe their protocol and meaning - usually from a third party core. Things like half the flags being active-high and half active-low and neither labelled in the identifier. So for myself I will disable component_019 and accept the trade-off between stale comments and undocumented third party cores...

One hard task from being a developer is remembering to step back and think of a way you yourself don't use the tool and try to envision how that use would break. Or hand it to a 5 year old. ("what do you mean it breaks if you turn it on and then plug the USB cable in? Surely everyone plugs all the cables in and then turns it on?!") And then we grow a set of corner cases and regressions.

Eventually we will work through all the issues you run into and VSG will be better for it.

Regards,

--Jeremy

So, the new branch fixes the issue where I saw it, doesn't appear to create any new issues, and looks good to me :+1:. Thanks again for all the work you put into this, my shiny new toy for breaking VHDL.

[jeremiah-c-leary]

Morning @SittingDuc ,

Having a shufti over the commit changes, I see vsg/rules/align_tokens_in_region_between_tokens_skipping_lines_starting_with_tokens.py got a new token in the call.. Since this is common code, I hope your regression tests are good, or else you may have broken things elsewhere while fixing this corner

Yeah, all my tests have passed but if the tests are not sufficient it could be an issue. There is a similar rule that already included the setting the include to beginning of new line so the change should be low risk of causing issues.

I am glad I have the test suite I do though. It may not be 100% complete, but it keeps me from breaking existing features and has saved me countless times.

I suspect this instance actually comes back to philosophy - the documentation for component_019 points out comments next to port lists inevitably become out-of-date and become a source of confusion, and confusion makes bugs. So your own code correctly follows the philsophy, deletes comments next to components and thus, you don't automatically think about testing if comments on components break - because you just don't do that.

Most of the rules are not very opinionated, but component_019 surely is. Deleting comments is a "bold" move. I have debated on whether to make it disabled by default, but then few people would know about it.

One hard task from being a developer is remembering to step back and think of a way you yourself don't use the tool and try to envision how that use would break. Or hand it to a 5 year old. ("what do you mean it breaks if you turn it on and then plug the USB cable in? Surely everyone plugs all the cables in and then turns it on?!")

I believe you are correct. I also believe it is related to experience. There are some productions I do not have much experience with and so have not come to my own styling preferences. I eventually took the right hand side of a production and tried to apply as many rules as possible for formatting. The block rules are an example of this method. I would eventually like to go through every production and generate any "missing" rules.

Thanks again for all the work you put into this, my shiny new toy for breaking VHDL.

Thanks for the kind words. It is nice to hear that people appreciate my effort.

So, the new branch fixes the issue where I saw it, doesn't appear to create any new issues, and looks good to me 👍.

Awesome,

I will get this merged to master.

--Jeremy