LSP-based IDE support plan

[gasparnagy]

This discussion board can be used to have a plan to an LSP-based IDE support based on the proof of concept (PoC) of @ThomasHeijtink: https://github.com/ThomasHeijtink/Reqnroll.Plugin

All of the below is just suggestions, feel free to comment or suggest changes.

Overall goals

• The PoC should be considered as a technical feasibility study the features there are implemented with dummy implementations and in some cases the terminology does not match to the official Reqnroll/Cucumber terminology.
• We should take care that the final implementation only contains functional code that has tests. Therefore the approach should be to start with a new empty LSP project and add the features incrementally instead of copying everything from the PoC.
• The primary source of the terminology and functionality should be the VS extension (https://github.com/reqnroll/Reqnroll.VisualStudio), as that is the most up-to-date IDE integration we have so far. We should try to take over code from that as much as possible.
• We should try to use the BDD specification from the existing VS extension (wherever possible): https://github.com/reqnroll/Reqnroll.VisualStudio/tree/main/Tests/Reqnroll.VisualStudio.Specs/Features
• We should concentrate on Reqnroll for now, supporting SpecFlow or other Cucumber implementation should be out of scope for this initiative
• The features should work similarly to how they work in VS extension, it is not the goal of this initiative to improve these features for now. The only exception is the binding discovery, that we can extend to support code-based discovery.

Implementation plan

I suggest making the implementation in phases: once we are done with a phase, we can make a review and move on to the next phase only after we are happy with the approach. With this we can avoid to produce a massive amount of code that gets stuck in review for forever.

The suggested phase arrangement could be:

Phase 1: Basic syntax coloring

We should start with an "empty" LSP server and the shells for VS, VSCode and Rider, that provides syntax coloring for Gherkin files using the Gherkin parser. The default language configuration should be read from reqnroll.json.

Related specs:

• https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/Tests/Reqnroll.VisualStudio.Specs/Features/Editor/SyntaxColoring.feature

Related codes (consider to reuse):

• https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/Reqnroll.VisualStudio/Editor/Services/Parser/DeveroomGherkinParser.cs

With this phase we can verify:

• General approach
• Repository structure, namings
• Reqnroll Project configuration
• Testing strategy

Phase 2: Binding discovery

Import existing binding discovery method from VS ext (processing compiled test assemblies with connectors after build), extend it with a code-based discovery approach and integrate it with core features like: showing undefined steps, define step and go to step definition

Related specs:

• https://github.com/reqnroll/Reqnroll.VisualStudio/tree/main/Tests/Reqnroll.VisualStudio.Specs/Features/Discovery
• https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/Tests/Reqnroll.VisualStudio.Specs/Features/Editor/StepAnalysis.feature
• https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/Tests/Reqnroll.VisualStudio.Specs/Features/Editor/Commands/DefineStepsCommand.feature
• https://github.com/reqnroll/Reqnroll.VisualStudio/blob/main/Tests/Reqnroll.VisualStudio.Specs/Features/Editor/Commands/GoToDefinitionCommand.feature

Related codes (consider to reuse):

• TBD

With this phase we can verify:

• Binding discovery and processing
• Improved code-based discovery process
• While many features are still missing from Phase 2, the result represents a minimum level that contains the most important features

Phase 3: Editor improvements

Extend editor support with additional missing features:

• Keyword completion
• Display gherkin errors
• Collapsible document outlines
• Trackability features (generate links from tags)
• Auto format document command
• Auto format table command
• Comment/Uncomment command

Phase 4: Navigation improvements

Extend with additional missing features:

• Step completion
• Find step definition usages
• Find unused step definitions command
• Go to hooks command
• Rename steps command

[clrudolphi]

This is a well thought out plan. A few thoughts :

To my understanding, there is no support in the LSP for commenting/uncommenting.

Please also document when we should tackle the creation of a hybrid VS extension so that we can include the templates.

Should we also include an aspirational statement about whether extension code not related to the LSP should be ported out of VSSDK to VS.Extensibility?

What appetite is there to include changes/improvements to the binding discovery process (such as code-gen bindings)?

[clrudolphi]

Will there be a project step to evaluate and select the LSP framework/library that will be used?
Would it be feasible to modify Cucumber/language-server to our needs?

[ThomasHeijtink]

Should we also include an aspirational statement about whether extension code not related to the LSP should be ported out of VSSDK to VS.Extensibility?

Not all features are supported in VS.Extensibility. From what I learned is that there is no hard line between these two. But I might be wrong. In order to add TextMate support I had to make changes so that certain language related files were included in the extension.

To get commenting and uncommenting working through a custom LSP command I had to add code which uses the classic editor MEF API. (see my comment below as an example). The extensibility seems to work for very limited/specific use cases. For more rich support, it requires the classic approach, I think.

I think we will have to come to an agreement on a way-of-working where we try to implement features based on pure LSP. But when not possible to handle this through custom LSP commands (so that the behaviour stays centralized but each client requires their own custom bridge).

And agree that no behaviour should be added outside LSP? This isn't as hard line as it seems, as for example TextMate support does not call any LSP endpoints. But this could be regarded as styling, rather than text manipulation and other behaviour.

[clrudolphi]

Understood that not all features are possible in VS.Extensibility.

What I'm asking is whether we take a VS.Extensibility first approach - everything possible through VS.Ex should be ported to it even if it has nothing to do with the LSP implementation. {IMO, I would prefer to have only the templates left in VSSDK and everything else moved over.}

A follow-up would be whether we attempt to move functionality that is not native to LSP (like commenting) to a custom LSP command or simply leave it to the editor specific code?

[gasparnagy]

I think porting things from VSSDK to VS.Extensibility is a nice-to-have. Let's first build a list of things (besides templates) that we need to put to the extension shell and then we can decide is porting to VS.Extensibility would slow us down too much or not.

[gasparnagy]

Would it be feasible to modify Cucumber/language-server to our needs?

This is an interesting question. We can surely progress faster with our own separate C# solution that focuses on the ecosystem and usage of Reqnroll (and we can focus on the compatibility with our existing packages). Also the discoverability of the IDE extension for Reqnroll users is better if the package is called "Reqnroll" and not Cucumber. I have seen this with many of my clients trying to use Reqnroll with VsCode.

But I can imagine that once we have a robust solution we can contribute back to the Cucumber/LSP. Let's see.

[ThomasHeijtink]

Sounds like a good plan.

Does that mean that the community has decided that this approach provides a good enough interaction/support as a reqnroll plugin in order to go ahead with this? Or should we still evaluate per feature how well it scores and whether it passes as good, good enough, or sub par and look for ways to mitigate any sub par user experiences?

A couple of remarks:
Phase 1:

• With regards to keyword highlighting we have three options:
  • We can use the LSP textDocument/Tokenization capability. Advantage: LSP based, Disadvantage: up to the IDE which color to apply
  • We can only rely on client side coloring. Which we will have to implement anyway for the table coloring anyway.
  • We use both in order to allow the LSP server to be used for IDE's we do not yet have a client for. And use the LSP tokenization as fall back
    I would advise to start with the tokenization approach first. And if people want alternative colors, they will have to find out whether their IDE supports this. I don't know how important it is to change the color of these keywords.
• Looking at the current feature files we have to decide how to formulate the asserts. Do we commit to the LSP result (which is abstract) or do we create a thin and simple assertion translator which adds extra code to maintain?

Phase 3:

• Collapsible document outlines is supported by LSP
• As @clrudolphi stated, commenting and uncommenting code is not supported by LSP. It can be implemented with a relatively small client bridge, letting all the work be done by the LSP server, even supporting (un/re)do. Here is an example which triggers a commenting routine using the LSP server command structure. If nothing is selected typing the # will only insert the # character. When selecting a line (fully or partially) and a # is typed, the LSP server will add the # character at the beginning of each line:

For now the uncommented routine just relies on ctrl+z but I can of course also toggle when a # is typed on selected lines which are already commented out.

[gasparnagy]

Basically this whole DeveroomTagParser is practically IDE agnostic and could be reused to the LSP based approach. This one generates "tags" that are a kind of tokens.

I'll look into this further as it might make a better choice than using the bare Gherkin parser.

Just an important note: This is based on the Gherkin parser, but processes the result AST and generates the "tags" based on that.

[clrudolphi]

Basically this whole DeveroomTagParser is practically IDE agnostic

Having now looked at the FeatureTagger and DeveroomTagParser, etc, I see the value. It provides a location/span-aware abstraction over the gherkin AST; and it will map pretty directly to the needs of the LSP for the purposes of generating semantic tokens and other needs.

The DeveroomTagParser, DeveroomTag and related eco-system in Editor/Services is tied to types provided by the VSSDK (such as SnapshotSpan, ITextSnapshot, ITagSpan, etc.)

I would think we will need to port these out and make them truly IDE agnostic by adapting them to the concepts and types offered by the OmniSharp/c-sharp-language-protocol library.

If not, are you comfortable integrating various Microsoft.VisualStudio.xxx assemblies into the LSP server?

[gasparnagy]

I would think we will need to port these out and make them truly IDE agnostic by adapting them to the concepts and types offered by the OmniSharp/c-sharp-language-protocol library.

Yes, this is what I would do.

[clrudolphi]

@gasparnagy BTW, what is the origin and meaning of the Deveroom prefix to these types? Does that meaning/intent still apply now or is it a historical artifact that has lost its meaning? Would a different term be a better fit at this stage of the evolution of the code?

[gasparnagy]

Historical artifact.
There was a time when I was a persona non grata at the SpecFlow project but I decided to rewrite the VS extension. I have chosen "Deveroom" as a name for it. Later the relationship normalized and I donated back Deveroom to be the new SpecFlow VS extension. At that time we supposed to rename all "deveroom" term to "specflow", but we apparently did not do that fully. Later I only renamed "specflow" to "reqnroll", so "deveroom" remained in the codebase.

[clrudolphi]

Any thoughts or preferences on how the server should be packaged/distributed? Would we publish this as a nuget package or kept interna as a git submodule?

[gasparnagy]

I think the telemetry can be in the LSP, so that can be also shared. If we don't get telemetry info about the IDE specific stuff that is not a disaster. (But of course we can add extra telemetry stuff to those if necessary - but I would not for the start)

[clrudolphi]

I think the telemetry can be in the LSP,

I will gently disagree. We know we're going to want the IDE-specific telemetry anyway (installation, update, etc). The LSP protocol defines messages for this purpose (for the LSP server to send log & telemetry data back to the client), so I would prefer that we do it right from the start. We can build a common telemetry library around what the existing VS Ext already has.

[gasparnagy]

I think the telemetry can be in the LSP,

I will gently disagree. We know we're going to want the IDE-specific telemetry anyway (installation, update, etc). The LSP protocol defines messages for this purpose (for the LSP server to send log & telemetry data back to the client), so I would prefer that we do it right from the start. We can build a common telemetry library around what the existing VS Ext already has.

I'm also fine with that. But just to be clear, currently I don't think we have an IDE specific telemetry. It calculates and decides about installation/upgrades on it's own without using any VS infrastructure. Or maybe I just don't fully understand what you want to say.

[clrudolphi]

IDE-specific telemetry

By 'IDE-specific telemetry`, I mean telemetry we will want to send that is not related to the LSP per se. When any of our extension/plugins are installed or upgraded, we would want to send telemetry. When a template is added, we send telemetry. My point being that the LSP server will not be the sole sender of telemetry, and thus my argument that telemetry code should be in a common library used by both the LSP server and the extension(s).

[gasparnagy]

I understand, actually what I wanted to say is that besides the template wizard usage events, everything can be tracked in the LSP. The LSP will be packaged to every extension and it will "know" whether it is currently being used for VS, VSCode or Rider, so it can track the installation/upgrade of the extension. This is also how it works right now, the install/upgrade telemetry is not detected by the VS infra, but the plugin saves some data to the registry and checks during initialization if it is there. So I think it is only the wizard usage that we cannot track in LSP - and that is not that important.
But anyhow it is, the telemetry events are simple HTTP calls (per event) at the end. So nothing stops us from sending telemetry events both from the LSP and from the extension part if we want.

[clrudolphi]

I've noticed that the POC is declaring server capabilities globally (during init) and those would (?) apply for both .feature and .cs files. I fear a risk that our LSP would not play well with other LSPs and/or native C# support provided by the IDEs. Let's take care to include some form of experience testing to confirm proper cooperative behaviors.

[ThomasHeijtink]

Yes, we definitely have to take this into account during our testing. We do relatively very little interaction with the actual C# itself. We would predominately use diagnostics, code actions, code lens and references/usages. The first two generally stack very well in combination with other LSP's. Code lens and references not so much. The both work well in vs code. Rider shows the code lens but cannot, just like visual studio, use/combine the references from its own and LSP servers. This would be a feature request for visual studio.

[clrudolphi]

An approach to consider is to only register as static options for those capabilities that we need for both document types. Then register dynamically for capabilities needed on a document-type basis. For instance, Completion is needed only against the feature document type, not c#.

[gasparnagy]

Do you think if this is something we need to check before start or is this just an optimization / nice to have?

[clrudolphi]

Do you think if this is something we need to check before start or is this just an optimization / nice to have?

I really do think we need this sorted before we start. The POC registers interest in both '.feature' and '.cs' files. The latter primarily because we want to provide diagnostics about bindings (reference counts, reference links, not-used warnings, etc). But by globally registering for capabilities we are also advertising to the IDE that we do things like formatting of c# files. That will, at least, result in needless overhead. It might also cause IDE misbehavior.

[gasparnagy]

Do you think if this is something we need to check before start or is this just an optimization / nice to have?

I really do think we need this sorted before we start. The POC registers interest in both '.feature' and '.cs' files. The latter primarily because we want to provide diagnostics about bindings (reference counts, reference links, not-used warnings, etc). But by globally registering for capabilities we are also advertising to the IDE that we do things like formatting of c# files. That will, at least, result in needless overhead. It might also cause IDE misbehavior.

I see. This should be high prio then.

[clrudolphi]

I will grant that the following is not a high priority item for the first implementation, but I am intrigued by the idea that we precompute the mapping between pickle steps and bindings during the compilation phase. The result would be an artifact that could be used by the LSP to provide Definition and References (especially to external assemblies). But such an artifact could also be used by other tooling such as reports on binding usage (or non-usage).

[gasparnagy]

Yeah, let's keep this in mind.

[clrudolphi]

Another idea to consider is to refactor the code within Reqnroll so that the Reqnroll.BindingSkeletons namespace (such as the StepDefinitionSkeletonProvider) can be used within the plugins.
This would allow us to remove the duplicative code we have today in the plugins.
We might do this as a 'common' VS project or as a git submodule or as a nuget package.

[gasparnagy]

Let's also keep this in mind, but not a high prio.

[clrudolphi]

We will need to give some thought on telemetry.
The VS Extension has concepts (interfaces and implementations) for both loggers and monitoringServices (which get forwarded to the Analytics transmitter).

Do we need to plan a step to normalize the set of monitored events (and data structures) from all three clients that will consume the LSP server?

The LSP standard includes a telemetry/event notification message (from server to client) but provides no standard for the data structure sent in that message. We will need to define a schema for the structure of telemetry notifications we'd like to send.

[clrudolphi]

As a follow-on for this topic - The existing VS Extension leverages the Microsoft.VisualStudio.ApplicationInsights library, which ties it directly to VisualStudio and the VSSDK. The Reqnroll runtime however, uses HttpClient to post data to a visualstudio.com URL endpoint (making it cross-platform).
@gasparnagy - are these two implementations hitting the same data collection endpoint?

If so, I would propose that we extract the Analytics stuff into a netstandard2.0 library project that all three IDE integrations could use. In order to do that we would need to refactor the code to use the same HttpClient technique used by the runtime.

Does that sound feasible/desirable?

[gasparnagy]

Yes, all of these go to the same endpoint. The Microsoft.VisualStudio.ApplicationInsights has a bunch of nice features of buffering and batching the telemetry messages, but we did not wanted to use that in Reqnroll, to avoid an additional dependency that might conflict with a different version of the assembly used by the test projects or the SUT.

So in general it would be better to use the Microsoft.VisualStudio.ApplicationInsights based approach, but that cannot be reused in Reqnroll runtime.

[clrudolphi]

So in general it would be better to use the Microsoft.VisualStudio.ApplicationInsights based approach, but that cannot be reused in Reqnroll runtime.

If we limit telemetry to the LSP server then I see your point. But when we want telemetry from the IDE plugin itself, are we comfortable bringing along those VisualStudio assemblies into the Rider and VS Code plugins?

[gasparnagy]

Sorry for the confusion. I have mixed up the things. I thought we use the Microsoft.ApplicationInsights.WorkerService package that is a generic SDK package for app insights (https://github.com/Microsoft/ApplicationInsights-dotnet). I use this one in a work project and this is the one that has the buffering, etc I have mentioned. And that is a standard NuGet package.

[clrudolphi]

Here is a proposed structure for the projects that will make up the Visual Studio extension.
I suggest we expand this to include diagram elements that depict the pieces related to VSCode and Rider and depict which portions will be shared across all three extensions.

graph TD
    CT["Analytics<br/>(harvested from existing VS Extension)<br/>netstandard2.0"]

    subgraph Common ["Common"]
        Config[Reqnroll Configuration<br/>netstandard2.0]
    end

    subgraph LSPHost ["Reqnroll Language Server"]
       LSP[LspServerProject<br/>net8<br/>LSP server logic]
       Connector["Binding Engine<br/>Reqnroll.dll<br/>run-time varies"]
       LSP -.->|"Out-of-proc"| Connector
    end
 
    subgraph VisualStudioExtension["Solution: Reqnroll.Plugin.VisualStudio.sln"]
        EP["ExtensionProject<br/>VSIX manifest<br/>VSSDK+Extensibility<br/>(net481)"]    
        subgraph Classic ["Classic VSSDK"]
            PTP["ProjectTemplate Project<br/>net481<br/>.vstemplate"]
            ITP["ItemTemplate Project<br/>net481<br/>.vstemplate"]
            WP[WizardsProject<br/>net481<br/>IWizard impl]
        end
        
        subgraph Modern ["VisualStudio.Extensibility"]
            Editing["EditingProject<br/>(net8-windows)<br/>LanguageServerProvider<br/>CodeLensProvider"]
        end
 
        EP -.->|"Assets"| PTP
        EP -.->|"Assets"| ITP
        EP -.->|"Assets"| WP
        EP -.->|"Out-of-proc"| Editing
    end
    

    EP -.->|consumes shared lib| CT
    WP -.->|consumes shared lib| CT
    Editing -.->|consumes shared lib| CT
    Editing -.->|"Out-of-proc"| LSP

    VisualStudioExtension-.->|"consumes shared lib"|Common

    classDef shared fill:#e1f5fe
    classDef classic fill:#fff3e0
    classDef modern fill:#f3e5f5
    classDef vsix fill:#e8f5e8
    classDef hostedreqnroll fill:#e8eaf6
    
    class Common,CT,Config shared
    class EP,PTP,ITP,WP classic
    class Editing,LSP modern
    class VSIX vsix
    class Connector hostedreqnroll

Loading

[clrudolphi]

In terms of the templates - @gasparnagy can we drop support for the VB templates?

[gasparnagy]

I am fine with the structure, but would vote to a monorepo style. I think the majority of the efforts (impl and maintenance) will be related to the LSP, so cross-testing it with the different IDEs would get super-hard if we need to share that across repos.

In terms of naming, I was thinking about using "Reqnroll.IdeSupport" as a root name and derive things from that, like:

• Reqnroll.IdeSupport.LspServer
• Reqnroll.IdeSupport.VisualStudio.Package
• Reqnroll.IdeSupport.VisualStudio.ItemTemplates
• Reqnroll.IdeSupport.Rider.xxx

What do you think?

[clrudolphi]

Regarding gherkin document formatting, the existing VisualStudio extension supports two ways of configuring a few settings - in the reqnroll.config file and in the .editorconfig file. The extension merges these settings together (the former provides a global set, the .editorconfig provide overrides at a folder level).

Do we want to support these in the new extensions? (We might consider dropping the support for reqnroll.config settings in favor of the .editorconfig approach)
Do we centralize these settings such that all IDEs support the same settings?

What approach should be taken to feed these settings to the formatting service that will run inside the LSP? Is reading these settings a responsibility of the LSP or of the IDE Extension?

The LSP message workspace/configuration (server-to-client) seems like it might be a good fit for this purpose.

[gasparnagy]

I think for formatting we can stick to the .editorconfig and we don't have to support the settings from reqnroll config.

Ideally the settings should be processed by the LSP, but whatever is the easiest...

[clrudolphi]

Ideally the settings should be processed by the LSP, but whatever is the easiest...

Having the LSP read the .editorconfig directly is easiest. I suspect that having the LSP ask the client for the workspace/configuration is probably more idiomatic, but probably not worth the effort in our situation. As long as the various IDEs don't keep settings like this in IDE-specific formats/storage locations, direct reading of the config files within the LSP is easier.

[clrudolphi]

Scope questions for you @gasparnagy:

1. VB Template support
2. SpecFlow support (esp reading/parsing of older config file formats)
3. NewProjectTemplate - can we remove all code related to FluentAssertion library

[gasparnagy]

Scope questions for you @gasparnagy:

1. VB Template support

Not needed

2. SpecFlow support (esp reading/parsing of older config file formats)

Not needed

3. NewProjectTemplate - can we remove all code related to FluentAssertion library

Yes, we can remove.

[clrudolphi]

User experience question related to binding discovery -
Our existing VS Extension relies on the Reqnroll assembly to perform reflection against project binding classes and external assemblies. Binding diagnostics (such as colorization of feature files when a step is bound) is deferred until the solution is built and changes in binding code aren't recognized until after the next subsequent build.

With the LSP we have the opportunity to use the Roslyn parser to discover bindings in project c# source without requiring a compilation cycle. But we also will continue to rely on Reqnroll to perform reflection-based discovery of external assemblies not in the current project.

Do we want to attempt this?
The complication I foresee is that the Reqnroll discovery mechanism will provide all compiled bindings, regardless of source (internal or externa assembly). We would then need to be able to merge these with the bindings discovered by our roslyn-based analysis of c# source. Might be tricky.

[gasparnagy]

Let's say that this is the best idea I had so far... But it is far from ideal, indeed. Do you have an alternative suggestion in mind?

On the merging: The discovered bindings via the connector-based discovery either have a source reference or not (this can happen if the binding is from a lib that does not have debug info). My idea was that we do the roslyn-based analysis always for a full file and we remove all existing bindings with the same file reference. After a successful connector-based discovery we always reset the binding registry and we only modify it again if a particular C# file is changed.

Special handling is needed for file/folder renames and file deletions, but in the worse case after a build, everything is reset again anyway.

[clrudolphi]

Let's say that this is the best idea I had so far.

Sounds workable.

[clrudolphi]

Interestingly enough - the existing VS extension already has the code necessary to make this work. When a step binding snippet is generated, it invokes Roslyn to get the step bindings from that class file and merges them into the bindings registry. I hadn't noticed that before. Pretty cool and provides a roadmap of how to proceed.

[gasparnagy]

Yes, but don't trust that code fully. It was a partly unfinished initiative. It can be a good start though, although the code was a bit too complicated for my taste. Feel free to decide. But @Code-Grump has a similar code based on Roslyn anyway in the analyzer...

[Code-Grump]

Yes, we have a generator that's discovering bindings and producing registry code that can be executed by the runtime to discover and invoke step definitions without needing to perform reflection.

What we keep running into is the need for some kind of "language service" that can answer questions about what definitions are available at edit-time and at runtime, and they both need to agree.

[clrudolphi]

I'm experimenting with extracting the Feature file parsing and tagging out of the existing VS Extension.
I note that the version of the Gherkin NuGet package used in the extension is outdated (using v29 when the current version is v39). There have been breaking changes in the interim.
Would you rather that we tackle that before we move forward or can that wait?

[gasparnagy]

Yes, it would be good to upgrade the current VS extension to the latest Gherkin first. We will need to maintain this code for a while...