library_001 reports wrong alignment for context (VHDL-2008)

[staerz]

Environment VSG 1.6.1., Fedora

Describe the bug The library_001 complains in a context environment (VHDL-2008) where the indentation is actually correct.

To Reproduce Run VSG on the following piece of code:

context components is
  library fpga;
  context fpga.interfaces;
end context;

Expected behavior I would not expect library_001 to complain here as the indentation is correct.

Additional context Pun intended :rofl:

See here: https://www.doulos.com/knowhow/vhdl_designers_guide/vhdl_2008/vhdl_200x_merged/#context

[jeremiah-c-leary]

I think I need to add some context rules also.

[staerz]

I think I need to add some context rules also.

Yes, I think that would be helpful :+1:

(FYI: I'm just testing VSG 2.0.0 and that one, of course, still remains.)

[jeremiah-c-leary]

Hey @staerz ,

Could you checkout the issue-393 branch and let me know if that fixes the issue for you.

I added 27 rules for context declarations. I have not implemented context_028 yet, which will align comments within a context declaration. I wanted your feedback before I tackled that one.

Also, let me know if there are any rules I may have missed.

[staerz]

Hi @jeremiah-c-leary,

I have cloned the repo, checked out the branch and ran the install script, but it seems it doesn't bring a difference.

How can I check if the version in that branch is really being used? Of course, I tried vsg --version, but this only gives me a "VHDL Style Guide (VSG) version 2.0.0", but not the commit SHA.

The reason I'm asking is that I don't see any change.

The file I'm checking is this:

context constants is
  library IEEE;
  context IEEE.IEEE_STD_CONTEXT;
  library PoC;
  use PoC.utils.all;
  library fpga;
  use fpga.fpga_cst.all;
end context;

and then later (in a different file, using this context) this:

library fpga;
--! @cond
  context fpga.constants;
--! @endcond

package fpga_if is

[jeremiah-c-leary]

hmm...I would expect your first file to look like:

context constants is

  library IEEE;
  context IEEE.IEEE_STD_CONTEXT;
  library PoC;
  use PoC.utils.all;
  library fpga;
  use fpga.fpga_cst.all;

end context constants;

You could use the -oc option to output all the rules and their configurations. Then check the output json file for context.

[staerz]

Alright, I would have to "pip uninstall vsg" first, and then install the local version using your install script.

So yes, absolutely, now VSG complains about the missing blank lines and wants the "context" in the last line.

But I now also get a fake violation on the package file fpga_if.vhd:

  context_021               |        191 | Add "context" after "end"

"For the record" (I like puns :rofl:), this looks like this:

  type avmm_master_t is record
    addr:   std_logic_vector(AVMM_ADDR_W-1 downto 0);
    data:   std_logic_vector((2**AVMM_DATA_WEXP)-1 downto 0);
    read:   std_logic;
    write:  std_logic;
  end record;

(line 191 is the last one here).

Edit: Which looks like your context_021 is triggered by the initial context fpga.constants; (as part of the library fpga statement, and falsely identifies this as a context declaration, while it is a context usage. The difference, of course, is the is in the declaration, or the surrounding library in the usage to make the distinction...

[staerz]

And a very different remark: I now only realise that you use 2-lettered short options in VSG, which I think is highly uncommon.

Being more of an expert for bash than for python, I'd say short options are to be one letter only, while long options is anything more than one character and then is indicated with a leading double hyphen.

I recommend you consider this for a VSG 3.0.0 ...

[jeremiah-c-leary]

"For the record" (I like puns 🤣), Nice!!

So I currently handle the declaration of a context, but not it's ... instantiation?. My guess is my parser is considering the end to be part of the context declaration.

Thanks for checking that out. I will add support for calling out the context.

[jeremiah-c-leary]

And a very different remark: I now only realise that you use 2-lettered short options in VSG, which I think is highly uncommon.

Agree. I would like to revamp my command line arguments. I seem to be collecting quite a few.

[staerz]

"For the record" (I like puns rofl), Nice!!

So I currently handle the declaration of a context, but not it's ... instantiation?. My guess is my parser is considering the end to be part of the context declaration.

Thanks for checking that out. I will add support for calling out the context.

In this e-book I found the term "context reference", and also the IEEE Standard VHDL Language Reference Manual (ftp://ftp.lpp.polytechnique.fr/jeandet/keep/sync/vhdl/4772740_IEEE-1076_Standard-VHDL-Language-Ref-Manual.pdf) calls it "context_reference" (chapter 13.3 and 13.4)

[jeremiah-c-leary]

and also the IEEE Standard VHDL Language Reference Manual (ftp://ftp.lpp.polytechnique.fr/jeandet/keep/sync/vhdl/4772740_IEEE-1076_Standard-VHDL-Language-Ref-Manual.pdf)

Thanks a lot. The version I had was really vague on context statements.

[staerz]

Hi @jeremiah-c-leary,

and after a little break, you know, actually, the format of the context file should even be a little more indented:

context constants is

  library IEEE;
    context IEEE.IEEE_STD_CONTEXT;
  library PoC;
    use PoC.utils.all;
  library fpga;
    use fpga.fpga_cst.all;

end context constants;

Note the extra indentation for the context and use keyword after the library indication. This extra indentation currently throws a

  library_008               |         17 | Invalid indentation.
  library_008               |         19 | Invalid indentation.

(17 and 19 are the use lines).

And further note (It's obvious, but to be complete):

context interfaces is

  library fpga;
    context fpga.constants;
    use fpga.fpga_if.all;

end context interfaces;

(multiple) context and a (/multiple) particular package(s) can be used from the same library, of course.

[jeremiah-c-leary]

And further note (It's obvious, but to be complete):

That helps me a lot. I like the use being indented from the library. The context seemed to behave like a use statement so it should be at the same level.

Parsing the context reference was a real pain. Both the reference and the declaration start with the same keyword context. So when I run into it, I have no idea which one it is. I think I finally nailed it though.

I writing rules for context references now.

[jeremiah-c-leary]

Hey @staerz , I just pushed my latest updates for this issue. Could you update again and try it out?

Thanks,

--Jeremy

[staerz]

On the constants (as posted above):

  context_ref_004           |         15 | Change "IEEE.IEEE_STD_CONTEXT" to "ieee.ieee_std_context"

really? :rofl:

Well, that's a matter of taste maybe, but I agree, for consistency, it is alright.

On a usage

library fpga;
context fpga.interfaces;

I get

  context_ref_001           |         16 | Indent 2 spaces

which is absolutely valid. Saying your check works here.

No problem on the

context interfaces is
...

So also that seems alright!

PS: Let me know which rules I should possibly re-enable as I still have a few disabled due to the context problem .... Looks like you have it!

[jeremiah-c-leary]

context_ref_004 | 15 | Change "IEEE.IEEE_STD_CONTEXT" to "ieee.ieee_std_context"

I struggled whether to include that rule. What would you think of disabling that rule by default? That way it is there for someone to use if they want. I do not have a strong opinion about the case of libraries.

PS: Let me know which rules I should possibly re-enable as I still have a few disabled due to the context problem .... Looks like you have it!

Sweet. You can enable all the context, context_ref, and library rules.

I need to do some refactoring before I pull this into master. Hopefully in the next couple of days.

[staerz]

Hi @jeremiah-c-leary,

alright, so on the upper/lower case for the library and context, see 16.9 in the ftp Link (IEEE specs):

context IEEE_BIT_CONTEXT is
  library IEEE;
  use IEEE.NUMERIC_BIT.all;
end context IEEE_BIT_CONTEXT;

context IEEE_STD_CONTEXT is
  library IEEE;
  use IEEE.STD_LOGIC_1164.all;
  use IEEE.NUMERIC_STD.all;
end context IEEE_STD_CONTEXT;

(note that they don't indent the use clause - but that would be sth. for your suggested indentation configuration, where simply a 0 would do that.)

So I think what I would do:

• go with this by default

Now, to make that in line with the rest of the VSG configuration, I'd propose:

• make a rule configuration for upper or lower casing the context (and library (name and use clause) - this you have...) names, lower is default (as you have that as general default)
• add an extra rule configuration for IEEE (where, against your global default, upper case would be the default). This rule should pick up on all the IEEE-specific syntax in the above example. so IEEE for library is fine, IEEE_STD_CONTEXT and IEEE_BIT_CONTEXT are valid contexts, IEEE\..*are valid use clauses.

Now with these 2 configurations you would, by default, have everything in lower case except for the IEEE library part. And a user with a particular preference could opt to all upper case, all lower case, or all lower case, but IEEE upper case (or, even all upper case, except IEEE libraries in lower case ...)

Now if you put these 2 configurations per rule, or make them global, I wouldn't have a preference.

So that's my view on this :wink:

[jeremiah-c-leary]

I could make it generic by using an attribute named always_uppercase_words_starting_with and default it to a list with IEEE in it. That attribute would be in the case class so it would apply to every case check. Then it could be overridden by the user on a rule by rule basis.

[staerz]

Yes, that's what I also though about after I sat back a bit as you have that feature for architecture-025 which I find very well suited and flexible enough to accommodate the idea expressed above.

Though watch out, your always_uppercase_words_starting_with wouldn't allow to intentionally do the inverse casing. I think what I'd rather do is an exception list - and this list could contain any casing, even mixed casing like CamelCase(who ever might use it).

I.e. we have a library called "PoC" where I'd like to enforce this writing (capital P and C, but lower case o). If I could dump that library name into a list of pre-defined exceptions, that'd solve my troubles.

And now, you could make this a rule-specific or a global attribute, depending on what is better in performance (I think a global attribute might make VSG quite slow the longer the list gets, while dedicated rules would though be a bit more work for the maintenance, but keep the speed up - but maybe I'm thinking too 1990 :rofl:

[jeremiah-c-leary]

I think what I'd rather do is an exception list - and this list could contain any casing, even mixed casing like CamelCase(who ever might use it).

That is an interesting idea. A list of correct word capitalizations. Then we parse the string for the word. If we find a lowercase version of it, then we compare it to what is in the list. For the fix, we just use what is in the list.

If it is not in the list, then we upper or lower case depending on the how the rule is configured.

[staerz]

Yeah well, just complaining how bad things are isn't really helpful, so proposing a possible solution once in a while does actually put things forward, although I won't be able to contribute directly to the code (my python knowledge is --)

[jeremiah-c-leary]

Hey @staerz ,

I would like to get this issue pushed to master to address the error. Would you be okay if I created a new issue to add case exception?

[staerz]

Hey @staerz ,

I would like to get this issue pushed to master to address the error. Would you be okay if I created a new issue to add case exception?

Hi @jeremiah-c-leary,

absolutely. Keeping the issues small is what a good developer should do :+1:

[staerz]

Hi @jeremiah-c-leary,

I'm just trying it out and it looks good to me.

Since I now pull directly on the repo, I see all the updates. I see that you add tests for every single rule. I wish that everybody would do like this! This is excellent! (And I only use this word when I mean it.I)

One remark when looking into your (latest) documentation: "context_011" you propose this check for the semicolon. Why do you make your life so complicated: Just introduce a rule (sorry, I didn't check if you have anything alike...) "^\s*;" (I guess you're very familiar with regex): Any line with the first non-whitespace character being the semicolon. And you're done for good, for any VHDL keyword.

[staerz]

Ah, and the new "contextref..." rules do not appear in the documentation, is that expected?

[jeremiah-c-leary]

Good catch. I spent all that time writing them and did not include them.

[jeremiah-c-leary]

I just pushed a commit to include them.