jeremiah-c-leary / vhdl-style-guide

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

pragma support causes pragma lines to be parsed as non-comments #1061

Closed SittingDuc closed 7 months ago

SittingDuc commented 7 months ago

Environment vsg-3.18.0-dev grabbed from github branch 'master' early December 2023: commit #e5ca972b. Ubuntu Linux 22.04

Describe the bug With rule pragma_300 enabled, the specified comment -- synthesis translate_off is now treated as not-a-comment by the code.

In particular, placing a translate-off pragma inside an IF statement to alter the behaviour for simulation as contrasted to synthesis, causes rule if_036 to want to move the 'then' onto the line with the 'translate_on', which is not going to work. May also require rule if_002 set to remove parenthesis.

Odds are placing the pragma-comment in other places causes other issues, but this 'IF' statement bug is the one I walked into first and hardest

To Reproduce Steps to reproduce the behavior:

  1. grab pragma_issue_2.vhd and pragma_issue_2.json
  2. vsg -c pragma_issue_2.json -ap -f pragma_issue_2.vhd
  3. observe rule if_036 attempting to move the 'THEN' keyword on line 16 (and if_009 attempting to indent the pragmas too)
  4. if vsg -c pragma_issue_2.json -f pragma_issue_2.vhd --fix is used, observe the updated .vhd has the then on the comment line

Expected behavior When -- synthesis translate_off is identified as a pragma, it needs to still be treated as a comment for everything except rule comment_010 indentation

Bonus Bug adding \s+--\s+vsg_ to the set of pragmas causes -- vsg_off to be ignored, which is entertaining

pragma_issue_2.json pragma_issue_2.vhd.txt

Bonus Bonus Bug Ooh, if I let --fix change the 'THEN' to be on the comment, vsg jumps to 100% CPU one thread. don't run vsg -c pragma_issue_2.json -f pragma_issue_2.vhd --fix twice!

jeremiah-c-leary commented 7 months ago

Morning @SittingDuc ,

This is a very interesting case and something I had not fully considered. I can not ignore pragmas when moving tokens because I could move a token from outside a pragma pair to inside a pragma pair.

I see the issue and there are several rules I will need to update to ensure pragmas are honored correctly.

--Jeremy

jeremiah-c-leary commented 7 months ago

Morning @SittingDuc ,

I believe I have updated all the necessary rules to prevent the issue with rule if_036 from occuring I pushed an update to the issue-1061 branch. Could you test it out and let me know.

I still need to track down the other two bugs you have identified.

Thanks,

--Jeremy

jeremiah-c-leary commented 7 months ago

Morning @SittingDuc ,

I fixed the bonus bug with my latest push.

It looks like the bonus bonus bug was fixed with the original push.

Let me know if you run into any other issues.

Thanks,

--Jeremy

SittingDuc commented 7 months ago

I am getting inconsistent results, I wonder if I borked my install?

(narrator: it seems the duc did indeed bork the install)

issue_1061_cfg.json issue_1061_input.vhd.txt

I have created an input vhd file and when parsed with issue_1061_cfg.json I would expect an output very similar to issue_1061_perfect.vhd. Instead I am seeing the synthesis translates around the library and use clauses "move" above or below the line they are on (bad, changes code behaviour), and the synthesis translates on the first if block move the 'then' either inside the block (bad) or onto the comment line still (bad too). issue_1061_perfect.vhd.txt

The second if-block is fine because it has brackets and they seem to protect it from pragmas.

Annoyingly, on the code-base I can share, I am getting few to no misbehaviours on the if-blocks; but on the code-base I cannot even admit the existence of (hi boss!), I am getting all the misbheaviours. So clearly my cfg.json varies on the two computers.

issue_1061_second.vhd.txt issue_1061_cfg2.json

For extra entertainment, I have created issue_1061_cfg2.json which adds the VSG lines as pragmas. Running it over the input vhd should produce a result like issue_1061_second.vhd, but instead I am still seeing translate pragmas "moving" around the code, on the library/use clauses, and on the if-blocks.

After writing this comment, I have tried to run the commands again and diff'd the outputs and suddenly it is not misbehaving as much. So I have achieved something alright, I just cannot describe what.

Stepping backwards, on the real vhdl I am tempted to refactor the code to put the entire if-block inside the pragma, so I have an entire if for synthesis and a separate if for simulation; and that will end this attempting to shove a pragma into the middle of a VHDL construct.

If I had hair I might start pulling it out, this one is not co-operating which means I feel I am not producing good bug reports. Please have a look at the attached vhdl, it at least shows what my eyeballs think code laced with pragmas in different constructs could look like; and I am hopeful it at least is useful for regression testing.

Thanks!

SittingDuc commented 7 months ago

oh yay. changing branches on my git checkout and then running setup.py does not install the new version. Having two versions in /usr/local/lib/python3.10/ will guarantee the wrong copy gets run. For some reason vsg --version keeps reporting "Installed from .zip file", even when I think it should be a git copy...

with parenthesis around the if (if_002 set to require), my -- synthesis translate_off either violates pragma_300 "use 0 spaces for indent" if it has any whitespace, or it violates if_009 "Indent with 20 space(s)" if it has no whitespace.

with no parenthesis (if_002 set to remove), I get violations of if_036 "Move **then** keyword to same line as -- synthesis translate_on". command line vsg is demanding I indent with 19 spaces, but codium vsg is not. if I edit the line in codium, then it does complain, and requires 19 spaces also.

changing configuration to yaml or json doesn't make a difference, so that's good. changing my pragma regexp to include .*$ or not doesn't make a difference.

(oi! github! I was NOT finished with that. and I did NOT close the issue)

SittingDuc commented 7 months ago

((so apparently misclicking on the "markdown is supported" button on the github pages submits the comment and closes the issue too? .. that seems totally intuitive ..))

Okay, again with the attempting to get to the bottom of this.

$ vsg --version
VHDL Style Guide (VSG) version: 3.18.0.dev14
Git commit SHA: f7f87566

(matches the git sha for end-branch 1061. first check pass)

real vhdl, configuration in yaml.. 12 warnings about line length. configuration in json.. same 12 warnings. So that's nice.

Manually messing up the vhdl indent on the -- synthesis translate_off... only pragma_300 complains, not if_019 so that's good too. Manually messing up the configuration file regexp causes all the old misbehaviours to come back, so that is A Clue (suggests the vsg code on GitHub is correct and either my config is broken at least sometimes, or I am running the wrong code)

Conclusion

Now that I have correctly checked out branch issue_1061 from github and applied it to my workstation, vsg is parsing my VHDL correctly, and pragmas in the middle of if blocks are working as I expect, indenting as I expect, and not attempting to move tokens across synthesis boundaries.

I think I am happy to see this one merged and closed. (Properly this time, thank you GitHub)

jeremiah-c-leary commented 7 months ago

Morning @SittingDuc ,

I think I am happy to see this one merged and closed. (Properly this time, thank you GitHub)

That is good news. When I started reading the problems you were having I was a little confused. Although it wouldn't be the first time I pushed an update and it didn't work. Others have had the same problem with using the wrong branch to validate an update.

btw, thanks for bringing up pragmas. I'm surprised it took this long for someone to point out the VSG was ignoring them and could create invalid code.

I will merge this to master and start the release process.

Thanks,

--Jeremy

SittingDuc commented 7 months ago

you were a little confused? I was a lot confused. Especially with getting different behaviours from different machines.

Lesson learned: sanitise the machine before testing branches - find and remove all old copies of vsg to ensure the only one available is the one you are attempting to test.

Thanks again for the linter and all the work within