*** WIP: Update roslyn package references to a newer version. by ToddGrun · Pull Request #11901 · dotnet/razor

ToddGrun · 2025-05-30T21:45:38Z

This is in anticipation of un-reverting a change I made in roslyn (dotnet/roslyn#78165). That change will need a coordinated Roslyn/Razor insertion, thus this PR attempts to move razor forward to current tip of roslyn before attempting that change.

There were two recent changes to roslyn that required changes in razor as part of this update:

The Uri => DocumentUri change. This is the vast majority of changes in this PR.
The EditorFeatures.WPF removal.

Starting as a draft PR as I'd like to get ci test results (with something I want to try out in a later commit once tests pass)

This is in anticipation of a change I made (dotnet/roslyn#78165) that was reverted and will need a coordinated Roslyn/Razor insertion. There were two recent changes to roslyn that required changes in razor as part of this update: 1) The Uri => DocumentUri change. This is the *vast* majority of changes in this PR. 2) The EditorFeatures.WPF removal.

ToddGrun · 2025-05-30T21:50:40Z

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

        if (initializeParams.WorkspaceFolders is [var firstFolder, ..])
        {
-            return firstFolder.Uri.GetAbsoluteOrUNCPath();
+            return firstFolder.DocumentUri.GetRequiredParsedUri().GetAbsoluteOrUNCPath();


GetAbsoluteOrUNCPath

Any place that required this now calls it via the result of DocumentUri.GetRequiredParsedUri

ToddGrun · 2025-05-30T22:19:43Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/MapCode/MapCodeEndpoint.cs

                // ever changes, we'll need to update this code to account for it (i.e., take into account
                // focus location URIs).
-                Debug.Assert(location.Uri == documentContext.Uri);
+                // TODO(toddgrun): switch back to == when roslyn implementation of DocumentUri.operator== is available on ci


// TODO(toddgrun): switch back to == when roslyn implementation of DocumentUri.operator== is available on ci

There were five places I found where I put this comment and switched the code from using operator== to calling Equals. I just added the operator== implementation to the DocumentUri class in roslyn yesterday, so it can't be used until:

Razor updates to a roslyn package that includes that

(I think) The ci has a roslyn installed that has that.

I'd love clarity on 2) as it seems like that could be a while down the road.

Number 2 is correct. Since we don't ship the Razor EA, we have to wait for any changes to it to be inserted into VS main. That normally doesn't take too long though. This is different to Roslyn where if you sometimes have to wait for something to be in a public preview, we only need to wait for main because our integration tests run on main, and we assume all team members work on VS main/IntPreview.

ToddGrun · 2025-05-30T22:20:45Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorLanguageQueryParams.cs

 {
    [JsonPropertyName("uri")]
-    public required Uri Uri { get; set; }
+    [JsonConverter(typeof(DocumentUriConverter))]


[JsonConverter(typeof(DocumentUriConverter))]

Matches how Roslyn marks serializable properties of this type.

ToddGrun · 2025-05-30T22:23:25Z

...rc/Microsoft.CodeAnalysis.Razor.CohostingShared/SignatureHelp/CohostSignatureHelpEndpoint.cs


    protected override RazorTextDocumentIdentifier? GetRazorTextDocumentIdentifier(SignatureHelpParams request)
-        => new RazorTextDocumentIdentifier(request.TextDocument.Uri, (request.TextDocument as VSTextDocumentIdentifier)?.ProjectContext?.Id);
+        => new RazorTextDocumentIdentifier(request.TextDocument.DocumentUri.GetRequiredParsedUri(), (request.TextDocument as VSTextDocumentIdentifier)?.ProjectContext?.Id);


RazorTextDocumentIdentifier

This is defined in the roslyn ea layer, so not something that can be changed in this PR.

Make GetAbsoluteOrUNCPath callable off both Uri and DocumentUri

ToddGrun · 2025-05-30T23:23:55Z

...Studio.RazorExtension.Dependencies/Microsoft.VisualStudio.RazorExtension.Dependencies.csproj

    <VSIXSourceItem Include="$(OutputPath)Microsoft.CodeAnalysis.CSharp.Workspaces.dll" />
    <VSIXSourceItem Include="$(OutputPath)Microsoft.CodeAnalysis.EditorFeatures.dll" />
    <VSIXSourceItem Include="$(OutputPath)Microsoft.CodeAnalysis.EditorFeatures.Text.dll" />
-    <VSIXSourceItem Include="$(OutputPath)Microsoft.CodeAnalysis.EditorFeatures.Wpf.dll" />


This assembly no longer exists

2) Revert a test back to it's original behavior

ToddGrun · 2025-05-30T23:52:48Z

...t.AspNetCore.Razor.Test.Common.Tooling/Microsoft.AspNetCore.Razor.Test.Common.Tooling.csproj

  </ItemGroup>

  <ItemGroup Condition="'$(TargetFramework)' == '$(NetFxVS)'">
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.EditorFeatures" />


EditorFeature packages are only published for netfx, not for net8 or net9

ToddGrun · 2025-05-30T23:55:24Z

src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/Mef/TestComposition.cs

        ImmutableHashSet<Type>.Empty,
        ImmutableHashSet<Type>.Empty);

+#if NETFRAMEWORK


This catalog is only valid for netfx

2) Changed tests back to using RootPath 3) Cleanup / spelling

davidwengier

This mostly LGTM, though see my comment about whether it perhaps makes changes a little too deeply. Wasn't clear from your comments whether you're planning another Roslyn change before opening this properly, but let me know if/when you want a proper review with a green tick.

davidwengier · 2025-06-01T22:34:38Z

eng/Version.Details.xml

    <Dependency Name="Microsoft.CodeAnalysis.Analyzers" Version="3.12.0-beta1.25230.6">
      <Uri>https://github.com/dotnet/roslyn</Uri>
-      <Sha>ded867328249b5a9b9e6e29e3f07abc19111f5d1</Sha>
+      <Sha>392fbb66a16ca3d34abdfa93cc5a3717f447d25e</Sha>


This one always trips me up too, it builds from the same commit, but as a different version, so its easy to accidentally update the sha without bumping the package if you just find-and-replace (which, to be clear, is exactly what I do too 😁)

Argh, find and replace got me again!

davidwengier · 2025-06-01T22:47:43Z

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/UriExtensions.cs

+        if (uri is null)
+        {
+            throw new ArgumentNullException(nameof(uri));
+        }


Is this necessary in a nullable annotated file? If so, we have an ArgHelper.ThrowIfNull helper method

Switching to ArgHelper. Lots of callers, didn't want to verify that they all are coming from nullable annotated contexts, so I kept it the same as the other method's check.

davidwengier · 2025-06-01T22:57:10Z

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorCodeDocumentExtensions.cs

    public static bool TryGetGeneratedDocument(
        this RazorCodeDocument codeDocument,
-        Uri generatedDocumentUri,
+        DocumentUri generatedDocumentUri,


It's not necessarily a huge problem, but this is a good example of a thought I'd had a few times looking at the changes: Are we going a little bit too far with them?

The idea is to get Uri parsing out of LSP message deserialization, so moving away from Uri to DocumentUri for everything that is at the boundary makes sense. This feels like a step further, where we're essentially saying that we'll be able to possibly produce a RazorCodeDocument even if a Uri is not valid, and that doesn't feel right. It feels like for this API, and for others at a similar level, we are probably fine to just expect the LSP message handlers to pass in the parsed Uri, so that if there is invalid data coming in it's validated relatively early (just not as early as deserialization).

That's a very fair point, this did expand to be a lot more than I had originally thought it would. I'll take a look to see where it can be reigned in a bit, but if you can point out the locations where you think it's gone a bit too far, that would be helpful.

even if a Uri is not valid

Wanted to make a clarification that issues here are most often caused by actual valid URIs that System.Uri barfs on. So ultimately that means these URIs are not parse-able (unless we write our own parser).

Generally it is up to you if parsing the URI is a requirement for running your handlers. For Roslyn it isn't a hard requirement (they'll become misc files)

Oooh thats a good distinction I missed/forgot. We definitely use the filename of the Uri to find/create various documents, so I think technically we do need things to be parsable. Probably a good rule to use though (ie, IFilePathService is the thing that does that work, so it should take Uri and let that inform the changes). Seems like we could go either way though really, and things would be pretty viral either way. Doesn't feel like something to nail down in this PR, but we can pivot later as we work out what works best.

dibarbet · 2025-06-02T18:54:23Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/MapCode/MapCodeEndpoint.cs

            }

-            if (!_documentContextFactory.TryCreate(mapping.TextDocument.Uri, out var documentContext))
+            if (!_documentContextFactory.TryCreate(mapping.TextDocument.DocumentUri.GetRequiredParsedUri(), out var documentContext))


I wasn't expecting the DocumentUri change to be so viral, but this appears to be one of the reasons why - directly passing in just the URI type to helpers such as these.

Would it be reasonable to have helpers like this take in for example a TextDocumentIdentifier instead? I think that is one reason the Roslyn side changes were a bit easier

(to be clear, not suggesting it happens in this PR, but as a general improvement).

…r to not take in DocumentUris

ToddGrun · 2025-06-03T02:03:23Z

Unit tests seem to be a bit flaky now, they seem to pass on my dev machine, so not sure what's going on and the test logs aren't pointing me to the culprit. Will take a look again tomorrow with fresh eyes.

davidwengier · 2025-06-03T12:24:24Z

I had some flakiness on one of my PRs tonight, with complement unrelated changes. Probably can just re-run.

ToddGrun · 2025-06-17T13:44:11Z

David was able to get this to work in #11949. Closing this one out.

ToddGrun commented May 30, 2025

View reviewed changes

Change RazorRequestContext.Uri to a DocumentUri too

6e5b300

ToddGrun commented May 30, 2025

View reviewed changes

Move DocumentUri to a couple other locations.

997095a

Make GetAbsoluteOrUNCPath callable off both Uri and DocumentUri

ToddGrun commented May 30, 2025

View reviewed changes

1) Fix another location that was == on DocumentUri

6d896a1

2) Revert a test back to it's original behavior

ToddGrun commented May 30, 2025

View reviewed changes

ToddGrun added 2 commits May 30, 2025 17:00

1) Found another == location

2185d78

2) Changed tests back to using RootPath 3) Cleanup / spelling

Remove wpf entry from versions.props

3329c43

ToddGrun requested a review from dibarbet May 31, 2025 01:27

davidwengier reviewed Jun 1, 2025

View reviewed changes

pull back the line for the changes to DocumentUri

c7dd6fe

dibarbet reviewed Jun 2, 2025

View reviewed changes

ToddGrun added 5 commits June 2, 2025 12:11

Change APIs for IDocumentpMappingService and IHtmlDocumentSynchronize…

ef05010

…r to not take in DocumentUris

Remove DocumentUri from TestLSPDocumentSnapshot and TestDocumentManager

2966a78

Change back apis on some test language servers to take in Uri

5481eb3

Further reduction in changes

d7f6b2f

a little more cleanup

0f1e1af

davidwengier mentioned this pull request Jun 17, 2025

Followups to previous PR that need EA changes #11949

Merged

ToddGrun closed this Jun 17, 2025

Conversation

ToddGrun commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dibarbet Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun commented Jun 3, 2025

Uh oh!

davidwengier commented Jun 3, 2025

Uh oh!

ToddGrun commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ToddGrun commented May 30, 2025 •

edited

Loading

ToddGrun May 30, 2025 •

edited

Loading

dibarbet Jun 2, 2025 •

edited

Loading