Merge pull request #41 from OmniSharp/fix/didchange-callback

Fix/didchange callback
OmniSharp · Oct 26, 2017 · d1ad86d · d1ad86d
2 parents b2bb198 + f128b27
commit d1ad86d
Show file tree

Hide file tree

Showing 14 changed files with 109 additions and 77 deletions.
diff --git a/src/JsonRpc/IRequestRouter.cs b/src/JsonRpc/IRequestRouter.cs
@@ -1,12 +1,12 @@
-using System;
+using System;
 using System.Threading.Tasks;
 using OmniSharp.Extensions.JsonRpc.Server;
 
 namespace OmniSharp.Extensions.JsonRpc
 {
     public interface IRequestRouter
     {
-        void RouteNotification(Notification notification);
+        Task RouteNotification(Notification notification);
         Task<ErrorResponse> RouteRequest(Request request);
     }
 }
diff --git a/src/JsonRpc/InputHandler.cs b/src/JsonRpc/InputHandler.cs
@@ -190,10 +190,10 @@ private void HandleRequest(string request)
                     _scheduler.Add(
                         type,
                         item.Notification.Method,
-                        () => {
+                        async () => {
                             try
                             {
-                                _requestRouter.RouteNotification(item.Notification);
+                                await _requestRouter.RouteNotification(item.Notification);
                             }
                             catch (Exception e)
                             {
@@ -202,7 +202,6 @@ private void HandleRequest(string request)
                                 // If an exception happens... the whole system could be in a bad state, hence this throwing currently.
                                 throw;
                             }
-                            return Task.CompletedTask;
                         }
                     );
                 }

diff --git a/src/JsonRpc/ProcessScheduler.cs b/src/JsonRpc/ProcessScheduler.cs
@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Threading;
@@ -128,7 +128,7 @@ public void Dispose()
         }
     }
 
-    static class Events
+    public static class Events
     {
         public static EventId UnhandledException = new EventId(1337_100);
         public static EventId UnhandledRequest = new EventId(1337_101);

diff --git a/src/JsonRpc/RequestRouter.cs b/src/JsonRpc/RequestRouter.cs
@@ -22,7 +22,7 @@ public IDisposable Add(IJsonRpcHandler handler)
             return _collection.Add(handler);
         }
 
-        public async void RouteNotification(Notification notification)
+        public async Task RouteNotification(Notification notification)
         {
             var handler = _collection.FirstOrDefault(x => x.Method == notification.Method);
 

diff --git a/src/Lsp/ClientCapabilityProvider.cs b/src/Lsp/ClientCapabilityProvider.cs
@@ -16,7 +16,7 @@ public ClientCapabilityProvider(IHandlerCollection collection)
             _collection = collection;
         }
 
-        public bool HasHandler<T>(Supports<T> capability)
+        public bool HasStaticHandler<T>(Supports<T> capability)
             where T : DynamicCapability, ConnectedCapability<IJsonRpcHandler>
         {
             if (!capability.IsSupported) return false;
@@ -26,13 +26,16 @@ public bool HasHandler<T>(Supports<T> capability)
             var handlerType = typeof(T).GetTypeInfo().ImplementedInterfaces
                 .Single(x => x.GetTypeInfo().IsGenericType && x.GetTypeInfo().GetGenericTypeDefinition() == typeof(ConnectedCapability<>))
                 .GetTypeInfo().GetGenericArguments()[0].GetTypeInfo();
-            return !capability.Value.DynamicRegistration && _collection.Any(z => z.HandlerType.GetTypeInfo().IsAssignableFrom(handlerType));
+            return !capability.Value.DynamicRegistration &&
+                _collection.Any(z =>
+                    z.HandlerType.GetTypeInfo().IsAssignableFrom(handlerType) ||
+                    z.Handler.GetType().GetTypeInfo().IsAssignableFrom(handlerType));
         }
 
-        public IOptionsGetter GetOptions<T>(Supports<T> capability)
+        public IOptionsGetter GetStaticOptions<T>(Supports<T> capability)
             where T : DynamicCapability, ConnectedCapability<IJsonRpcHandler>
         {
-            return !HasHandler(capability) ? Null : new OptionsGetter(_collection);
+            return !HasStaticHandler(capability) ? Null : new OptionsGetter(_collection);
         }
 
         private static readonly IOptionsGetter Null = new NullOptionsGetter();

diff --git a/src/Lsp/LanguageServer.cs b/src/Lsp/LanguageServer.cs
@@ -130,22 +130,22 @@ async Task<InitializeResult> IRequestHandler<InitializeParams, InitializeResult>
             var ccp = new ClientCapabilityProvider(_collection);
 
             var serverCapabilities = new ServerCapabilities() {
-                CodeActionProvider = ccp.HasHandler(textDocumentCapabilities.CodeAction),
-                CodeLensProvider = ccp.GetOptions(textDocumentCapabilities.CodeLens).Get<ICodeLensOptions, CodeLensOptions>(CodeLensOptions.Of),
-                CompletionProvider = ccp.GetOptions(textDocumentCapabilities.Completion).Get<ICompletionOptions, CompletionOptions>(CompletionOptions.Of),
-                DefinitionProvider = ccp.HasHandler(textDocumentCapabilities.Definition),
-                DocumentFormattingProvider = ccp.HasHandler(textDocumentCapabilities.Formatting),
-                DocumentHighlightProvider = ccp.HasHandler(textDocumentCapabilities.DocumentHighlight),
-                DocumentLinkProvider = ccp.GetOptions(textDocumentCapabilities.DocumentLink).Get<IDocumentLinkOptions, DocumentLinkOptions>(DocumentLinkOptions.Of),
-                DocumentOnTypeFormattingProvider = ccp.GetOptions(textDocumentCapabilities.OnTypeFormatting).Get<IDocumentOnTypeFormattingOptions, DocumentOnTypeFormattingOptions>(DocumentOnTypeFormattingOptions.Of),
-                DocumentRangeFormattingProvider = ccp.HasHandler(textDocumentCapabilities.RangeFormatting),
-                DocumentSymbolProvider = ccp.HasHandler(textDocumentCapabilities.DocumentSymbol),
-                ExecuteCommandProvider = ccp.GetOptions(workspaceCapabilities.ExecuteCommand).Get<IExecuteCommandOptions, ExecuteCommandOptions>(ExecuteCommandOptions.Of),
-                HoverProvider = ccp.HasHandler(textDocumentCapabilities.Hover),
-                ReferencesProvider = ccp.HasHandler(textDocumentCapabilities.References),
-                RenameProvider = ccp.HasHandler(textDocumentCapabilities.Rename),
-                SignatureHelpProvider = ccp.GetOptions(textDocumentCapabilities.SignatureHelp).Get<ISignatureHelpOptions, SignatureHelpOptions>(SignatureHelpOptions.Of),
-                WorkspaceSymbolProvider = ccp.HasHandler(workspaceCapabilities.Symbol)
+                CodeActionProvider = ccp.HasStaticHandler(textDocumentCapabilities.CodeAction),
+                CodeLensProvider = ccp.GetStaticOptions(textDocumentCapabilities.CodeLens).Get<ICodeLensOptions, CodeLensOptions>(CodeLensOptions.Of),
+                CompletionProvider = ccp.GetStaticOptions(textDocumentCapabilities.Completion).Get<ICompletionOptions, CompletionOptions>(CompletionOptions.Of),
+                DefinitionProvider = ccp.HasStaticHandler(textDocumentCapabilities.Definition),
+                DocumentFormattingProvider = ccp.HasStaticHandler(textDocumentCapabilities.Formatting),
+                DocumentHighlightProvider = ccp.HasStaticHandler(textDocumentCapabilities.DocumentHighlight),
+                DocumentLinkProvider = ccp.GetStaticOptions(textDocumentCapabilities.DocumentLink).Get<IDocumentLinkOptions, DocumentLinkOptions>(DocumentLinkOptions.Of),
+                DocumentOnTypeFormattingProvider = ccp.GetStaticOptions(textDocumentCapabilities.OnTypeFormatting).Get<IDocumentOnTypeFormattingOptions, DocumentOnTypeFormattingOptions>(DocumentOnTypeFormattingOptions.Of),
+                DocumentRangeFormattingProvider = ccp.HasStaticHandler(textDocumentCapabilities.RangeFormatting),
+                DocumentSymbolProvider = ccp.HasStaticHandler(textDocumentCapabilities.DocumentSymbol),
+                ExecuteCommandProvider = ccp.GetStaticOptions(workspaceCapabilities.ExecuteCommand).Get<IExecuteCommandOptions, ExecuteCommandOptions>(ExecuteCommandOptions.Of),
+                HoverProvider = ccp.HasStaticHandler(textDocumentCapabilities.Hover),
+                ReferencesProvider = ccp.HasStaticHandler(textDocumentCapabilities.References),
+                RenameProvider = ccp.HasStaticHandler(textDocumentCapabilities.Rename),
+                SignatureHelpProvider = ccp.GetStaticOptions(textDocumentCapabilities.SignatureHelp).Get<ISignatureHelpOptions, SignatureHelpOptions>(SignatureHelpOptions.Of),
+                WorkspaceSymbolProvider = ccp.HasStaticHandler(workspaceCapabilities.Symbol)
             };
 
             var textSyncHandlers = _collection
@@ -168,16 +168,21 @@ async Task<InitializeResult> IRequestHandler<InitializeParams, InitializeResult>
             }
             else
             {
-                if (ccp.HasHandler(textDocumentCapabilities.Synchronization))
+                if (ccp.HasStaticHandler(textDocumentCapabilities.Synchronization))
                 {
                     // TODO: Merge options
-                    serverCapabilities.TextDocumentSync = textSyncHandlers.FirstOrDefault()?.Options ?? new TextDocumentSyncOptions() {
-                        Change = TextDocumentSyncKind.None,
-                        OpenClose = false,
-                        Save = new SaveOptions() { IncludeText = false },
-                        WillSave = false,
-                        WillSaveWaitUntil = false
-                    };
+                    serverCapabilities.TextDocumentSync =
+                        textSyncHandlers.FirstOrDefault()?.Options ?? new TextDocumentSyncOptions() {
+                            Change = TextDocumentSyncKind.None,
+                            OpenClose = false,
+                            Save = new SaveOptions() { IncludeText = false },
+                            WillSave = false,
+                            WillSaveWaitUntil = false
+                        };
+                }
+                else
+                {
+                    serverCapabilities.TextDocumentSync = TextDocumentSyncKind.None;
                 }
             }
 

diff --git a/src/Lsp/LspRequestRouter.cs b/src/Lsp/LspRequestRouter.cs
@@ -57,35 +57,48 @@ private ILspHandlerDescriptor FindDescriptor(string method, JToken @params)
             if (typeof(ITextDocumentIdentifierParams).GetTypeInfo().IsAssignableFrom(descriptor.Params))
             {
                 var textDocumentIdentifierParams = @params.ToObject(descriptor.Params) as ITextDocumentIdentifierParams;
-                var textDocumentSyncHandlers = _collection
-                    .Select(x => x.Handler is ITextDocumentSyncHandler r ? r : null)
-                    .Where(x => x != null)
-                    .Distinct();
-                var attributes = textDocumentSyncHandlers
-                    .Select(x => x.GetTextDocumentAttributes(textDocumentIdentifierParams.TextDocument.Uri))
-                    .Where(x => x != null)
-                    .Distinct()
-                    .ToList();
+                var attributes = GetTextDocumentAttributes(textDocumentIdentifierParams.TextDocument.Uri);
 
                 _logger.LogTrace("Found attributes {Count}, {Attributes}", attributes.Count, attributes.Select(x => $"{x.LanguageId}:{x.Scheme}:{x.Uri}"));
 
                 return GetHandler(method, attributes);
             }
-            else if (typeof(DidOpenTextDocumentParams).GetTypeInfo().IsAssignableFrom(descriptor.Params))
+            else if (@params?.ToObject(descriptor.Params) is DidOpenTextDocumentParams openTextDocumentParams)
             {
-                var openTextDocumentParams = @params.ToObject(descriptor.Params) as DidOpenTextDocumentParams;
                 var attributes = new TextDocumentAttributes(openTextDocumentParams.TextDocument.Uri, openTextDocumentParams.TextDocument.LanguageId);
 
                 _logger.LogTrace("Created attribute {Attribute}", $"{attributes.LanguageId}:{attributes.Scheme}:{attributes.Uri}");
 
                 return GetHandler(method, attributes);
             }
+            else if (@params?.ToObject(descriptor.Params) is DidChangeTextDocumentParams didChangeDocumentParams)
+            {
+                // TODO: Do something with document version here?
+                var attributes = GetTextDocumentAttributes(didChangeDocumentParams.TextDocument.Uri);
+
+                _logger.LogTrace("Found attributes {Count}, {Attributes}", attributes.Count, attributes.Select(x => $"{x.LanguageId}:{x.Scheme}:{x.Uri}"));
+
+                return GetHandler(method, attributes);
+            }
 
             // TODO: How to split these
             // Do they fork and join?
             return descriptor;
         }
 
+        private List<TextDocumentAttributes> GetTextDocumentAttributes(Uri uri)
+        {
+            var textDocumentSyncHandlers = _collection
+                .Select(x => x.Handler is ITextDocumentSyncHandler r ? r : null)
+                .Where(x => x != null)
+                .Distinct();
+            return textDocumentSyncHandlers
+                .Select(x => x.GetTextDocumentAttributes(uri))
+                .Where(x => x != null)
+                .Distinct()
+                .ToList();
+        }
+
         private ILspHandlerDescriptor GetHandler(string method, IEnumerable<TextDocumentAttributes> attributes)
         {
             return attributes
@@ -105,29 +118,37 @@ private ILspHandlerDescriptor GetHandler(string method, TextDocumentAttributes a
                 _logger.LogTrace("Document Selector {DocumentSelector}", registrationOptions.DocumentSelector.ToString());
                 if (registrationOptions.DocumentSelector == null || registrationOptions.DocumentSelector.IsMatch(attributes))
                 {
+                    _logger.LogTrace("Handler Selected: {Handler} via {DocumentSelector} (targeting {HandlerInterface})", handler.Handler.GetType().FullName, registrationOptions.DocumentSelector.ToString(), handler.HandlerType.GetType().FullName);
                     return handler;
                 }
             }
             return null;
         }
 
-        public async void RouteNotification(Notification notification)
+        public async Task RouteNotification(Notification notification)
         {
             var handler = FindDescriptor(notification.Method, notification.Params);
             if (handler is null) { return; }
 
-            Task result;
-            if (handler.Params is null)
+            try
             {
-                result = ReflectionRequestHandlers.HandleNotification(handler);
+                Task result;
+                if (handler.Params is null)
+                {
+                    result = ReflectionRequestHandlers.HandleNotification(handler);
+                }
+                else
+                {
+                    var @params = notification.Params.ToObject(handler.Params);
+                    result = ReflectionRequestHandlers.HandleNotification(handler, @params);
+                }
+
+                await result;
             }
-            else
+            catch (Exception e)
             {
-                var @params = notification.Params.ToObject(handler.Params);
-                result = ReflectionRequestHandlers.HandleNotification(handler, @params);
+                _logger.LogCritical(Events.UnhandledRequest, e, "Failed to handle request {Method}", notification.Method);
             }
-
-            await result;
         }
 
         public async Task<ErrorResponse> RouteRequest(Request request)
@@ -184,6 +205,11 @@ public async Task<ErrorResponse> RouteRequest(Request request)
             {
                 return new RequestCancelled();
             }
+            catch (Exception e)
+            {
+                _logger.LogCritical(Events.UnhandledRequest, e, "Failed to handle notification {Method}", request.Method);
+                return new InternalError(id);
+            }
             finally
             {
                 _requests.TryRemove(id, out var _);

diff --git a/src/Lsp/Models/DidChangeTextDocumentParams.cs b/src/Lsp/Models/DidChangeTextDocumentParams.cs
@@ -1,4 +1,4 @@
-using Newtonsoft.Json;
+using Newtonsoft.Json;
 using Newtonsoft.Json.Serialization;
 
 namespace OmniSharp.Extensions.LanguageServer.Models
@@ -18,4 +18,4 @@ public class DidChangeTextDocumentParams
         /// </summary>
         public Container<TextDocumentContentChangeEvent> ContentChanges { get; set; }
     }
-}
+}
diff --git a/test/JsonRpc.Tests/InputHandlerTests.cs b/test/JsonRpc.Tests/InputHandlerTests.cs
@@ -206,7 +206,7 @@ public void ShouldHandleError()
         }
 
         [Fact]
-        public void ShouldHandleNotification()
+        public async Task ShouldHandleNotification()
         {
             var inputStream = new MemoryStream(Encoding.ASCII.GetBytes("Content-Length: 2\r\n\r\n{}"));
             var outputHandler = Substitute.For<IOutputHandler>();
@@ -234,7 +234,7 @@ public void ShouldHandleNotification()
                         });
                 }))
             {
-                incomingRequestRouter.Received().RouteNotification(notification);
+               await incomingRequestRouter.Received().RouteNotification(notification);
             }
         }
 

diff --git a/test/JsonRpc.Tests/MediatorTestsNotificationHandler.cs b/test/JsonRpc.Tests/MediatorTestsNotificationHandler.cs
@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Reflection;
 using System.Threading.Tasks;
 using NSubstitute;
@@ -23,10 +23,10 @@ public async Task ExecutesHandler()
 
             var notification = new Notification("exit", null);
 
-            mediator.RouteNotification(notification);
+            await mediator.RouteNotification(notification);
 
             await exitHandler.Received(1).Handle();
         }
 
     }
-}
+}
diff --git a/test/JsonRpc.Tests/MediatorTestsNotificationHandlerOfT.cs b/test/JsonRpc.Tests/MediatorTestsNotificationHandlerOfT.cs
@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Reflection;
 using System.Threading.Tasks;
 using Newtonsoft.Json;
@@ -33,10 +33,10 @@ public async Task ExecutesHandler()
             var @params = new CancelParams() { Id = Guid.NewGuid() };
             var notification = new Notification("$/cancelRequest", JObject.Parse(JsonConvert.SerializeObject(@params)));
 
-            mediator.RouteNotification(notification);
+            await mediator.RouteNotification(notification);
 
             await cancelRequestHandler.Received(1).Handle(Arg.Any<CancelParams>());
         }
 
     }
-}
+}
diff --git a/test/Lsp.Tests/ClientCapabilityProviderTests.cs b/test/Lsp.Tests/ClientCapabilityProviderTests.cs
@@ -111,7 +111,7 @@ private static bool HasHandler(ClientCapabilityProvider provider, Type type)
         private static bool GenericHasHandler<T>(ClientCapabilityProvider provider, Supports<T> supports)
             where T : DynamicCapability, ConnectedCapability<IJsonRpcHandler>
         {
-            return provider.HasHandler(supports);
+            return provider.HasStaticHandler(supports);
         }
 
         private static IEnumerable<object[]> GetItems<T>(IEnumerable<T> types, Func<T, IEnumerable<object>> func)

diff --git a/test/Lsp.Tests/LspRequestRouterTests.cs b/test/Lsp.Tests/LspRequestRouterTests.cs
@@ -30,7 +30,7 @@ public LspRequestRouterTests(ITestOutputHelper testOutputHelper)
         }
 
         [Fact]
-        public void ShouldRouteToCorrect_Notification()
+        public async Task ShouldRouteToCorrect_Notification()
         {
             var textDocumentSyncHandler = TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cs"));
             textDocumentSyncHandler.Handle(Arg.Any<DidSaveTextDocumentParams>()).Returns(Task.CompletedTask);
@@ -44,13 +44,13 @@ public void ShouldRouteToCorrect_Notification()
 
             var request = new Notification("textDocument/didSave", JObject.Parse(JsonConvert.SerializeObject(@params)));
 
-            mediator.RouteNotification(request);
+            await mediator.RouteNotification(request);
 
-            textDocumentSyncHandler.Received(1).Handle(Arg.Any<DidSaveTextDocumentParams>());
+            await textDocumentSyncHandler.Received(1).Handle(Arg.Any<DidSaveTextDocumentParams>());
         }
 
         [Fact]
-        public void ShouldRouteToCorrect_Notification_WithManyHandlers()
+        public async Task ShouldRouteToCorrect_Notification_WithManyHandlers()
         {
             var textDocumentSyncHandler = TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cs"));
             var textDocumentSyncHandler2 = TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cake"));
@@ -66,10 +66,10 @@ public void ShouldRouteToCorrect_Notification_WithManyHandlers()
 
             var request = new Notification("textDocument/didSave", JObject.Parse(JsonConvert.SerializeObject(@params)));
 
-            mediator.RouteNotification(request);
+            await mediator.RouteNotification(request);
 
-            textDocumentSyncHandler.Received(0).Handle(Arg.Any<DidSaveTextDocumentParams>());
-            textDocumentSyncHandler2.Received(1).Handle(Arg.Any<DidSaveTextDocumentParams>());
+            await textDocumentSyncHandler.Received(0).Handle(Arg.Any<DidSaveTextDocumentParams>());
+            await textDocumentSyncHandler2.Received(1).Handle(Arg.Any<DidSaveTextDocumentParams>());
         }
 
         [Fact]