Add markdown formatting to hover messages

[chioni16]

No description provided.

[seanyoung]

Rather than caching the rendered text in the hovers structure, is it possible to only render it on demand, i.e. the hovers contains enough information to render the text, but not the rendered text? I guess this would mean storing some sort of enum in the hovers structure.

[chioni16]

Rather than caching the rendered text in the hovers structure, is it possible to only render it on demand, i.e. the hovers contains enough information to render the text, but not the rendered text? I guess this would mean storing some sort of enum in the hovers structure.

@seanyoung Sure, that should be possible, but with a caveat.
Suppose we need to display the type of a variable in the hover message, here we can't store the Type directly in the enum.
mainly because in order to get the string representation of the type, we need the Namespace struct which may not be available when we want to render the whole enum as a string at the end, right before we send it back to the client.
So, we will. have to store the string version of Type in the enum.
Also storing the whole Type or Parameter struct can be memory intensive. It might be cheaper to store rendered string.
So, all we are doing at the end is to put these individual string components together to form the final hover message that we want to send to the client.

[xermicus]

I'd prefer to use the fully qualified syntax itertools::Itertools::intersperse(...) instead of disabling the lint (as clippy suggests)

[xermicus]

Commented out code is almost never kept in the source (if there is a good exception, you can explain in a comment why it should be kept instead).

[seanyoung]

Struct type definition members do not use markdown:

Struct member expression has no hover:

Missing hovers for for loops

Builtins have non-markdown hovers that a bit ugly.

Hovers should really say where a variable is a constant (and what value it has, if it is a constant) or contract variable.

[LucasSte]

Builtins have non-markdown hovers that a bit ugly.

We have descriptions for the builtins in builtins.rs. Is our intention to use them in the hovers description box?

[chioni16]

1. Struct type definition members do not use markdown:
   I have added the markdown for this.
2. Struct member expression has no hover:
   Hovering over struct member expression now displays its type, which is in accordance with the behaviour observed with hovers over variables.
3. Missing hovers for for loops
   Variable names of length one sometimes don't show hovers. I believe this has to do with the position sent (0 based indexing vs 1 based indexing, perhaps?)

4. Builtins have non-markdown hovers that a bit ugly.
   I believe this happens only when the solang::sema::builtin::get_prototype function can't find a suitable entry in solang::sema::builtin::BUILTIN_VARIABLE:. Maybe it's better to not display a hover message in such cases? Or display a generic message without the params, return values etc?
5. Hovers should really say where a variable is a constant (and what value it has, if it is a constant) or contract variable.
   I have added code to display if the variable is a constant / contract variable.

But I had some difficulties displaying the constant value. I decided to make use of Namespace::var_constants, which according to the comments

/// For a variable reference at a location, give the constant value
/// This for use by the language server to show the value of a variable at a location

But this doesn't seem to be working as expected.

[chioni16]

Builtins have non-markdown hovers that a bit ugly.

We have descriptions for the builtins in builtins.rs. Is our intention to use them in the hovers description box?

We are already making use of solang::sema::builtin::BUILTIN_VARIABLE in builtin.rs. I believe this is what you were referring to.

[chioni16]

Variable names of length one sometimes don't show hovers.

This may be due to the fact that the range (start, stop) isn't inclusive on stop. This causes an issue when start equals stop and the find method seems to skip such intervals.
from rust-lapper docs (https://docs.rs/rust-lapper/1.1.0/rust_lapper/):

So [start, stop) is not inclusive of the stop position for neither the queries, nor the Intervals.

I was able to get hovers displayed for variables of length 1 by incrementing the intervals' stop value by 1.

But adding 1 everywhere looks a bit weird.

TL;DR - rust-lapper expects half open ranges (start is included , end is excluded) but we have been passing inclusive ranges (both start and end are included). This discrepancy doesn't cause any problem unless the range is of length 1 (which happens in case of variable names of length 1).

[seanyoung]

@chioni16 this is already looking much better. I do have some more observations:

The Solidity syntax is uint32 x, uint32 y. x: uint32, y: uint32 is rust syntax (it's nicer, I agree!). There are also unnecessary spaces in the formatting of the function, and it does not include the contract name.

function test.bar(uint32 x, uint32 y) public returns (uint32)

There is also a needless newline between the doc comment and the prototype.

The comment/description now follows the definition, rather than before. Also this is not a function, yet we have ( ).

Wrong order of comment/description which is also getting incorrect Solidity syntax highlighting. Also unnecessary spacing.

What is : c? It's the contract then it would be better to have address c.token. Would be possible to include the constant value?

[seanyoung]

Single letter variables don't seem to work:

Enums give non-markdown hovers

Please can you can walk through all Solidity files in docs/ and ensure everything works.

(I do realize there is stuff missing because it's not in the ast, like import statements, but let's make sure that what's in the ast works.)

[seanyoung]

Thanks @chioni16