Pragmas and Blank Line rules

[SittingDuc]

Hello again,

Getting greedy now, this request is low priority / nice-to-have only.

At present, rules like instantiation_019 force a blank line after an instance, component_018 after end component, and so forth.
If I put a pragma after a function, component declaration, or instance, it would be visually nicer to have it next to the end declaration, and then the blank line enforced below it.

  -- Organise our seige engines

-- vsg_off component_012
  component Catapult is
    generic (
        distance : natural;   -- inches or miles, impacts scale of aim
        tension  : natural    -- how tightly wound is the spring? in ft-lb
    );
    port (
        load : in  std_logic;
        aim  : in  real;      -- note scale depends on distance generic
        fire : in  std_logic  -- "ready, fire, aim"
    );
  end component Catapult;
-- vsg_on

  component trebuchet is
  -- [...]

As defined, component_018 will try to push the vsg_on line down, making the code less compact and a fraction less readable. Feature request then is some way to make a previously-defined pragma "not count" for the blank line rules, so it can "snug" up to the code it is disabling / modifying, and then the original whitespace rule applies after it (i.e. the blank line remains between the Catapult and the trebuchet components)

Not a major concern and I can totally live with it, but if I have noticed, other people might too, now that pragmas are a thing ;)

Thanks once again!

[jeremiah-c-leary]

Afternoon @SittingDuc ,

I pushed an update for this to the issue-1065 branch.

I added the option require_blank_line_unless_pragma to those rules which will enforce a blank line unless there is a pragma on the next line.

Could you give it a try and let me know if there are any changes I need to make?

Thanks,

--Jeremy

[SittingDuc]

Hello, checked out the latest branch and running it on real vhdl..

Component instance (instantiation_019) is good with the new code - no longer complains about pragma next to the close bracket/ semicolon; and also does not complain if there is a blank line. Good.

component_018 good, process_011 good. I don't have time to test the others, but the new code is behaving as expected/desired for the code I can reach today.

Much thanks,

[jeremiah-c-leary]

Morning @SittingDuc ,

I can leave this issue open for a while if you want to spend more time validating the change.

--Jeremy

[SittingDuc]

Hello,

A little further review shows that all the places I have checked (entity declaration end, instantiation start/end, architecture end, process end, generate end, etc) all tolerate a pragma line after themselves, when configured to do so.

However, I then note the rule now behaves like the no_code option for component_003 and similar where, if a pragma is present after the block, the linter no longer requires a blank line after the pragma. I can live with it, but it does mean people around me I can create uglier code

-- synthesis translate_off
  gen_fred : if true generate
  begin
    o_wilma <= b_pebbles;
  end generate gen_fred;
-- synthesis translate_on
  o_betty <= b_bam_bam

with generate_003 set to require blank line, a blank line is added between the end generate and the pragma. With generate_003 set to require blank line_unless_pragma, the code as written passes the linter.

What I would ideally love would be a violation with a fix of

  end generate gen_fred;
-- synthesis translate_on

  o_betty <= b_bam_bam

which I guess makes the semantics "require blank line or pragma" not "unless".

In an effort to be less of a say-er and more of a do-er, I tried reading the commit to see if I could work out how to write the change I want, and I must admit I am better at VHDL than Python.

In summary, the code in PR#1068 does improve the behaviour, and allows me to have my pragmas snug up to the code block they are to jacket; and if technical reasons make the further feature request extension impractical, I can live with that. But if the feature could be extended to then require a blank line after the pragma (or before in the above-code cases, generate_004 and similar), that would be even more powerful.

Thanks again,

[jeremiah-c-leary]

Morning @SittingDuc ,

I think there might a different method to handle the desired formatting if we create rules around pragmas. I believe we can divide pragmas into three types: opening, closing, and single.

opening and closing refer to a pair of pragmas. For example:

-- synthesis translate_off
-- synthesis translate_on

where -- synthesis translate_off is the opening pragma and -- synthesis translate_on is the closing pragma.

single pragmas do not come in pairs. For example:

-- synthesis library <my_lib>

If pragmas are divided into these types then rules can be written against those types:

• require blank line before opening pragma

• no blank line after opening pragma

• no blank line before closing pragma

• require blank line after closing pragma

So now the behavior of the blank lines around opening and closing pragmas apply to the pragmas themselves instead of being relative to other productions.

For example:

end component;
-- synthesis translate_off

signal debug_wr_en : std_logic;
signal debug_rd_en ; std_logic;

-- synthesis translate_on
-- synthesis translate_off

signal debug2_wr_en : std_logic;

-- synthesis translate_on
component fifo is

I would think the desired formatting would be:

end component;

-- synthesis translate_off
signal debug_wr_en : std_logic;
signal debug_rd_en ; std_logic;
-- synthesis translate_on

-- synthesis translate_off
signal debug2_wr_en : std_logic;
-- synthesis translate_on

component fifo is

So blank line is added before every opening pragma and a blank line is added after every closing pragma.
Blank lines are removed after opening pragmas and before `closing pragmas.

I believe this would give the compactness of pragmas you are looking for.

What do think?

--Jeremy

[jeremiah-c-leary]

Morning @SittingDuc ,

I pushed an update to the issue-1065 branch where I modified the pragma definitions to include close, open, and single styles. I also added four new rules pragma_400, pragma_401, pragma_402, and pragma_403. These new rules handle blank lines around open and close pragmas.

Using a slightly modified version of your code example:

  1
  2 architecture rtl of fifo is
  3
  4 begin
  5
  6   o_ruble <= o_betty;
  7 -- synthesis translate_off
  8
  9   gen_fred : if true generate
 10   begin
 11     o_wilma <= b_pebbles;
 12   end generate gen_fred;
 13
 14 -- synthesis translate_on
 15   o_betty <= b_bam_bam;
 16
 17 end;

and using this configuration file:

  1 rule:
  2   generate_004:
  3     style:  allow_comment
  4   generate_003:
  5     style:  require_blank_line_unless_pragma

The above example code will be fixed to:

  1
  2 architecture rtl of fifo is
  3
  4 begin
  5
  6   o_ruble <= o_betty;
  7
  8   -- synthesis translate_off
  9   gen_fred : if true generate
 10   begin
 11     o_wilma <= b_pebbles;
 12   end generate gen_fred;
 13   -- synthesis translate_on
 14
 15   o_betty <= b_bam_bam;
 16
 17 end architecture rtl;

Could you check out the new rules and let me know if it gives the formatting you are looking for?

Thanks,

--Jeremy

[SittingDuc]

Hello,

The idea to allocate pragmas based on how they impact the code is a good idea. One drawback is I get python 'keyError' if I don't update my config file:

Migrating existing configurations

  "pragma": {
    "patterns": [
      "^\\s*--\\s+synthesis\\s+\\w+\\s*$",
      "^\\s*--\\s+synthesis\\s+\\w+\\s+\\w+\\s*$",
      "^\\s*--\\s+pragma\\s+\\w+\\s*$",
      "^\\s*--\\s+pragma\\s+\\w+\\s+\\w+\\s*$",
      "^\\s*--vsg_[on|off]\\s*$",
    ]
  },

yields

Traceback (most recent call last):
  File "/home/duc/.local/bin/vsg", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.10/dist-packages/vsg-3.19.0.dev4-py3.10.egg/vsg/__main__.py", line 110, in main
    oConfig = config.New(commandLineArguments)
  File "/usr/local/lib/python3.10/dist-packages/vsg-3.19.0.dev4-py3.10.egg/vsg/config.py", line 243, in New
    add_pragma_regular_expressions(dConfig)
  File "/usr/local/lib/python3.10/dist-packages/vsg-3.19.0.dev4-py3.10.egg/vsg/config.py", line 229, in add_pragma_regular_expressions
    for types in list(dStyle['pragma']['patterns'].keys()):
AttributeError: 'list' object has no attribute 'keys'

Might be possible to fix in documentation.

Testing the Changes
Once I have a config file updated, the changes all look good. I made an ugly-as vhdl and fed it to the linter and it does what I:
cfg_is1065.json
is1065_1.vhd.txt

Blank lines above and below the pragmas, and testing it with the default config, all the expected rules report violations (I modified the pragma regexp for this test, purely so I can label each line with the rule name)

Exceptional case
Of course, every beautiful plan is spoiled by an exception, and I found one. While most of the time we are using translate_off / translate_on to jacket a block of code for simulation, in functions in particular, I have found a couple of instances of

  function sleep return natural is
  begin
-- synthesis translate_off
    return 12; -- nap in sim
-- synthesis translate_on
    return 2400; -- decent sleep in hardware
  end function sleep;

So the semantics in my head returns to "if the thing the pragma is snuggled up to expects a blank line, that blank line request passes to the pragma. If the pragma is not snuggled up to a blank-line-expector, the pragma does not request a blank line either".

For this example, I can probably throw a vsg_off around the function, or just add the blank lines. It might even improve readability by making the simulation block "stand out".

Conclusion
The approach for treating pragmas as Opening, Closing, or Indifferent works, and for code that expected blank lines, I can get the behaviour / white-space I want. The one exceptional case feels like more effort than is justified to fix, so I will try to fix it in education; and the 'keyError' migrating from a config file with 3.19.0-pragmas to the new style can probably be fixed in documentation, or even with a more helpful message in config.py (I have found stackdumps help programmers and confuse user)

Thanks once more,

[jeremiah-c-leary]

Evening @SittingDuc ,

One drawback is I get python 'keyError' if I don't update my config file:

I added some error checking on configuring pragmas. In your example the following error message will be displayed:

Error in pragma definition:
  Refer to "configuring pragmas" section in documentation for details on how to configure pragmas.

Of course, every beautiful plan is spoiled by an exception, and I found one.
So the semantics in my head returns to "if the thing the pragma is snuggled up to expects a blank line, that blank line request passes to the pragma. If the pragma is not snuggled up to a blank-line-expector, the pragma does not request a blank line either".

Yeah, implementing that logic would be complicated. The rules are designed to be independent of each other to eliminate dependences. So implementing a rule to have knowledge of another rules configuration would violate the intent of the design.

and the 'keyError' migrating from a config file with 3.19.0-pragmas to the new style can probably be fixed in documentation

The "configuration pragmas" documentation was updated on the branch to show the open, close, and single formatting of the pragmas. It is a little terse, but there is something there.

Hopefully pointing users to that documentation if they followed the old format will be sufficient.

Let me know if you think the error checking I implemented is sufficient.

--Jeremy

[SittingDuc]

Hello,

I understand the philosophy and I can live with the code as it stands.

I tested the error message and it works, and is nicer than the Trackback. All the effort people put in to things that might never be seen

Thanks for all the work, I am happy for this to close / merge. Happy Holidays

[jeremiah-c-leary]

Evening @SittingDuc ,

All the effort people put in to things that might never be seen

I used to complain, well...still do, about incomprehensible error messages. Now I have an appreciation for how much work really goes into creating a useful one and why it is so hard to create a useful error message.

I will merge this to master.

--Jeremy