Rybrande/o#upgrade v2 #2439

ryanbrandenburg · 2020-08-26T23:26:01Z

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

This is a huge change with lots to clean up, putting it up early so yall can play with it, but let me know if you find any functional or code-style problems.

NuGet.config

eng/Versions.props

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageServer.cs

...zor/src/Microsoft.VisualStudio.LanguageServer.ContainedLanguage/DefaultLSPDocumentManager.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/AutoFlushingNerdbankStream.cs

...est/Microsoft.AspNetCore.Razor.LanguageServer.Common.Test/BackgroundDocumentGeneratorTest.cs

...oft.VisualStudio.LanguageServerClient.Razor/DefaultRazorLanguageServerCustomMessageTarget.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageServer.cs

...c/Microsoft.AspNetCore.Razor.LanguageServer/Microsoft.AspNetCore.Razor.LanguageServer.csproj

...c/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperDescriptionFactory.cs

ajaybhargavb

Looks good! Nothing major. Some questions and the rest of it is just cleanups.

eng/Versions.props

...c/Microsoft.AspNetCore.Razor.LanguageServer/Completion/DefaultTagHelperDescriptionFactory.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultDocumentVersionCache.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultRazorConfigurationService.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Definition/RazorDefinitionEndpoint.cs

...oft.VisualStudio.LanguageServerClient.Razor/DefaultRazorLanguageServerCustomMessageTarget.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorLanguageServerClient.cs

src/Razor/src/Microsoft.VisualStudio.RazorExtension/AssemblyBindingRedirects.cs

...est/Microsoft.AspNetCore.Razor.LanguageServer.Common.Test/BackgroundDocumentGeneratorTest.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer.Common/LanguageServerConstants.cs

eng/Build.props

eng/Signing.props

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AutoInsert/OnAutoInsertEndpoint.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionEndpoint.cs

src/Razor/src/Microsoft.VisualStudio.RazorExtension/AssemblyBindingRedirects.cs

NTaylorMullen · 2020-08-27T20:35:19Z

src/Razor/src/Microsoft.VisualStudio.RazorExtension/AssemblyBindingRedirects.cs

+//     OldVersionLowerBound = "8.0.0.0",
+//     OldVersionUpperBound = "8.1.0.0",
+//     NewVersion = "8.1.0.0")]
+[assembly: ProvideBindingRedirection(


This definitely isn't ideal. @david-driscoll is this requirement introduced based on a dependency that O# depends on outside of our control?

I think I've bumped all of Msft.Ext to 3.1

I'm not using any new features so technically they can drop back down to 2.1 if you need.

I think that would be really helpful for us, otherwise we need this redirect and it might cause big problems.

Is this something we can get access to early? Only asking because it's gonna be an ngen pain to insert this 😄

Does this apply to logging as well, or just DI?

Currently we have these:

<PackageReference Update="Microsoft.Extensions.Logging" Version="3.1.0" /> <PackageReference Update="Microsoft.Extensions.Logging.Abstractions" Version="3.1.0" /> <PackageReference Update="Microsoft.Extensions.Logging.Debug" Version="3.1.0" /> <PackageReference Update="Microsoft.Extensions.Configuration.Abstractions" Version="3.1.0" /> <PackageReference Update="Microsoft.Extensions.Configuration" Version="3.1.0" /> <PackageReference Update="Microsoft.Extensions.DependencyInjection" Version="3.1.0" /> <PackageReference Update="Microsoft.Extensions.DependencyInjection.Abstractions" Version="3.1.0" />

I can drop all or some of them. down to 2.1 or 2.0?

So we don't necessarily care about which version they are as long as the dependency graph is coherent. Given we run our language server in VS which is a .NET framework app it relies on a hugeeee set of binding redirects to ensure that every dependency uses the correct version of a dll; however, every time we have to include a binding redirect we essentially restrict what versions of an assembly can exist for other teams because we then binding redirect it to our own. Hope that makes sense 😄

If you hit me up on Teams or Slack I can figure out the best solution.

If this works for you guys, heres the proposed change: OmniSharp/csharp-language-server-protocol#342

src/Razor/src/Microsoft.VisualStudio.RazorExtension/AssemblyCodeBases.cs

...Razor/src/Microsoft.VisualStudio.RazorExtension/Microsoft.VisualStudio.RazorExtension.csproj

src/Razor/src/Microsoft.VisualStudio.RazorExtension/source.extension.vsixmanifest

ryanbrandenburg · 2020-08-28T18:24:04Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DocumentUriExtensions.cs

+                throw new ArgumentNullException(nameof(documentUri));
+            }
+
+            return documentUri.ToUri().GetAbsoluteOrUNCPath();


I know this is still doing ToUri and creating additional objects, but getting the detection of UNC and absolute paths correct is harder than I think we want to deal with in this PR, to I used extension methods to wrap everything so we can deal with it later.

ya that's fine

DocumentUri should support unc and other paths, it's based off the vscode implementation with a suite of tests. This was important for @TylerLeonhardt .

This test suite was shamelessly pilfered from vscode. And there are some additional ones as well just ensuring it works with various different character types.

If there is one thing I'm an expert of, I'm not an expert of anything (I'm not...) it's converting C# to TypeScript. or TypeScript to C#, lol.

ryanbrandenburg · 2020-08-28T19:20:06Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultDocumentVersionCache.cs

        {
            if (documentSnapshot == null)
            {
                throw new ArgumentNullException(nameof(documentSnapshot));
            }

-            if (version < 0)
+            if (version is null)


We basically already had the null version handling, it was just using -1, so I converted anywhere that handled -1 to use int?

ryanbrandenburg · 2020-08-28T21:02:32Z

🆙📅

TanayParikh · 2020-08-28T21:40:22Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/RazorFormattingEndpoint.cs

@@ -65,9 +66,17 @@ internal class RazorFormattingEndpoint : IDocumentFormattingHandler, IDocumentRa
            _logger = loggerFactory.CreateLogger<RazorFormattingEndpoint>();
        }

-        public TextDocumentRegistrationOptions GetRegistrationOptions()
+        DocumentRangeFormattingRegistrationOptions IRegistration<DocumentRangeFormattingRegistrationOptions>.GetRegistrationOptions()


Access modifiers

This is actually not allowed because this method is set to only show when the object is accessed as a IRegistration<DocumentRangeFormattingRegistrationOptions>, further modifiers are forbidden.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/RazorFormattingEndpoint.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDocumentSynchronizationEndpoint.cs

ajaybhargavb

LGTM

TanayParikh

👏 👏 👏 👏 👏

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageServer.cs

NTaylorMullen

Requesting changes mostly because we can't break WTE without more thought

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultRazorConfigurationService.cs

NTaylorMullen · 2020-08-28T21:11:43Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DocumentUriExtensions.cs

+                throw new ArgumentNullException(nameof(documentUri));
+            }
+
+            return documentUri.ToUri().GetAbsoluteOrUNCPath();


ya that's fine

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DocumentUriExtensions.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DocumentVersionCache.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/RazorFormattingEndpoint.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServer.ContainedLanguage/DefaultLSPDocument.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServer.ContainedLanguage/VirtualDocument.cs

NTaylorMullen · 2020-08-28T21:49:06Z

src/Razor/src/Microsoft.VisualStudio.RazorExtension/AssemblyBindingRedirects.cs

+//     OldVersionLowerBound = "8.0.0.0",
+//     OldVersionUpperBound = "8.1.0.0",
+//     NewVersion = "8.1.0.0")]
+[assembly: ProvideBindingRedirection(


Is this something we can get access to early? Only asking because it's gonna be an ngen pain to insert this 😄

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/RazorDiagnosticsPublisherTest.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDocumentSynchronizationEndpoint.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServer.ContainedLanguage/LSPDocument.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServer.ContainedLanguage/DefaultLSPDocument.cs

ghost · 2020-08-29T00:30:21Z

Hello @ryanbrandenburg!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This reverts commit 6a84cf5. # Conflicts: # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpOnTypeFormattingPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CodeBlockDirectiveFormattingPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/DefaultRazorFormattingService.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingContentValidationPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingPassBase.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormatter.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/OnTypeFormattingStructureValidationPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Refactoring/RazorComponentRenameEndpoint.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingContentValidationPassTest.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingLanguageServerClient.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingTestBase.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/OnTypeFormattingStructureValidationPassTest.cs

* Revert "Rybrande/o#upgrade v2 (#2439)" This reverts commit 6a84cf5. # Conflicts: # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpOnTypeFormattingPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CodeBlockDirectiveFormattingPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/DefaultRazorFormattingService.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingContentValidationPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingPassBase.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormatter.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/OnTypeFormattingStructureValidationPass.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Refactoring/RazorComponentRenameEndpoint.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingContentValidationPassTest.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingLanguageServerClient.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingTestBase.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/OnTypeFormattingStructureValidationPassTest.cs * Addressed code review comments and commented out all formatting tests.

ryanbrandenburg requested review from ajaybhargavb, NTaylorMullen and TanayParikh August 26, 2020 23:26

TanayParikh reviewed Aug 26, 2020

View reviewed changes

NuGet.config Outdated Show resolved Hide resolved

TanayParikh suggested changes Aug 26, 2020

View reviewed changes

ajaybhargavb suggested changes Aug 27, 2020

View reviewed changes

NTaylorMullen requested changes Aug 27, 2020

View reviewed changes

ryanbrandenburg force-pushed the rybrande/O#UpgradeV2 branch from 0790f9b to c1e3064 Compare August 28, 2020 16:48

ryanbrandenburg commented Aug 28, 2020

View reviewed changes

ryanbrandenburg marked this pull request as ready for review August 28, 2020 21:31

ryanbrandenburg requested a review from dougbu as a code owner August 28, 2020 21:31

ryanbrandenburg requested review from NTaylorMullen, TanayParikh and ajaybhargavb August 28, 2020 21:31

TanayParikh reviewed Aug 28, 2020

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/RazorFormattingEndpoint.cs Outdated Show resolved Hide resolved

TanayParikh reviewed Aug 28, 2020

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDocumentSynchronizationEndpoint.cs Outdated Show resolved Hide resolved

ajaybhargavb approved these changes Aug 28, 2020

View reviewed changes

TanayParikh approved these changes Aug 28, 2020

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageServer.cs Show resolved Hide resolved

NTaylorMullen requested changes Aug 28, 2020

View reviewed changes

ryanbrandenburg requested a review from NTaylorMullen August 28, 2020 23:07

NTaylorMullen approved these changes Aug 28, 2020

View reviewed changes

ryanbrandenburg force-pushed the rybrande/O#UpgradeV2 branch from fa34670 to 61ffa9e Compare August 29, 2020 00:24

ryanbrandenburg added the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Aug 29, 2020

ryanbrandenburg added 3 commits August 30, 2020 21:20

O# upgrade

ed26d5f

Fix recursion

33a1182

Fix update buffer

407fe8a

ryanbrandenburg added 15 commits August 30, 2020 21:20

Remove bad nuget badness

688f655

PR feedback

587743b

PR feedback

81f932e

PR feedback

37c0a3f

PR feedback

49c2f4f

Fix compile errors

9c55939

long to int

e082b91

Mandatory int?s

064fd7f

use null instead of -1

0afca42

Cleanup

51d2e8c

PR feedback

c00db98

Don't break WTE

22d138a

Fix WTE

51bcbed

PR feedback

8d584b4

Update tests

671cfbe

ryanbrandenburg force-pushed the rybrande/O#UpgradeV2 branch from dc1cce7 to 671cfbe Compare August 31, 2020 04:20

ryanbrandenburg added 2 commits August 31, 2020 10:26

Dump handling

ed9ad9e

Fix tests

ef56c43

ryanbrandenburg removed the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Aug 31, 2020

Revert dump stuff

ba0a3ef

ryanbrandenburg merged commit 6a84cf5 into master Aug 31, 2020

ryanbrandenburg deleted the rybrande/O#UpgradeV2 branch August 31, 2020 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rybrande/o#upgrade v2 #2439

Rybrande/o#upgrade v2 #2439

ryanbrandenburg commented Aug 26, 2020

ajaybhargavb left a comment

NTaylorMullen Aug 27, 2020

david-driscoll Aug 28, 2020

ryanbrandenburg Aug 28, 2020

NTaylorMullen Aug 28, 2020

david-driscoll Aug 28, 2020

NTaylorMullen Aug 29, 2020

david-driscoll Aug 29, 2020

david-driscoll Aug 29, 2020

ryanbrandenburg Aug 28, 2020

NTaylorMullen Aug 28, 2020

david-driscoll Aug 28, 2020

david-driscoll Aug 29, 2020

ryanbrandenburg Aug 28, 2020

ryanbrandenburg commented Aug 28, 2020

TanayParikh Aug 28, 2020

ryanbrandenburg Aug 28, 2020

ajaybhargavb left a comment

TanayParikh left a comment

NTaylorMullen left a comment

NTaylorMullen Aug 28, 2020

NTaylorMullen Aug 28, 2020

ghost commented Aug 29, 2020

Rybrande/o#upgrade v2 #2439

Rybrande/o#upgrade v2 #2439

Conversation

ryanbrandenburg commented Aug 26, 2020

ajaybhargavb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb left a comment

Choose a reason for hiding this comment

TanayParikh left a comment

Choose a reason for hiding this comment

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 29, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.