From 6408aaab77262c392bd47e5fae1e99fced2e619f Mon Sep 17 00:00:00 2001 From: David Driscoll Date: Fri, 12 Jul 2019 21:54:53 -0400 Subject: [PATCH 1/3] Updated support for capabilities to be more generous to clients that do not report their support via capabilities --- src/Server/ClientCapabilityProvider.cs | 7 ++-- .../ClientCapabilityProviderTests.cs | 41 +++++++++++++++---- test/Lsp.Tests/LanguageServerTests.cs | 1 + 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/Server/ClientCapabilityProvider.cs b/src/Server/ClientCapabilityProvider.cs index 010d418e7..e00f836d9 100644 --- a/src/Server/ClientCapabilityProvider.cs +++ b/src/Server/ClientCapabilityProvider.cs @@ -21,9 +21,10 @@ public ClientCapabilityProvider(IHandlerCollection collection) public bool HasStaticHandler(Supports capability) where T : DynamicCapability, ConnectedCapability { - if (!capability.IsSupported) return false; - if (capability.Value == null) return false; - if (capability.Value.DynamicRegistration == true) return false; + // Dynamic registration will cause us to double register things if we report our capabilities staticly. + // However if the client does not tell us it's capabilities we should just assume that they do not support + // dynamic registraiton + if (capability.IsSupported && capability.Value != null && capability.Value.DynamicRegistration == true) return false; var handlerTypes = typeof(T).GetTypeInfo().ImplementedInterfaces .Where(x => x.GetTypeInfo().IsGenericType && x.GetTypeInfo().GetGenericTypeDefinition() == typeof(ConnectedCapability<>)) diff --git a/test/Lsp.Tests/ClientCapabilityProviderTests.cs b/test/Lsp.Tests/ClientCapabilityProviderTests.cs index 534443538..165beb0de 100644 --- a/test/Lsp.Tests/ClientCapabilityProviderTests.cs +++ b/test/Lsp.Tests/ClientCapabilityProviderTests.cs @@ -42,15 +42,24 @@ public static IEnumerable AllowSupportedCapabilities() }); } - [Theory, MemberData(nameof(DisallowUnsupportedCapabilities))] - public void Should_DisallowUnsupportedCapabilities(IJsonRpcHandler handler, object instance) + [Theory, MemberData(nameof(AllowUnsupportedCapabilities))] + public void Should_AllowUnsupportedCapabilities(IJsonRpcHandler handler, object instance) { var textDocumentSyncHandler = TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cs")); var collection = new HandlerCollection(SupportedCapabilitiesFixture.AlwaysTrue) { textDocumentSyncHandler, handler }; var provider = new ClientCapabilityProvider(collection); - HasHandler(provider, instance).Should().BeFalse(); + HasHandler(provider, instance).Should().BeTrue(); + } + + public static IEnumerable AllowUnsupportedCapabilities() + { + return GetItems(Capabilities, type => { + var handlerTypes = GetHandlerTypes(type); + var handler = Substitute.For(handlerTypes.ToArray(), new object[0]); + return new[] { handler, Activator.CreateInstance(typeof(Supports<>).MakeGenericType(type), false) }; + }); } [Fact] @@ -81,17 +90,29 @@ public void Should_Invoke_Reduce_Delegate() stub.Received().Invoke(Arg.Any>()); } - public static IEnumerable DisallowUnsupportedCapabilities() + [Theory, MemberData(nameof(AllowNullSupportsCapabilities))] + public void Should_AllowNullSupportedCapabilities(IJsonRpcHandler handler, object instance) + { + var textDocumentSyncHandler = TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cs")); + + var collection = new HandlerCollection(SupportedCapabilitiesFixture.AlwaysTrue) { textDocumentSyncHandler, handler }; + var provider = new ClientCapabilityProvider(collection); + + HasHandler(provider, instance).Should().BeTrue(); + } + + public static IEnumerable AllowNullSupportsCapabilities() { return GetItems(Capabilities, type => { var handlerTypes = GetHandlerTypes(type); var handler = Substitute.For(handlerTypes.ToArray(), new object[0]); - return new[] { handler, Activator.CreateInstance(typeof(Supports<>).MakeGenericType(type), false) }; + return new[] { handler, Activator.CreateInstance(typeof(Supports<>).MakeGenericType(type), true) }; }); } - [Theory, MemberData(nameof(DisallowNullSupportsCapabilities))] - public void Should_DisallowNullSupportedCapabilities(IJsonRpcHandler handler, object instance) + + [Theory, MemberData(nameof(DisallowDynamicSupportsCapabilities))] + public void Should_DisallowDynamicSupportedCapabilities(IJsonRpcHandler handler, object instance) { var textDocumentSyncHandler = TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cs")); @@ -101,12 +122,14 @@ public void Should_DisallowNullSupportedCapabilities(IJsonRpcHandler handler, ob HasHandler(provider, instance).Should().BeFalse(); } - public static IEnumerable DisallowNullSupportsCapabilities() + public static IEnumerable DisallowDynamicSupportsCapabilities() { return GetItems(Capabilities, type => { var handlerTypes = GetHandlerTypes(type); var handler = Substitute.For(handlerTypes.ToArray(), new object[0]); - return new[] { handler, Activator.CreateInstance(typeof(Supports<>).MakeGenericType(type), true) }; + var capability = Activator.CreateInstance(type); + if (capability is DynamicCapability dyn) dyn.DynamicRegistration = true; + return new[] { handler, Activator.CreateInstance(typeof(Supports<>).MakeGenericType(type), true, capability) }; }); } diff --git a/test/Lsp.Tests/LanguageServerTests.cs b/test/Lsp.Tests/LanguageServerTests.cs index 0ed57bb2f..77ee99ced 100644 --- a/test/Lsp.Tests/LanguageServerTests.cs +++ b/test/Lsp.Tests/LanguageServerTests.cs @@ -66,6 +66,7 @@ public async Task GH141_CrashesWithEmptyInitializeParams() .WithLoggerFactory(LoggerFactory) .AddDefaultLoggingProvider() .WithMinimumLogLevel(LogLevel.Trace) + .AddHandlers(TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cs"))) ) as IRequestHandler; var handler = server as IRequestHandler; From 3ddab5f173b31f8b82e9d8670c34655e819c3210 Mon Sep 17 00:00:00 2001 From: David Driscoll Date: Fri, 12 Jul 2019 21:56:35 -0400 Subject: [PATCH 2/3] Added additional comment --- src/Server/ClientCapabilityProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Server/ClientCapabilityProvider.cs b/src/Server/ClientCapabilityProvider.cs index e00f836d9..d5088bafa 100644 --- a/src/Server/ClientCapabilityProvider.cs +++ b/src/Server/ClientCapabilityProvider.cs @@ -23,7 +23,7 @@ public bool HasStaticHandler(Supports capability) { // Dynamic registration will cause us to double register things if we report our capabilities staticly. // However if the client does not tell us it's capabilities we should just assume that they do not support - // dynamic registraiton + // dynamic registraiton but we should report any capabilities statically if (capability.IsSupported && capability.Value != null && capability.Value.DynamicRegistration == true) return false; var handlerTypes = typeof(T).GetTypeInfo().ImplementedInterfaces From e2868ae18d76e67506f7d4628ce411ac95ee1830 Mon Sep 17 00:00:00 2001 From: David Driscoll Date: Fri, 12 Jul 2019 22:13:37 -0400 Subject: [PATCH 3/3] Added addiitional mixed text to prove through it working with multiple items --- .../ClientCapabilityProviderTests.cs | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/test/Lsp.Tests/ClientCapabilityProviderTests.cs b/test/Lsp.Tests/ClientCapabilityProviderTests.cs index 165beb0de..e1d6eeeb1 100644 --- a/test/Lsp.Tests/ClientCapabilityProviderTests.cs +++ b/test/Lsp.Tests/ClientCapabilityProviderTests.cs @@ -9,6 +9,7 @@ using OmniSharp.Extensions.LanguageServer.Protocol; using OmniSharp.Extensions.LanguageServer.Protocol.Client.Capabilities; using OmniSharp.Extensions.LanguageServer.Protocol.Models; +using OmniSharp.Extensions.LanguageServer.Protocol.Server; using OmniSharp.Extensions.LanguageServer.Protocol.Server.Capabilities; using OmniSharp.Extensions.LanguageServer.Server; using Xunit; @@ -133,18 +134,38 @@ public static IEnumerable DisallowDynamicSupportsCapabilities() }); } - private static bool HasHandler(ClientCapabilityProvider provider, object instance) + [Fact] + public void Should_Handle_Mixed_Capabilities() { - return (bool)typeof(ClientCapabilityProviderTests).GetTypeInfo() - .GetMethod(nameof(GenericHasHandler), BindingFlags.Static | BindingFlags.NonPublic) - .MakeGenericMethod(instance.GetType().GetTypeInfo().GetGenericArguments()[0]).Invoke(null, new[] { provider, instance }); + var textDocumentSyncHandler = TextDocumentSyncHandlerExtensions.With(DocumentSelector.ForPattern("**/*.cs")); + + var codeActionHandler = Substitute.For(); + var definitionHandler = Substitute.For(); + var typeDefinitionHandler = Substitute.For(); + + var collection = new HandlerCollection(SupportedCapabilitiesFixture.AlwaysTrue) { textDocumentSyncHandler, codeActionHandler, definitionHandler, typeDefinitionHandler }; + var provider = new ClientCapabilityProvider(collection); + var capabilities = new ClientCapabilities() { + TextDocument = new TextDocumentClientCapabilities() { + CodeAction = new Supports(true, new CodeActionCapability() { + DynamicRegistration = false, + }), + TypeDefinition = new Supports(true, new TypeDefinitionCapability() { + DynamicRegistration = true, + }) + } + }; + + provider.GetStaticOptions(capabilities.TextDocument.CodeAction).Get(CodeActionOptions.Of).Should().NotBeNull(); + provider.HasStaticHandler(capabilities.TextDocument.Definition).Should().BeTrue(); + provider.HasStaticHandler(capabilities.TextDocument.TypeDefinition).Should().BeFalse(); } - private static bool HasHandler(ClientCapabilityProvider provider, Type type) + private static bool HasHandler(ClientCapabilityProvider provider, object instance) { return (bool)typeof(ClientCapabilityProviderTests).GetTypeInfo() .GetMethod(nameof(GenericHasHandler), BindingFlags.Static | BindingFlags.NonPublic) - .MakeGenericMethod(type).Invoke(null, new object[] { provider, null }); + .MakeGenericMethod(instance.GetType().GetTypeInfo().GetGenericArguments()[0]).Invoke(null, new[] { provider, instance }); } private static bool GenericHasHandler(ClientCapabilityProvider provider, Supports supports)