Environment master branch

VHDL Style Guide (VSG) version: 3.17.0.dev19
Git commit SHA: 92503eee

Describe the bug Rule selected_assignment_012, which adds carriage returns after the comma in choices, does not account for the fact that there could be commas within the selection that it should ignore, e.g. within function call.

To Reproduce Steps to reproduce the behavior:

Create a file "test.vhd" with the following contents:
```
architecture rtl of test is
```

begin

with select_signal select selected_signal <= input when to_unsigned(1, select_signal'length), 'X' when others;

end architecture rtl;

2. Run `vsg -f test.vhd`
3. See the following output

================================================================================ File: ./test.vhd

Phase 1 of 7... Reporting Total Rules Checked: 157 Total Violations: 1 Error : 1 Warning : 0 ----------------------------+------------+------------+-------------------------------------- Rule | severity | line(s) | Solution ----------------------------+------------+------------+-------------------------------------- selected_assignment_012 | Error | 6 | Move code after the comma to the next line. ----------------------------+------------+------------+-------------------------------------- NOTE: Refer to online documentation at https://vhdl-style-guide.readthedocs.io/en/latest/index.html for more information.


4. Note the `selected_assignment_012` has raised an error despite the code being as desired. Running the `--fix` argument causes the code to be messed up. The first comma, in `to_unsigned(1, selected_signal'length)` 

**Expected behavior**
I would expect this error not to be raised on the code.

I would be happy to have a go at making this change, but I am not sure what the best approach would be. I was thinking about adding a new class to vsg/token/selected_expressions.py and vsg/token/selected_waveforms.py called selection_comma which is a comma that is not within parenthesis. I would then pass that to rule_012 to instead of the normal the comma token. Does that make any sense?

Good Afternnon @JHertz5 ,

You can run a script called vsg_parser to see how VSG is classifying tokens in a file. This file is located in the bin directory where the top level vsg file is located.

In your example the following would be generated:

%bin/vsg_parser -f original.vhd
--------------------------------------------------------------------------------
0 |
--------------------------------------------------------------------------------
1 | architecture rtl of test is
<class 'vsg.token.architecture_body.architecture_keyword'>
<class 'vsg.token.architecture_body.identifier'>
<class 'vsg.token.architecture_body.of_keyword'>
<class 'vsg.token.architecture_body.entity_name'>
<class 'vsg.token.architecture_body.is_keyword'>
--------------------------------------------------------------------------------
2 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
3 | begin
<class 'vsg.token.architecture_body.begin_keyword'>
--------------------------------------------------------------------------------
4 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
5 |   with select_signal select selected_signal <=
<class 'vsg.token.concurrent_selected_signal_assignment.with_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.concurrent_selected_signal_assignment.select_keyword'>
<class 'vsg.token.concurrent_selected_signal_assignment.target'>
<class 'vsg.token.concurrent_selected_signal_assignment.assignment'>
--------------------------------------------------------------------------------
6 |     input when to_unsigned(1, select_signal'length),
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.parser.open_parenthesis'>
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.comma'>
<class 'vsg.parser.todo'>
<class 'vsg.parser.tic'>
<class 'vsg.token.predefined_attribute.keyword'>
<class 'vsg.parser.close_parenthesis'>
<class 'vsg.parser.comma'>
--------------------------------------------------------------------------------
7 |     'X'     when others;
<class 'vsg.parser.character_literal'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.concurrent_selected_signal_assignment.semicolon'>
--------------------------------------------------------------------------------
8 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
9 | end architecture rtl;
<class 'vsg.token.architecture_body.end_keyword'>
<class 'vsg.token.architecture_body.end_architecture_keyword'>
<class 'vsg.token.architecture_body.architecture_simple_name'>
<class 'vsg.token.architecture_body.semicolon'>

This shows on line 6 that the first comma encountered is being classified as vsg.token.selected_waveforms.comma instead of the last comma. So there is something not quite right with the classification process.

It looks like I was trying to shortcut the classifying of the choice production.

I will see what it will take to fully classify choice, otherwise I will find a way to take parenthesis into account.

--Jeremy

Good Evening @JHertz5 ,

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

I decided not to fully classify the simple_expression production at this time, but the code should take parenthesis into account when searching for the selected_waveform comma.

I ran your code with the latest updates and it looks like the alignment is off:

  1 architecture rtl of test is
  2
  3 begin
  4
  5   with select_signal select selected_signal <=
  6                                                input when to_unsigned(1, select_signal'length),
  7                                                           'X' when others;
  8
  9 end architecture rtl;

and I get two errors:

================================================================================
File:  fixed.vhd
================================================================================
Phase 5 of 7... Reporting
Total Rules Checked: 525
Total Violations:    2
  Error   :     2
  Warning :     0
----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  selected_assignment_400   | Error      |          6 | Indent with 4 space(s)
  selected_assignment_400   | Error      |          7 | Indent with 4 space(s)
----------------------------+------------+------------+--------------------------------------

when I try to fix the file.

I will need to debug that issue next.

If you could give this a try on your end I would appreciate it.

Thanks,

--Jeremy

Hi @jeremiah-c-leary. Thanks for looking in to this. I've checked out your branch and I can replicate your results, so the branch solves this issue, thanks very much!

On the alignment issue, I should have mentioned earlier, but those issues are also present on the master branch, I was planning to raise it as a separate issue. They go away when I disable rule concurrent_selected_signal_assignment_400, so I think that's where the problem lies.

Output with default rule configuration:

architecture rtl of test is

begin

  with select_signal select selected_signal <=
                                               input when to_unsigned(1, select_signal'length),
                                                          'X' when others;

  -- concurrent_selected_signal_assignment_400 snippet
  with mux_sel select addr <=
                              c_input_data when 0,
                                                c_output_data when 1,
                                                (others => 'X') when others;

  -- selected_assignment_400 snippet
  with (mux_select or reset) select addr <=
                                            "0000" when 0,
                                                        "0001" when 1,
                                                        "1111" when others;

end architecture rtl;

Output with concurrent_selected_signal_assignment_400 disabled:

architecture rtl of test is

begin

  with select_signal select selected_signal <=
    input when to_unsigned(1, select_signal'length),
    'X' when others;

  -- concurrent_selected_signal_assignment_400 snippet
  with mux_sel select addr <=
                              c_input_data when 0,
                                                c_output_data when 1,
                                                (others => 'X') when others;

  -- selected_assignment_400 snippet
  with (mux_select or reset) select addr <=
    "0000" when 0,
    "0001" when 1,
    "1111" when others;

end architecture rtl;

The snippet for concurrent_selected_signal_assignment_400 doesn't even align correctly, which suggests to me that the rule may be broken?

Perhaps concurrent_selected_signal_assignment_400 is trying to modify code at the same time as selected_assignment_400 and it's causing a conflict? concurrent_selected_signal_assignment_400's default behaviour is to align with the assignment operator, while selected_assignment_400's default behaviour looks like it left-aligns. I'm sure it is not a simple change and perhaps there are practical reasons as to why this wouldn't work, but I think a "philosophically correct" solution would be to have only one rule to cover all selected assignments.

Again, your branch looks to have solved the parenthesis issue, so if you agree, I'll raise a separate issue for this alignment problem.

Morning @JHertz5 ,

This sounds very familiar.

Perhaps concurrent_selected_signal_assignment_400 is trying to modify code at the same time as selected_assignment_400 and it's causing a conflict?

That is correct. Both rules are being applied so if they are not configured the same way then one will fail.

I'm sure it is not a simple change and perhaps there are practical reasons as to why this wouldn't work, but I think a "philosophically correct" solution would be to have only one rule to cover all selected assignments.

One of the struggles I have with VSG is trying to balance the number of rules against how the productions are defined. I want to name rules based on the LRM, but at the same time I want to minimize the number of rules. In this example, there are four selected assignment productions:

This diagram maps how they are related:

So I remember trying to decide if I should have four separate rules that basically did the same thing or to combine them into a single rule. According to the map I should have made two rules: selected_signal_assignment and selected_variable_assignment. However, they are basically the same, so I must have decided to combine them into selected_assignment. Unfortunately, selected_assignment does not exist in the LRM.

If I split up selected_assignment, then I will also have duplicated 26 rules, which essentially check for the same thing. I want to be internally consistent, but at the same time having 52 rules instead of 26 does not seem very efficient.

I believe I just need to remove concurrent_selected_signal_assignment_400 and let selected_assignment_400 handle the alignment.

--Jeremy

Hey @JHertz5 ,

I just pushed an update where I removed rule concurrent_selected_signal_assignment_400. let me know how that works for you.

--Jeremy

Hi @jeremiah-c-leary. That makes sense. I can definitely see how that would be tough to manage! I have given the updated branch a try, and it looks to be mostly working, it gives the same output as in my previous comment in which I had disabled concurrent_selected_signal_assignment_400.

There is still one problem, though - it looks like selected_assignment_400 isn't running on the concurrent_selected_signal_assignment_400 snippet. In the code below, the middle select statement isn't being aligned.

architecture rtl of test is

begin

  with select_signal select selected_signal <=
    input when to_unsigned(1, select_signal'length),
    'X' when others;

  -- concurrent_selected_signal_assignment_400 snippet
  with mux_sel select addr <=
                              c_input_data when 0,
                                                c_output_data when 1,
                                                (others => 'X') when others;

  -- selected_assignment_400 snippet
  with (mux_select or reset) select addr <=
    "0000" when 0,
    "0001" when 1,
    "1111" when others;

end architecture rtl;

I've run vsg_parser and it looks like the concurrent_selected_signal_assignment.with_keyword token on line 10 and the concurrent_selected_signal_assignment.semicolon token on line 13 are both being recognised correctly so I don't see why the rule wouldn't be running.

--------------------------------------------------------------------------------
0 |
--------------------------------------------------------------------------------
1 | architecture rtl of test is
<class 'vsg.token.architecture_body.architecture_keyword'>
<class 'vsg.token.architecture_body.identifier'>
<class 'vsg.token.architecture_body.of_keyword'>
<class 'vsg.token.architecture_body.entity_name'>
<class 'vsg.token.architecture_body.is_keyword'>
--------------------------------------------------------------------------------
2 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
3 | begin
<class 'vsg.token.architecture_body.begin_keyword'>
--------------------------------------------------------------------------------
4 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
5 |   with select_signal select selected_signal <=
<class 'vsg.token.concurrent_selected_signal_assignment.with_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.concurrent_selected_signal_assignment.select_keyword'>
<class 'vsg.token.concurrent_selected_signal_assignment.target'>
<class 'vsg.token.concurrent_selected_signal_assignment.assignment'>
--------------------------------------------------------------------------------
6 |     input when to_unsigned(1, select_signal'length),
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.parser.open_parenthesis'>
<class 'vsg.parser.todo'>
<class 'vsg.parser.comma'>
<class 'vsg.parser.todo'>
<class 'vsg.parser.tic'>
<class 'vsg.token.predefined_attribute.keyword'>
<class 'vsg.parser.close_parenthesis'>
<class 'vsg.token.selected_waveforms.comma'>
--------------------------------------------------------------------------------
7 |     'X' when others;
<class 'vsg.parser.character_literal'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.token.choice.others_keyword'>
<class 'vsg.token.concurrent_selected_signal_assignment.semicolon'>
--------------------------------------------------------------------------------
8 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
9 |   -- concurrent_selected_signal_assignment_400 snippet
<class 'vsg.parser.comment'>
--------------------------------------------------------------------------------
10 |   with mux_sel select addr <=
<class 'vsg.token.concurrent_selected_signal_assignment.with_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.concurrent_selected_signal_assignment.select_keyword'>
<class 'vsg.token.concurrent_selected_signal_assignment.target'>
<class 'vsg.token.concurrent_selected_signal_assignment.assignment'>
--------------------------------------------------------------------------------
11 |                               c_input_data when 0,
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.comma'>
--------------------------------------------------------------------------------
12 |                                                 c_output_data when 1,
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.comma'>
--------------------------------------------------------------------------------
13 |                                                 (others => 'X') when others;
<class 'vsg.token.aggregate.open_parenthesis'>
<class 'vsg.token.choice.others_keyword'>
<class 'vsg.token.element_association.assignment'>
<class 'vsg.parser.character_literal'>
<class 'vsg.token.aggregate.close_parenthesis'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.token.choice.others_keyword'>
<class 'vsg.token.concurrent_selected_signal_assignment.semicolon'>
--------------------------------------------------------------------------------
14 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
15 |   -- selected_assignment_400 snippet
<class 'vsg.parser.comment'>
--------------------------------------------------------------------------------
16 |   with (mux_select or reset) select addr <=
<class 'vsg.token.concurrent_selected_signal_assignment.with_keyword'>
<class 'vsg.parser.open_parenthesis'>
<class 'vsg.parser.todo'>
<class 'vsg.token.logical_operator.or_operator'>
<class 'vsg.parser.todo'>
<class 'vsg.parser.close_parenthesis'>
<class 'vsg.token.concurrent_selected_signal_assignment.select_keyword'>
<class 'vsg.token.concurrent_selected_signal_assignment.target'>
<class 'vsg.token.concurrent_selected_signal_assignment.assignment'>
--------------------------------------------------------------------------------
17 |     "0000" when 0,
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.comma'>
--------------------------------------------------------------------------------
18 |     "0001" when 1,
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.comma'>
--------------------------------------------------------------------------------
19 |     "1111" when others;
<class 'vsg.parser.todo'>
<class 'vsg.token.selected_waveforms.when_keyword'>
<class 'vsg.token.choice.others_keyword'>
<class 'vsg.token.concurrent_selected_signal_assignment.semicolon'>
--------------------------------------------------------------------------------
20 |
<class 'vsg.parser.blank_line'>
--------------------------------------------------------------------------------
21 | end architecture rtl;
<class 'vsg.token.architecture_body.end_keyword'>
<class 'vsg.token.architecture_body.end_architecture_keyword'>
<class 'vsg.token.architecture_body.architecture_simple_name'>
<class 'vsg.token.architecture_body.semicolon'>

If I remove the (others => 'X') from the code and replace it with a string that looks like a normal signal name, the rule runs correctly. Do you have any ideas on what could be causing this? Maybe the others keyword is being classified incorrectly and that's confusing things? Sorry that this is turning out to be such a pain!

Hey @JHertz5 ,

If I remove the (others => 'X') from the code and replace it with a string that looks like a normal signal name, the rule runs correctly. Do you have any ideas on what could be causing this?

I found the issue. If there was an open parenthesis, the selected assignment was being thrown away because it was being considered as an array. That is probably a long story...

Sorry that this is turning out to be such a pain!

No problem. This forces me to fix poor decisions I made earlier and make the code better.

Could you check the latest update on the branch?

Thanks,

--Jeremy

@jeremiah-c-leary that makes sense. I've tested the latest branch and it is working perfectly for me. Thanks very much!

jeremiah-c-leary / vhdl-style-guide

`selected_assignment_012` adds carriage returns after commas within function calls #1033

================================================================================ File: ./test.vhd