Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Omnisharp Version #1763

Closed
wants to merge 1 commit into from

Conversation

ryanbrandenburg
Copy link
Contributor

Fixes https://github.com/dotnet/aspnetcore/issues/20320.

This is THE MOST DRAFT PR. Just trying to get something out in front of everyone before the weekend so we can iterate quickly on Monday. Will do general cleanup and explanations on Monday when it's not already past quitting time.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@@ -1,6 +1,14 @@
<?xml version="1.0" encoding="utf-8"?>
<Dependencies>
<ProductDependencies>
<Dependency Name="Microsoft.Extensions.Configuration" Version="5.0.0-preview.4-runtime.20201.19">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added with Darc I assume? We may need to pin this version outside of darc is why I ask. @JunTaoLuo did work to make it so we don't depend on pieces of the extensions repo. He can also weigh in to see if this is a problem.

@@ -123,8 +125,9 @@
<MonoDevelopSdkPackageVersion>1.0.15</MonoDevelopSdkPackageVersion>
<MoqPackageVersion>4.10.0</MoqPackageVersion>
<!-- STOP!!! We need to reference the version of JSON that our HOSTS supprt. -->
<NewtonsoftJsonPackageVersion>9.0.1</NewtonsoftJsonPackageVersion>
<OmniSharpExtensionsLanguageServerPackageVersion>0.14.2</OmniSharpExtensionsLanguageServerPackageVersion>
<NewtonsoftJsonPackageVersion>11.0.2</NewtonsoftJsonPackageVersion>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you verify that this in VS by booting VS (not by hitting f5 in our solution), attaching and seeing the version of Newtonsoft.Json loaded?

@KirillOsenkov what version of Newtonsoft.Json is VS4Mac on?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're on 12.0.2. We have a binding redirect in place so we should be good with 11.0.2 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ya'll are using 12.0.2 I may as well do so too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ya'll are using 12.0.2 I may as well do so too.

Gotta make sure it's in VS Windows though too

@@ -9,6 +9,7 @@

<ItemGroup>
<PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonPackageVersion)" />
<PackageReference Include="OmniSharp.Extensions.LanguageProtocol" Version="$(OmniSharpExtensionsLanguageProtocolPackageVersion)" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is here because of the JsonConverter pieces. Could we move those from the Common project to the LanguageServer project? I ask because this project is used by O#'s plugin and our language server. In the plugin world we don't need the converters you added.


public static void AddDelegatingSupportsConverter(JsonConverterCollection collection)
{
Type supportsConverterType = typeof(PublishDiagnosticsCapability).Assembly.GetType("OmniSharp.Extensions.LanguageServer.Protocol.Serialization.Converters.SupportsConverter");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahah, oh man this be ugly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole DelegatingSupportsConverter class is brought mostly straight from Todd's PR. Still, if you have a suggestion for cleaning this up I'm all ears.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine if we contain the ugliness. Was just mentioning it is all 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ugliness took quite a while to figure out, unfortunately :(

@@ -35,7 +36,7 @@ private RazorLanguageServer()
{
}

public static Task<ILanguageServer> CreateAsync(Stream input, Stream output, Trace trace)
public static Task<ILanguageServer> CreateAsync(Stream input, Stream output, Trace trace, Version version = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rename version to defaultProtocolVersion since it represents a part of the protocol that we fall back to when ClientInfo isn't provided.

I thought a bit more about this too and another route here would be to update VS' language server protocol extension to pass this along. I wonder how hard that'd be.


if (version is null)
{
version = new Version("1337");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this version into a private static readonly Version MaxSupportedProtocol = new Version(1337, 0, 0, 0) with a comment about us picking a large number to emulate "latest" support and do

Suggested change
version = new Version("1337");
version = MaxSupportedProtocol;

version = new Version("1337");
}

if (version > new Version(3, 14, 0, 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd switch up these statements a little bit and create a private static readonly Version Protocol_3_14

if (clientProtocol <= Protocol_3_14)
{
    // Assume any protocol before 3.14 expects a bool capabilities for some of the server side capabilities.

	if(s.ServerSettings.Capabilities.HoverProvider.IsValue)
	{
		s.ServerSettings.Capabilities.HoverProvider = new BooleanOr<HoverOptions>(true);
	}

	if(s.ServerSettings.Capabilities.DocumentRangeFormattingProvider.IsValue)
	{
		s.ServerSettings.Capabilities.DocumentRangeFormattingProvider = new BooleanOr<DocumentRangeFormattingOptions>(true);
	}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Food for thought: If all of this code ends up growing outside of this PR we may want to take the approach of building some sort of protocol configuration system

@@ -69,10 +68,7 @@ public async Task<Connection> ActivateAsync(CancellationToken token)
{
var (clientStream, serverStream) = FullDuplexStream.CreatePair();

// Need an auto-flushing stream for the server because O# doesn't currently flush after writing responses. Without this
// performing the Initialize handshake with the LanguageServer hangs.
var autoFlushingStream = new AutoFlushingStream(serverStream);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think we might still need this because O# calls Flush (not FlushAsync) after writes. Could you try writing a TagHelper attribute (via completions) to ensure this doesn't explode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, this scenario actually explodes with or without the AutoFlushingStream.

[assembly: ProvideCodeBase(CodeBase = @"$PackageFolder$\Microsoft.Extensions.FileSystemGlobbing.dll")]
[assembly: ProvideCodeBase(CodeBase = @"$PackageFolder$\Microsoft.Extensions.FileProviders.Abstractions.dll")]
[assembly: ProvideCodeBase(CodeBase = @"$PackageFolder$\Microsoft.Extensions.FileProviders.Physical.dll")]
[assembly: ProvideCodeBase(CodeBase = @"$PackageFolder$\System.Reactive.dll")]
[assembly: ProvideBindingRedirection(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should talk with WTE folks to see if this is the recommended way to have codebases/binding redirections (having both vs. having just one)

@@ -360,6 +360,8 @@ public TestServer()

public ILanguageServerWorkspace Workspace => throw new NotImplementedException();

public ProgressManager ProgressManager => throw new NotImplementedException();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP i assume right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to latest O# LangaugeServer Extension
4 participants