Implement language server textDocument/hover

[j-zeppenfeld]

As discussed in chat, this series of commits adds support for textDocument/hover to show information about declaration nodes through the language server.

I was careful to implement hover using as much of the available infrastructure as possible, specifically the AST-traversal implemented via the Search trait. This is accomplished by replacing Searcher::search_decl_pos with Searcher::search_decl, which takes an additional enum variant (FoundDeclaration) with a reference back to the actual declaration node. The default implementation of search_decl simply calls search_decl_pos in order to be backwards compatible.

The most work was needed to implement the Display trait for the declarative AST types. Most types are formatted such that they are compatible with the associated parse function. There are a few exceptions where I do not include some fields in order to reduce verbosity. The types to which this applies are:

• TypeDefinition::Protected: Formats as "type foo is protected", missing items. This was the most difficult decision in this list, since it is all pure declaration and not as terribly verbose as some packages, but in use I found that including all declared items was still too much to be useful.
• TypeDefinition::ProtectedBody: Formats as "type foo is protected body", missing type_reference and decl. Internals of a protected body are implementation rather than declaration, so should not be visible to the user and are not displayed.
• PackageDeclaration: Formats as "package pkg_name", missing context_clause, and decl. Including all declarations within the package is simply way too verbose to be helpful.
• InterfacePackageDeclaration: Formats as "package foo is new lib.pkg", missing generic_map. This was a difficult decision, but in my opinion the generic map is part of the implementation rather than the declaration, so I left it out.
• PackageInstantiation: Formats as "package ident is new lib.foo.bar;", missing context_clause and generic_map. Same as InterfacePackageDeclaration - not really part of the declaration.
• ForGenerateStatement: Formats as "for idx in 0 to 1 generate", missing the body. The body can be arbitrarily complex and represents the implementation, rather than the declaration.
• ContextDeclaration: Formats as "context ident", missing items, i.e., the context clause. Again, generally too verbose, not really useful, and implementation rather than declaration. The idea of a context declaration is to hide the underlying implementation details from the user.
• ConfigurationDeclaration: Formats as "configuration cfg of entity_name", missing context_clause, decl, block_config and vunit_bind_inds, all of which make up the implementation rather than the declaration.
• EntityDeclaration: Formats as "entity foo is generic (...); port(...); end entity;", missing context_clause, decl and statements. Again, they really are not part of the declaration.

If you don't agree with some of the rationale above I'd be happy to make the relevant changes, otherwise I hope that the code meets your expectations.

[j-zeppenfeld]

I just realized that PackageDeclaration is missing unit tests containing a generic clause, so setting this to WIP.

[j-zeppenfeld]

Also, the trailing semicolon of PackageInstantiation should only be present if there is no generic map.

Alternatively, PackageInstantiation and InterfacePackageDeclaration should include the generic map.

[kraigher]

Isn't the search done twice here? Why does format_declaration need to search again when we already found it with search_reference. I assume it was because search_reference did not return the AST-value but in that case this function should be generalized to avoid searching twice.

[j-zeppenfeld]

There are two searches done here, yes, but they are different. The first searches for the token at the position provided over the language server protocol, and retrieves the referenced declaration from it. At the moment, that only provides the position of the declaration, so we need to search for the declaration itself in order to retrieve the AST node that we can actually format.
This second search could possibly be rendered obsolete if Reference were instead the ID of a NamedEntity (as you suggest below), but that would require two things:

1. Lookup of the named entity by id. This may already exist, in fact I assume it must, but I haven't found it yet.
2. Enough information within the named entity in order to reconstruct the VHDL code. This would probably be most easily achieved by referencing back into the AST in order to avoid duplication of information.
   Unfortunately, that second point is beyond my understanding of the code atm.

[kraigher]

This fix should really have been a separate PR. It creates value on its own and should not wait for the other stuff.

[j-zeppenfeld]

Agreed, I'll split it off.

[kraigher]

I think this type is redundant with NamedEntity in named_entity.rs The goal of analysis is to build a data structure of declarative regions with symbols (Which the LRM calls named entites). The idea was that Reference type should point to such a NamedEntity instead of just a SrcPos like it does today. The SrcPos was only a shortcut to get the goto-definiton feature which did not need to know anything more about the named entity other than the source position.

[kraigher]

Also, the trailing semicolon of PackageInstantiation should only be present if there is no generic map.

Alternatively, PackageInstantiation and InterfacePackageDeclaration should include the generic map.

I think it is better to leave the generic map out, it can be very long.

Edit: Maybe it should be included if function signatures are which seems to be the case in other languages.

[kraigher]

I am ok with the PR. We can try to refractor towards a NamedEntity in the future.

[j-zeppenfeld]

Yes, I think that would probably be the best way to go. The alternative would probably be to put this PR on hold for quite a while.
I have added the missing unit test for PackageDeclaration, and added a commit to include the generic map in the formatting of InterfacePackageDeclaration and PackageInstantiation.
Like you, I am unsure whether to keep it in or not. It isn't really part of the declaration of that instantiation after all, unlike the generic clause of the PackageDeclaration.
I hope it's ok if I leave that decision up to you - leave the commit in or throw it out as you like.

[Xcodo]

@j-zeppenfeld this is a fantastic improvement. I was missing this the other day (no time in life to help myself unfortunately 😞), really glad to have it now 👍.