Fix invalid cast issue in omnisharp-roslyn by fixing LspHandlerDescriptor ctor #488
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ptor .ctor The issue (apparently) is that LspHandlerDescriptor is constructed for DelegatingRequest that is registered in LanguageServerHost.cs (in omnisharp-roslyn) via .OnRequest(). The constructor attempts to resolve RPC return type based on what generic type is set on IRequest<T> that inherits DelegatingRequest. However DelegatingRequest implements both IRequest and IRequest<TResponse> and .LINQ query selects IRequest<MediatR.Unit> type as the return value -- while it should be IRequest<JToken>. Fixed by ordering implemented IRequest<> interfaces by generic param type and taking the first that is not MediatR.Unit -- only then use the MediatR.Unit version.
Codecov Report
@@ Coverage Diff @@
## master #488 +/- ##
==========================================
- Coverage 73.08% 73.07% -0.01%
==========================================
Files 256 256
Lines 12246 12246
Branches 843 843
==========================================
- Hits 8950 8949 -1
- Misses 3296 3297 +1
Continue to review full report at Codecov.
|
|
@razzmatazz This change makes sense to me, we could probably just filter I'm not too worried about tests as the other tests have all passed. I might look to see if I can add some more validation tests around the delegated request/responses however for the future. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See:
The issue (apparently) is that
LspHandlerDescriptoris constructed forDelegatingRequestthat is registered inLanguageServerHost.cs(in omnisharp-roslyn) viar.OnRequest()here:https://github.com/OmniSharp/omnisharp-roslyn/blob/b68f146a649bd6b0294894831e08bbd3b802110c/src/OmniSharp.LanguageServerProtocol/LanguageServerHost.cs#L310
The constructor attempts to resolve RPC return type based on what generic type is set on
IRequest<T>that is inherited byDelegatingRequest. HoweverDelegatingRequestimplements bothIRequestandIRequest<TResponse>and LINQ code selectsIRequest<MediatR.Unit>type as the return value -- while it should beIRequest<JToken>for LSP requests (apparently?).The issue was fixed by ordering implemented
IRequest<>interfaces by generic param type and taking the first that is notMediatR.Unit-- only then use theMediatR.Unitversion.I am not 100% sure this is the proper/correct fix for my issue (OmniSharp/omnisharp-roslyn#1995) but it seems to do the job.
I can try to write tests or change this you feel that is the wrong approach to fix the issue.