Support highlighting Markdown/RST in comments

[taminomara]

This PR adds an optional feature that highlights Markdown, MySt (a Markdown extension used with Sphinx), and RST syntax in comments and provides Go To Definition implementation for cross-references. This feature is disabled by default; it can be enabled by specifying doc.syntax in .emmyrc.json.

Caveats

• We use Semantic Tokens to provide syntax highlighting. Since there are no Semantic Tokens for bold and italic text, those are not highlighted.

[taminomara]

Note: this is a rather large PR, see individual commits to get more digestible diffs.

[CppCXY]

Sorry, but I do not agree with any changes to the AST, especially those that use regular expressions to match and modify the usage of regions.

[taminomara]

especially those that use regular expressions to match and modify the usage of regions.

I can redo those without regexes, but might you tell me what exactly you disagree with and why?

• requiring spaces after region markers,
• adding new tokens,
• adding new nodes,
• the whole concept in general?

[taminomara]

...

• handling of whitespaces in beginning of comments?

[CppCXY]

You just want to highlight the documentation in comments, so you only need to parse the comments once during semantic token request and highlight them in the appropriate places, rather than modifying the entire structure of the syntax tree.

[taminomara]

Hm, I see your point. I can implement this. Although there will be complications if we want to support Go To Definition in comments.

I see it like this:

• make a cache for comment markup, in case if IDE requests semantic tokens twice;
• in Go To Definition request, if the user clicked inside of the documentation comment, we can check the comment markup cache to see if there's a cross reference in that place;
• in Document Selection Range request, we can either ignore comments, or use comment markup cache once again.

Would you agree with this implementation?

[CppCXY]

This is a multithreaded language server, and caching introduces data races

[CppCXY]

Even if you re-parse everything, it will still be faster than using a cache.

[taminomara]

Comment markup cache doesn't need its own AST, it will only save highlighted ranges.

[taminomara]

caching introduces data races

I can make cache async/multi-threaded, that's not a big issue.

Even if you re-parse everything, it will still be faster than using a cache.

That's possible, I'll need to benchmark this.

[taminomara]

I'll start with re-parsing on every request, and see if caching is warranted later.

Two more questions:

1. do you agree with changes in 06cc982? I've struggled with this behavior quite a bit, because no other language server does this.
2. would you be interested in a separate PR that requires a space/end-of-line after "#region", provided I don't use regexes for that? Current behavior seems a bit too unspecific to me.

[taminomara]

Reworked this PR to avoid changes to AST.

Caching wasn't required, it's fast enough to re-parse docs on every relevant request.

[CppCXY]

When the code block area uses Lua syntax, should we use luaLexer to parse it and highlight keywords, strings, and numbers?

[taminomara]

When the code block area uses Lua syntax, should we use luaLexer to parse it and highlight keywords, strings, and numbers?

Done:

[CppCXY]

Your changes to error have caused the error location to change.

local p

--- @return fun(): (integer?, any?, ...)
local function f(tasks)
end

[CppCXY]

I got error:

thread 'tokio-runtime-worker' panicked at crates\emmylua_parser\src\text\text_range.rs:18:9:
assertion failed: start_offset <= end_offset
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'tokio-runtime-worker' panicked at crates\emmylua_parser\src\text\text_range.rs:18:9:
assertion failed: start_offset <= end_offset

---```lua
---
--- local t = 213
---```
---
--- .. code-block:: lua
---
---    local t = 123
---    yes = 1123
local t = 123

[taminomara]

Fixed bugs, added more tests, implemented autocomplete for ref targets, improved go to definition to handle modules.

[CppCXY]

Why does the highlighting look like this?

[taminomara]

based on conversations I've had with people, this will be polarising to a lot of people

Just for context, is that because highlighting in comments is distracting for some people, or are there other concerns?

Does it only parse comments with --- or all comments? If not I suggest this is limited to only --- comments.

I've disabled highlighting for any comment that doesn't start with three dashes. It should also detect comment blocks adorned with long dashed lines:

---------------   <- not highlighted
--- Comment       <- highlighted
---------------   <- highlighted
--- Comment       <- highlighted
---------------   <- not highlighted

---------------   <- not highlighted
-- Comment        <- not highlighted
---------------   <- not highlighted
-- Comment 2      <- not highlighted
---------------   <- not highlighted

Can we configure whether to use markdown or rst?

Yes. Also, std will use markdown even if user configured rst.

[lewis6991]

Just for context, is that because highlighting in comments is distracting for some people, or are there other concerns?

Yes, and I've also had someone say they want treesitter (which has more highlights and is faster) to handle this kind of highlighting, not the server.

[taminomara]

Yes, and I've also had someone say they want treesitter (which has more highlights and is faster) to handle this kind of highlighting, not the server.

Then there should be a way to disable highlighting even if emmyrc specifies otherwise. I think I'll do it as a separate PR though.

[taminomara]

I ran parsing and completions on my codebase, fixed some minor things. I believe I've done all features I wanted for this PR, it's ready for merge.

In next PR I'll add option to force-disable markdown parsing even if .emmyrc enables it; it will be coupled with updates to VSCode plugin to expose this new option to users.

[clason]

Please don't invent new off-spec options that only VS Code is set up to handle it. It's enough to make this opt-in via .emmrc.json (or the LSP configuration requests), anything beyond that makes it much more (and too) difficult to reason about.

If you want to do anything, split the doc.syntax option into two: one for enabling/disabling it, and one that controls the type of syntax. This way, the latter can be set per-configuration in a local .emmyrc.json while enabling this extension can be done in the client configuration.

I would also (strongly) recommend to name and structure the options in a way to make clear that they relate to semantic tokens. You probably also want to document the (off-spec?) tokens you send for documentation.

[taminomara]

If you want to do anything, split the doc.syntax option into two

Do you think it's needed? I like this idea, but I don't have a strong opinion here.

I would also (strongly) recommend to name and structure the options in a way to make clear that they relate to semantic tokens.

It's not just semantic tokens, though, but also hovers, completion, text ranges, and go to definition. Plus, option names must reflect that they affect highlighting markup within descriptions, and don't affect highlighting @tags, types, and so on.

So, this is what I propose then (ideas for better naming are welcome):

• doc.syntax for overall syntax configuration,
• semantic_tokens.enableForDocumentationMarkup to enable/disable semantic tokens,
• not sure if options for other sub-systems are necessary, but we can have hover.enableForDocumentationMarkup and others.

As for defaults, I want to avoid scenario where a user sets doc.syntax and doesn't see anything because semantic_tokens.enableForDocumentationMarkup is disabled. So we can either have doc.syntax = "none" and semantic_tokens.enableForDocumentationMarkup = true, or doc.syntax = "md" and semantic_tokens.enableForDocumentationMarkup = false. IMO, the latter option seems more reasonable.

Please don't invent new off-spec options that only VS Code is set up to handle it.

With settings being more granular this is indeed not necessary. Bear in mind, though, that there's already lots of off-spec things happening in VSCode extension, mostly because (1) VSCode doesn't support overlapping semantic tokens, and (2) VSCode doesn't allow customizing LSP configuration call the same way neovim does. So some changes will have to be made regardless.

You probably also want to document the (off-spec?) tokens you send for documentation.

Actually, no off-spec tokens so far. Maybe I'll add some for italic/bold text, but that will be later).

[clason]

I don't think the "active" LSP features need to be opt-in, as they need to be actively triggered on a documentation location anyway. (It would be different if references also included documentation locations -- which I feel they shouldn't either way.)

I still -- strongly -- recommend the option split, since the syntax format is a project choice while the highlighting is a user choice. This needs to be respected. If this requires making doc semantic tokens opt-out, so be it.

And I understand that the extension needs special support for this; that is fine. I amjust concerned with the server configuration not being driven by that -- everything should be (also) configurable through standard interfaces (config file and ConfigurationChanged notifications).

[taminomara]

Added semanticTokens.renderDocumentationMarkup, default is false.

The reason for this name is for consistency with possible future options.

For example, currently we don't provide documentation modifier for semantic tokens related to documentation comments. In future, we might implement this, but we'll need to make it opt-out because at least in VSCode, most themes render documentation comments in horrendously bright colors. Option name renderDocumentationAsComments will sound consistent with renderDocumentationMarkup.

And I understand that the extension needs special support for this; that is fine. I amjust concerned with the server configuration not being driven by that -- everything should be (also) configurable through standard interfaces (config file and ConfigurationChanged notifications).

I've created #680 to address this issue. When it's merged, VSCode and NeoVim (and other editors) will be able to configure things using the same mechanism.

[CppCXY]

please donot change my local keyword color
now:

before:

[CppCXY]

please resolve conflicts

[CppCXY]

If everything works well, I will merge it before the release tomorrow, but please resolve the conflicts first.

[taminomara]

Rebased.

Since you want to release tomorrow, I've also added support for bold/italic highlighting via emmy annotator, see also EmmyLua/VSCode-EmmyLua#248.