Python Exception due to uninitialized variable dExpectedIndent under certain conditions.

[zack-vii]

Environment Current master branch (latest tested with commit 57e7f8b)

Describe the bug Under certain conditions the indentation checker runs into the problem of an uninitialized variable.

To Reproduce Steps to reproduce the behavior:

1. create a vhd-file 'test.vhd'

   library ieee;
   use ieee.std_logic_1164.all;
   entity INDENT_TEST is
   end entity INDENT_TEST;
   architecture BEHAVIORAL of INDENT_TEST is
   -- line break after ':=' assignment may break indentation detector
   constant const : std_logic_vector(63 downto 0) :=
   "1010101010101010101010101010101010101010101010101010101010101010";
   begin
   end architecture BEHAVIORAL;

2. create style file 'test.yml'

   rule:
   global:
   style: no_blank_line
   align_left: yes

3. run vsg on file 'test.vhd' with configuration in 'test.yml' `git/vhdl-style-guide/bin/vsg -c ./test.yml -f ./test.vhd

Expected behavior Normal program execution with report

Screenshots Instead you get a python exception

multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/usr/lib/python3.9/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/home/cloud/.local/lib/python3.9/site-packages/vsg/apply_rules.py", line 111, in apply_rules
    oRules.check_rules(
  File "/home/cloud/.local/lib/python3.9/site-packages/vsg/rule_list.py", line 229, in check_rules
    oRule.analyze(self.oVhdlFile)
  File "/home/cloud/.local/lib/python3.9/site-packages/vsg/rule.py", line 143, in analyze
    self._analyze(lToi)
  File "/home/cloud/.local/lib/python3.9/site-packages/vsg/rules/multiline_alignment_between_tokens.py", line 150, in _analyze
    if indents_match(dActualIndent[iLine], dExpectedIndent[iLine]):
UnboundLocalError: local variable 'dExpectedIndent' referenced before assignment
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/git/ttev2-fw/release/../format/python/bin/vsg", line 15, in <module>
    main()
  File "/home/cloud/.local/lib/python3.9/site-packages/vsg/__main__.py", line 142, in main
    for tResult in pool.imap(f, enumerate(commandLineArguments.filename)):
  File "/usr/lib/python3.9/multiprocessing/pool.py", line 870, in next
    raise value
UnboundLocalError: local variable 'dExpectedIndent' referenced before assignment

Additional context Can be fixed by initializing dExpectedIndent with dActualIndent and effectively fall back to no-change.

--- a/vsg/rules/multiline_alignment_between_tokens.py
+++ b/vsg/rules/multiline_alignment_between_tokens.py
@@ -127,6 +127,7 @@ class multiline_alignment_between_tokens(alignment.Rule):

             iFirstTokenLength = len(lTokens[0].get_value())

+            dExpectedIndent = dActualIndent
             if self.align_paren == 'no' and self.align_left == 'yes':
                 dExpectedIndent = _analyze_align_left_yes_align_paren_no(iFirstLine, iLastLine, lParens, self.indentSize, dActualIndent, bStartsWithParen, self.bIgnoreStartParen, self.override)
             if self.align_paren == 'yes' and self.align_left == 'no':

[alonbl]

I had this as well, this is because yes and no in yaml are translated into boolean while this action requires yes and no as strings.

BTW: there are many places in which yes and no are used as boolean but actually mean strings, should fix all for consistency.

Using current implementation please use:

rule:
  global:
    style: no_blank_line
    align_left: 'yes'

[jeremiah-c-leary]

Good Evening @zack-vii ,

I pushed an update for this to the issue-1009 branch and was wondering if you could test it out on your end and let me know if the issue is resolved. I added a couple of functions to test for strings and booleans on those parameters.

Regards,

--Jeremy

[alonbl]

Hello @jeremiah-c-leary ,

I suggest a function such as the following instead of code repeating.

def fixup_boolean(candidate):
    return candidate is True or candidate == 'yes'

Please also consider to use it for all over the boolean variables that were documented as 'yes'/'no', and fixup the documentation to use booleans instead of 'yes'/'no' from now on.

Thanks,

[zack-vii]

I just tested this branch and it seems to format the respective lines now. I agree however with @alonbl, to encapsulate the interpretation of the yaml value in helper functions. Probably for consistency if applicable one would probably add as_bool() as_string() as_int() as_float() which would do the appropriate conversion or throw an exception if no trivial conversion is possible. this would catch most unexpected behavior related to broken yaml configuration. thanks for looking into it.

[jeremiah-c-leary]

Good Evening @alonbl and @alonbl ,

I updated my tests and injected booleans in addition to yes and no and then updated the rules which failed. I decided to keep the yes and no in the documentation due to other options in some rule configurations such as ignore. I feel it keeps it consistent to have strings instead of booleans and strings as possible values for the same option.

All tests are passing. Let me know if you want to test the latest update or if you want me to merge it to master as is.

--Jeremy

[alonbl]

Hello @jeremiah-c-leary ,

Thank you for making it more consistent, I see this was quite an investment.

I looked at the configuration and there are still cases that are not consistent with removing boolean usage:

$ ./bin/vsg -oc a.json
$ grep -P "(true|false)" a.json | grep -v -P "(disable|fixable)" | wc -l
259
$ grep -P "(yes|no)" a.json | wc -l
113

It is not that important, however, it is very confusing that some variables accepts string and some accepts boolean, especially in the yaml approach of guessing types based on words.

Regardless, If string is the approach the -oc should output yes/no and not boolean for these settings.

Regards,

[jeremiah-c-leary]

Good Evening @alonbl,

I switch all the specific configurable from true/false to yes/no except for the fixable and disable options. So essentially any rule specific configuration option will be always be a string. If a bare yes or no is used, it will be converted into a string for those rules that require strings. Otherwise, if a 'yes' or a 'no' will be converted to a boolean if a rule requires a boolean.

I still need to update documentation though.

Regards,

--Jeremy

[alonbl]

Thank you @jeremiah-c-leary ,

I see this was not trivial.

You have some trailing spaces, not sure if on purpose.

I can help you integrate pre-commit into your project if you like, my change #1011 adds the vsg tool as pre-commit for other projects.

I also do not understand how the following change is related:

-            dAnalysis[iKey]['adjust'] = iMaxTokenColumn - dAnalysis[iKey]['identifier_column'] + 1
+            dAnalysis[iKey]['adjust'] = iMaxTokenColumn - dAnalysis[iKey]['identifier_column']

[jeremiah-c-leary]

Morning @alonbl ,

I see this was not trival

Yeah, there was quite a bit of technical debt I had to overcome and I seem to have some issues with consistency as the code has evolved.

I can help you integrate pre-commit into your project if you like...

I do have code climate running, but that occurs after I push to GitHub. How do you setup a pre-commit hook?

I also do not understand how the following change is related:

It turns out I did not have a test for the False/no case of the that rule. When I added it to verify the conversion of the no to a boolean, the rule was adding a space. This meant the rule could never pass.

I fixed the extra lines and spaces pointed out by code climate.

I am going to work on updating to the documentation to move the True/False to yes/no.

Regards,

--Jeremy

[alonbl]

That's great.

After we finish with the current patch queue, I can help you to migrate the project into pyproject.toml and use tox to build/test with pre-commit source validations executed by tox as well. I would like to merge the existing queue first and drop my private repo we are using for now.

[jeremiah-c-leary]

Good Evening @alonbl and @zack-vii ,

I am finally done scrubbing the true and false from the documentation and replacing them with yes and no. I also updated documentation to my current standard. No most of the configuration documentation is consistent in style.

I will plan to merge this to master over the weekend.

Let me know if I missed anything or if I should make any other changes.

Regards,

--Jeremy