Have `constant_400` align keywords only at the same hierarchical level

[JHertz5]

Is your feature request related to a problem? Please describe. At the moment, constant_400 applies the same alignment to all => keywords within a constant decalaration. This can cause problems if there is a hierarchy, e.g. the constant is an an array of records or an array of arrays.

For example, take the file below. The => keywords next to ENUM_1 and ENUM_2 are aligned, and the => keywords within each of the sub-arrays/records is also aligned. I would like the structure to maintain this.

entity test is
end entity test;

architecture rtl of test is

  constant my_constant : my_type := (
    ENUM_1 => (
      A => 1,
      B => 2,
      C => 3
    ),
    ENUM_2 => (
      A => 1,
      B => 2,
      C => 3
    )
  );

begin

end architecture rtl;

When I run rule constant_400 on the code, I get the code below. In this code, all of the => keywords are aligned regardless of their level in the heirarchy of the data structure.

entity test is
end entity test;

architecture rtl of test is

  constant my_constant : my_type := (
    ENUM_1 => (
      A    => 1,
      B    => 2,
      C    => 3
    ),
    ENUM_2 => (
      A    => 1,
      B    => 2,
      C    => 3
    )
  );

begin

end architecture rtl;

Note: I have disabled a few other rules to produce the result above, for the purposes of demonstration:

rule:
  constant_012:
    disable: true
  constant_016:
    disable: true
  element_association_101:
    disable: true
  whitespace_005:
    disable: true
  whitespace_006:
    disable: true

Describe the solution you'd like I would like cosntant_400 to be able to handle separately align => keywords at different heirarchical levels. Apologies if this is complicated!

[JHertz5]

It would be good to have the same on sequential_400 and concurrent_400. With the following config

rule:
  concurrent_012:
    first_paren_new_line: 'no'
    assign_on_single_line: 'ignore'
  sequential_009:
    first_paren_new_line: 'no'
    assign_on_single_line: 'ignore'
  variable_assignment_008:
    first_paren_new_line: 'no'
    assign_on_single_line: 'ignore'
  element_association_100:
    disable: true
  concurrent_401:
    align_left: 'yes'
    align_paren: 'no'
  sequential_402:
    align_left: 'yes'
    align_paren: 'no'
  variable_assignment_004:
    align_left: 'yes'
    align_paren: 'no'
  variable_assignment_401:
    align_left: 'yes'
    align_paren: 'no'

the code below:

entity test is
end entity test;

architecture rtl of test is

begin

  test_signal <= (
    others => (
      data         => (others => '0'),
      update_pulse => '0'
    )
  );

  proc_test : process is
  begin

    test_signal <= (
      others => (
        data         => (others => '0'),
        update_pulse => '0'
      )
    );

    test_variable := (
      others => (
        data         => (others => '0'),
        update_pulse => '0'
      )
    );

  end process proc_test;

end architecture rtl;

produces the following output:

$ ./bin/vsg -c ./test.yml -f ./test.vhd
================================================================================
File:  ./test.vhd
================================================================================
Phase 5 of 7... Reporting
Total Rules Checked: 532
Total Violations:    2
  Error   :     2
  Warning :     0
----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  concurrent_400            | Error      |         10 | Move => 8 columns
  sequential_400            | Error      |         20 | Move => 8 columns
----------------------------+------------+------------+--------------------------------------

[jeremiah-c-leary]

Morning @JHertz5 ,

Question on your first example, would you want the assignments to be aligned per level? For example:

entity test is
end entity test;

architecture rtl of test is

  constant my_constant : my_type := (
    ENUM_1   => (
      A  => 1,
      B  => 2,
      C  => 3
    ),
    ENUM_224 => (
      AA => 1,
      BB => 2,
      CC => 3
    )
  );

begin

end architecture rtl;

where the ENUM_1 assignment is aligned with the ENUM_224s assignment and the A assignment is aligned to the AA assignment?

I would assume you would want each hierarchical element to be aligned independently like this:

entity test is
end entity test;

architecture rtl of test is

  constant my_constant : my_type := (
    ENUM_1 => (
      A => 1,
      B => 2,
      C => 3
    ),
    ENUM_224 => (
      AA => 1,
      BB => 2,
      CC => 3
    )
  );

begin

end architecture rtl;

That looks better to me since the ENUM_1 elements do not seem related to the ENUM_234 elements.

What do you think?

--Jeremy

[JHertz5]

Hi @jeremiah-c-leary. I agree, I also prefer the second snippet. As you said, the sets of elements are indepent from eachother. Thanks very much!

[jeremiah-c-leary]

Morning @JHertz5 ,

I just pushed an implementation for rule constant_400 to the issue-1081 branch. I will check on what it takes to implement this for sequential_400 and concurrent_400. In the mean time, could you check out the current implementation and let me know if it works on your end.

Thanks,

--Jeremy

[jeremiah-c-leary]

Afternoon @JHertz5 ,

I ran into something interesting if your second example:

  test_signal <= (
    others => (
      data         => (others => '0'),
      update_pulse => '0'
    )
  );

the data element is an aggregate of `others => '0', so my current implementation will fix the above code to:

  test_signal <= (
    others => (
      data => (others => '0'),
      update_pulse => '0'
    )
  );

Since data and update_pulse are at the same hierarchical level, but there is another aggragate between them.

Maybe there should be an option to ignore aggregates on a single line?

--Jeremy

[JHertz5]

I just pushed an implementation for rule constant_400 to the issue-1081 branch. I will check on what it takes to implement this for sequential_400 and concurrent_400. In the mean time, could you check out the current implementation and let me know if it works on your end.

Hi @jeremiah-c-leary. Thanks very much! I have tested this branch with the new option enabled, and it is looking good (aside from the next issue that you mentioned).

Maybe there should be an option to ignore aggregates on a single line?

That is a good catch and a very good point. I agree, that option sounds like a good solutiion Thanks again for you help, I appreciate that this request is not straightforward!

[jeremiah-c-leary]

Afternoon @JHertz5 ,

I pushed an update to the issue-1081 branch and added two options (aggregate_parens_ends_group and ignore_single_line_aggregates) to the rules sequential_400 and concurrent_400.

I would like to do some refactoring on these rules, but they appear to be functioning correctly.

Please let me know if there are any changes you would like to see.

Thanks,

--Jeremy

[JHertz5]

Hi @jeremiah-c-leary. Thanks very much for this, I have tested the branch and it is looking great! I just have one request - could you please also add the ignore_single_line_aggregates option to constant_400? The same issue is possible in a constant and could result in the following code:

  constant c_my_constant : my_type := (
    ENUM_1 => (
      A => 1,
      B => 2,
      C => 3
    ),
    ENUM_2 => (
      AA      => 1,
      BBBBBBB => ((others => '0')),
      CC => 3 -- Not aligned!
    )
  );

[jeremiah-c-leary]

Good Afternoon @JHertz5 ,

I finished the refactor and added the ignore_signle_line_aggregates to rule constant_400. Please check it out when you get the chance and let me know if I can merge it to master.

Thanks,

--Jeremy

[JHertz5]

Hi @jeremiah-c-leary. I've tried the branch and it is working perfectly, thank you so much! I'm happy for this to be merged. Again, I really appreciate your work on this.

[jeremiah-c-leary]

Awesome, I will merge this to master.

--Jeremy