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.

[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]

The existing Connector binding discovery mechanism loads the Test assembly and executes Reqnroll code to build an ObjectContainer and Load and Configure all plugins for the Test project. This has gotten complicated (needs to be out-of-proc b/c the Test Assembly might target a different TFM than the code running the VS Extension). The benefit is that we have access to the binding registry as the Test project will see it at run-time (high fidelity). This might include the possibility that a Plugin modifies the Reqnroll Configuration in some way (such as adding assemblies to the list of External Assemblies, changing language-culture settings, or how template/skeletons are configured).

I believe it is feasible to build a simpler assembly-based discovery system if we forgo loading of plugins for the purposes of IDE binding discovery. We could use a .Net metadata read tool like Mono.Cecil to identify the binding classes and methods without the use of reflection (and without loading the assemblies into a LoadContext).

An additional optimization to consider is to cache the bindings by assembly and, after a project rebuild, compare an assembly hash or other identifier that would change with every build; re-using the bindings for external assemblies that weren't affected by the rebuild.

@gasparnagy how often do projects use Plugins to affect runtime bindings? Would you consider dropping support for IDE binding diagnostics for them?

[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...

[clrudolphi]

Yes, it would be good to upgrade the current VS extension to the latest Gherkin first.

I've completed an update on my experiments. There was a hit fix to the token scanner that is no longer required and a few minor contract changes.

[clrudolphi]

Re: The VisualStudio extension - would you like this release to behave as an upgrade of the existing VS Extension? (Use the same identifying GUID, name, etc) or be an entirely different product?
This would affect the welcome experience and upgrade experience logic.

[gasparnagy]

No, we should have a new GUID, name, etc (for now at least). Probably there will be a period when we need to support both.
We should put the word "preview" in the name of this new one.

[clrudolphi]

Cucumber has an LSP implementation. How closely would we like to match their handling of semantic tagging?

In their handling of semantic tag mapping they place a semantic tag on the mediaType assigned to a docString (if it exists) (such as ```xml)
The DeveroomTagParser does not provide a tag for docString mediaTypes. We would need to enhance the DeveroomTagParser to match what Cucumber has.

Secondly, should we match their pre-existing tag mapping? Their mapping is:

export const semanticTokenTypes: SemanticTokenTypes[] = [
  SemanticTokenTypes.keyword, // Feature, Scenario, Given etc
  SemanticTokenTypes.parameter, // step parameters
  SemanticTokenTypes.string, // DocString content and ``` delimiter, table cells (except example table header rows)
  SemanticTokenTypes.type, // @tags and DocString ```type
  SemanticTokenTypes.variable, // step <placeholder>
  SemanticTokenTypes.property, // examples table header row
  SemanticTokenTypes.comment, // # comments

From the comments in the code, this seems mostly driven by what VSCode easily supports.

[gasparnagy]

If that mapping is easier to handle with LSP, we can use that one as well. Does not matter much as long as it is easy to adapt with our code. (As a generic guideline, we should avoid feature implementations for this initiative to avoid scope creep - we can note the additional feature ideas and implement them separately)

[clrudolphi]

I would like to suggest a few new/changed features that should be possible with Diagnostics and CodeLens:

• Mal-formed regex and cucumber expressions in step binding attributes are highlighted with a red error squiggle
• Mal-formed Scope expressions in Scope attributes are highlighted with a red error squiggle
• Binding expressions (syntactically valid) that don't match the argument count of the binding method are highlighted with a red error squiggle
• Step Binding attributes are given a CodeLens with the count of matching steps. When clicked, the codelens provides a popup menu of links to those steps (and UI navigates to the one clicked)
• Ctrl and hover over a step in a feature will result in the step being displayed with an underline (if the step is matched to a single step binding). Clicking on the step then navigates to the matching binding method.
• Ambiguously matched steps have a codelens indicating Ambiguity. Clicking on the codelens will present a pop-up of the matching step bindings; clicking one of them navigates to it.
• Features, scenarios, steps that have matching hooks will have a codelens with a hook icon (or other indicator). Clicking on the indicator will present a pop-up of matching hooks, clicking one will navigate to it. If only a single matching hook, clicking on the indicator will directly navigate.

[gasparnagy]

As mentioned in my other comment, let's keep these ideas, but not implement them at the first place. Maybe we can create a separate discussion topic for these, so that we don't forget.

[clrudolphi]

I'm noticing in the Reqnroll.VisualStudio projects the use of C# attributes such as [NotNull], [CanBeNull] etc that are defined in the Reqnroll.VisualStudio/Properties/Annotations.cs file and appear to be source code copied from JetBrains.

Do we wish to continue to use them? Is there an update/upgrade that we should consider using?

[gasparnagy]

I think those were workarounds for C# nullable support. We don't need them in the new code.

[clrudolphi]

After some experimentation on my own and reading through @ThomasHeijtink's POC, I've drafted the attached design document. My intent is to confirm the scope, approach, non-functional requirements, and open issues. I apologize in advance for the length. But please review and provide any feedback you have.
LSP-IDE-Support-Design.md

[Code-Grump]

My first feedback would be: can we break this down into several documents, please? 😅 As an input for an LLM to take a stab at vibe-coding a solution, it probably provides some useful structure. For us fleshy humans it's just too much to process and I don't want to ask an LLM to chew it up and miss the important details.

I think I worry with a document of this nature is that it wants to be authoritative and I'm not sure that's realistic. There's just so much detail in here I don't really feel comfortable going "yes, I agree with everything".

With that said, I can see some things that ought to be corrected / clarified / removed:

• Regarding a step definition registry, is this required to be a new implementation of the existing discovery algorithm, or are we re-using the existing components?
• The more I read "IdeSupport" the more vague it feels. Would we consider "IdeIntegration" for a slightly more-specific name?
• The repository structure feels a little off to me. I generally dislike "Common" libraries as a pattern, because the architectural constraints are under-specified and often result in it becoming a dumping-ground for homeless code. Seeing "Common" and "Core" projects is also a little confusing. What seems to be described is more the platform on which server and client components are being built. Can we please consider .Platform and .LanguageServices for these projects respectively? I would also suggest that projects should be named LspServer, rather than LSP.Server and LspConnector rather than LSP.Connector, because we're not implementing an LSP parent namespace and these are the full names of the components being described.
• The test projects outlined don't seem to quite line up with the projects I'm seeing. What's the intention of the Specs project for the server? What's Specs project for "End-to-end BDD specs (Reqnroll)? Should we get a spec for each client IDE we're integrating with?
• Debounce between incremental parsing updates seems like a wise idea. 👍 What made 200ms the default? Would having an incremental parser be useful for this? (i.e. does the didChange event give us a delta to work with?)
• What's the motivation behind using MediatR? I've rarely seen it add more than it obfuscates, but would be interested to know if it really fits into this use-case.
• The Kotlin source components use ReqnrollLspServer and ReqnrollServer, and I can't tell if it's referring to two different things or if this the naming should be consolidated.
• Likewise the server path resolution uses a REQNROLL_GHERKIN_SERVER_PATH, which feels like it should be REQNROLL_LSP_SERVER_PATH.
• There's no immediate explanation why the Binding Connector is implemented as an out-of-process discovery system. It's obviously comparatively expensive to launch and kill a process repeatedly, so a summary of why this choice was made would be great. Has AssemblyLoadContext been considered for .NET Core runtimes or was this ruled out-of- scope for this implementation?

And that's as much as I could find time for.

[clrudolphi]

@Code-Grump Thank you for taking the time on a week-end to read through and provide feedback. I do acknowledge that the document is dense. The results from @ThomasHeijtink's POC and my experiments are promising, but my intuition is raising concerns that we could naively get down a path and find ourselves deep in a quagmire. So, I wanted to spell out as much detail as necessary in order to get a feel for the full scope of the effort and the risks. The intent of sharing it is obtain push-back on those areas where my thinking is not aligned with the teams'.

Regarding a step definition registry, is this required to be a new implementation of the existing discovery algorithm, or are we re-using the existing components?

We can re-use the existing Connectors, full-stop, if we wish. We could also adopt new approaches. I have a POC that runs the Connector as a long-running out-of-proc service and fully supports both .NET Framework and .Net runtimes. We could also use the approach you've experimented with in terms of build-time generation. I have other ideas that might be less expensive than reflection for discovery from built assemblies.

The key point to understand is that I would like to attempt a hybrid approach in which C# source file edits result in immediate binding registry updates without a build/discovery cycle. Does that cause any concern?

The more I read "IdeSupport" the more vague it feels. Would we consider "IdeIntegration" for a slightly more-specific name?

What ever the team agrees on; I'm not tied to any specific naming. Same goes to the suggestions on LSP.Server versus LSPServer, etc.

I generally dislike "Common" libraries as a pattern,

This was created with the thought that there would be functionality required by the LSP Server and all three IDE client integrations. In retrospect, I doubt that such is technically feasible and these classes may only be useful to the LSP Server and the VisualStudio extension. Perhaps these go into a 'shared' project that is source compiled into each.

Tests - What's the intention of the Specs project for the server?

This section of the document is probably the weakest. I am not familiar with how tests of IDE plugins for VSCode and Rider are typically structured, so there is a lot of hand-waving here. By Specs for the server, my intent is that we have gherkin features exercising the server at the LSP protocol level in order to provide API (integration) testing. By Specs at the extension level, my intent is that we have end-to-end testing that proves that the server-provided information and behaviors are properly surfaced within the IDE.

Debounce ... What made 200ms the default? Would having an incremental parser be useful for this? (i.e. does the didChange event give us a delta to work with?)

The performance targets are arbitrary at this point. I'd like the community's input on what those should be.
While the didChange message will provide a range of the delta, I'm not sure it's useful; as the gherkin AST includes source line location data, any change that causes line additions/deletions means that the rest of the file needs to be parsed as well. May be easier to reparse the feature file completely.

What's the motivation behind using MediatR?

The best available option for building the server is the omniSharp/csharp-language-server-protocol library, which itself uses MediatR. When considering options for providing asynchronous processing of one-to-many notifications of internal server events, its an easy choice to make. If the team is opposed to taking a direct dependency on it, we can find something else.

There's no immediate explanation why the Binding Connector is implemented as an out-of-process discovery system.

This draft is specifically vague on the Binding discovery mechanism. As mentioned above we can start with the existing generic Connector from the existing VisualStudio extension (out-of-proc, one-shot-only, not reused). The Connector is out-of-proc b/c we have to load the test assembly and execute the Reqnroll start-up (up to the point of plugins executing their start-up) in order to have a full understanding of the list of external assemblies involved. Since the test assembly might be on a different run-time/version than the IDE extension, we load it out-of-proc.

I'll look at the other naming issues noted.

Keep the great feedback coming!

[clrudolphi]

I have a working prototype, with tests, for Visual Studio, that implements Features F1, F2, F3, and F4 of the design plan (updated) I previously posted.

The prototype:

• Uses Omnisharp/Csharp_Language_Server_Protocol to implement an LSP server for gherkin
  • The LSP server spins off an out-of-proc Connector to perform reflection-based discovery (port of existing Reqnroll.VisualStudio generic connector with the addition of using an AppDomain to host .NET Framework test assemblies). [F2]
  • Editing changes to .cs step binding source files are parsed using Roslyn and immediately update the binding registry and mapping. [F2]
  • The LSP provides the IDE with Semantic Token using custom token types [F1]
  • The LSP provides gherkin document diagnostic notifications for parsing errors and binding mismatches (F3 and F4)
• Provides a VisualStudio hybrid VSSDK+VS.Extensibility extension
  • Wizards for New Project and New Item are ported from Reqnroll.VisualStudio (not yet completed: Welcome and Upgrade wizards)
  • Semantic tokens are mapped to VisualStudio ClassificationSpans such that the coloring behavior of Reqnroll.VisualStudio retained.

I will keep working through the plan and documenting findings by updating the plan. The handling of semantic tokens for VisualStudio was quite challenging as the support for it within its built-in LSP client and VS.Extensibility is just not mature.

Comments of course are welcome.

[clrudolphi]

While attempting to implement Find Step Usages, I went down the rabbit-hole that is linked files.
Unfortunately, this leads me to believe that there will be a risk of user experience confusion due to a limitation in how the LSP protocol works.

Presume you have a feature file: Feat.feature in Project A. Project A also has a set of step bindings that map to that feature and it binds without error.

You create Project B and link in the feature file Feat.feature from Project A. Project B has its own set of bindings.

If those bindings in B differ from those in A it will not be obvious in the editor. When the IDE opens a file it provides the LSP server with the file-path. It does not indicate which project context this editing operation is within. Both Projects A and B will see the file as having a file-path from within's Project A's folder structure.
When the IDE asks the LSP for semantic tokens, it asks for them by file-path-name. So the only choice that the LSP has is to return the binding matches from the project in which the feature file physically sits, not the project that it will be running against.

This differs from our current VS extension as the display of binding coloring is sensitive to the project from which the feature file has been opened.

My apologies if that description is convoluted and hard to follow.
I will continue to experiment. There may be nuances to the LSP protocol that I've not yet fully mastered. There may be custom work-arounds we can make.

[clrudolphi]

Per feedback from @Code-Grump the design doc has been restructured into 4 separate documents:
Overview
Architecture
Feature Designs
Open Questions

[clrudolphi]

I made good progress this week on the experimental VS implementation here

The following features are implemented:

• [F1 · Gherkin Syntax Highlighting]
• [F2 · Binding Discovery]
• [F3 · Gherkin File Diagnostics]
• [F4 · Gherkin Parse Error Display]
• [F5 · Go to Step Definition]
• [F6 · Define Steps (Scaffolding)]
• [F7 · Keyword Completion]
• [F8 · Step Completion]
• [F11 · Document Auto-formatting]
• [F14 · Find Step Definition Usages]
• [F15 · Find Unused Step Definitions]
• [F16 · Step Rename Refactoring]
• [F17 · Hook Navigation]
• [F18 · Code Lens (Step Usage Counts)]
• [F19 · New Project / Item Wizards]

With the following yet to be implemented:

• [F9 · Document Outline]
• [F10 · Code Folding]
• [F12 · Table Auto-formatting] almost finished, a few quirks remaining
• [F13 · Comment / Uncomment]
• [F20 · Installation & Upgrade Experience]

The largest hurdle to overcome has been the mismatch between the .net project system and the LSP server's limited concept of file ownership. To overcome that, I've defined several custom request methods and notifications which are used by the VS extension code to send information to the LSP server about which documents belong to a project (and changes in that status).

I have one undiagnosed issue that occurs at startup in that when opening an existing solution, if that solution had open files in the editor during its last session those will be reopened. The reopening occurs before the LSP gets spun up so those editors don't get LSP support. Closing and reopening them fix that. (for now). Also, the LSP is spun up lazily by VS and so if there are binding classes open for editing they don't get LSP support until a feature file is opened.

I also have a TODO to build out the performance validation tests.
Of course, I welcome feedback from anyone that would like to take it for a spin.