Add mapped edits helper #11146

ryzngard · 2024-11-04T04:29:05Z

Razor side of implementing IRazorMappingService.

Suggested file by file. Bulk of the implementation is in RazorEditHelper. Commits don't really tell a story but a lot of the changes are mechanical to implement new service + endpoint. The process is generally as follows:

Call LSP endpoint to map edits from IRazorMappingService. LSP endpoints cass RazorEditHelper.MapCSharpEditsAsync. This uses RazorEditHelper.TextChangeBuilder to get the initial changes the normalizes them since there may be overlapping changes.

Tests are in in RazorMapToDocumentEditsEndpointTest.cs and should generally describe functionality fairly well. I'm still flushing out more scenarios.

davidwengier

I love the tests in general, but I wonder if there might be false positives from manually supplying the mappings. Is it possible to create a test that gets a real set of changes from Roslyn when renaming a using in a C# file that is the real razor compiler output, and use those changes and mappings?

I didn't have the closest look at the actual RazorEditHelper algorithm, so not approving yet, but left a few comments for how far I've gotten so far. Nothing worrying though :)

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

...r/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorEditHelper.TextChangeBuilder.cs

davidwengier · 2024-11-05T05:38:52Z

...r/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorEditHelper.TextChangeBuilder.cs

+                    continue;
+                }
+
+                if (RazorSyntaxFacts.IsInUsingDirective(node))


Not for this PR obviously, and perhaps never if this isn't a problem, but I'm 99% sure that the Razor compiler always outputs all using directives in a contiguous block in C#, regardless of where they come from (ie, which block of usings, _Imports files, etc.). It might be worth having the compiler expose the start and end line, in the C# document, of the using directives and then just compare the edit against those two line numbers, rather than repeatedly digging through the Razor tree here.

ooo that's interesting. As long as that holds true it would simplify this a bit.

This looks to be true, but brings about a point related to #6155 in that we're not correctly handling changes that should go into _imports files. Let me think more on it....

I would be fine with v1 not handling _Imports files, as I don't think the compiler maps them at all. Doing so would need a filename added to SourceMapping or SourceSpan, and updating everywhere to check that property, and I don't think that is in the realms of possibility right now. In future (cohosting) we can use the #line mappings to handle this.

...r/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorEditHelper.TextChangeBuilder.cs

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

...icrosoft.VisualStudio.LanguageServices.Razor/DynamicFiles/DefaultDynamicDocumentContainer.cs

...c/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/RazorDynamicFileInfoProvider.cs

...icrosoft.AspNetCore.Razor.LanguageServer.Test/Mapping/RazorMapToDocumentEditsEndpointTest.cs

DustinCampbell

All of my feedback is relatively minor. Happy to drop a green check when tests are passing

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

...r/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorEditHelper.TextChangeBuilder.cs

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

...r/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorEditHelper.TextChangeBuilder.cs

...rosoft.VisualStudio.LanguageServices.Razor.Test/LanguageClient/RazorLSPMappingServiceTest.cs

DustinCampbell · 2024-11-05T22:57:13Z

...r/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/RazorLSPMappingService.cs

+            cancellationToken);
+    }
+
+    public async Task<ImmutableArray<RazorMappedEditoResult>> MapTextChangesAsync(


Amusingly, RazorMappedEditResult is misspelled in the EA. 😁

My worst version of spanglish to date

Too funny. 🤣

ryzngard · 2024-11-07T08:46:34Z

Is it possible to create a test that gets a real set of changes from Roslyn when renaming a using in a C# file that is the real razor compiler output, and use those changes and mappings?

I'd like to add one of these. Other than an integration test is it possible to fully set up a test workspace for this and unit test it? I was having trouble figuring it out but it's also quite late so brain may not be top tier functioning

davidwengier · 2024-11-07T09:57:42Z

Other than an integration test is it possible to fully set up a test workspace for this and unit test it?

Not sure we have anything outside of cohosting that does the whole thing, but we have the bones of it.

For getting a real generated C# document, with real mappings, yes. Anything that inherits from SingleServerDelegatingEndpointTestBase can call CreateLanguageServerAsync and pass in additionalRazorDocuments which will be compiled and added to the Roslyn workspace. eg, this test adds a component, and the test infra automatically adds the generated C#: https://github.com/dotnet/razor/blob/main/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Definition/DefinitionEndpointDelegationTest.cs#L148-L177

I wonder if you could use that, and put a test in RenameEndpointDelegationTest, which has one Razor file with @using Goo, one with @namespace Goo, and do a rename on the namespace name, which should delegate to Roslyn. Would that then hit the edit mapping service?

If not, I guess just poke around at what the CreateLanguageServerAsync method does, and CSharpTestLspServerHelpers.CreateCSharpTestWorkspace, and make a few things public and string them together some how?

davidwengier

Love the tests, and I don't think any of my comments are blocking. A real world test could be a pain so I don't think its a problem to follow up. Despite my comments about having the bones of it, an integration test might just be easier :)

I do wonder which tests fail, and in what way, if the AddComplexUsingsChanges method is removed, and we just always call AddNewUsingChanges and AddRemoveUsingsChanges

...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorMapToDocumentEditsEndpoint.cs

...Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditHelper.TextChangeBuilder.cs

davidwengier · 2024-11-07T10:34:07Z

...Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditHelper.TextChangeBuilder.cs

+                return;
+            }
+
+            AddComplexUsingsChanges(codeDocument, addedUsings, removedUsings, cancellationToken);


I'm not sure I understand why we need separate logic to have additions and removals. Can't we just get the add changes, then then the remove changes, and then sort them to make sure they're in order?

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditHelper.cs

davidwengier · 2024-11-07T10:45:18Z

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/AddUsingsHelper.cs

-            .Select(u => u.ToString()["using ".Length..^1]);
+            .Select(u => u.Name)
+            .WhereNotNull()
+            .SelectAsArray(n => n.ToString());


This is definitely a lot easier than the code I had before, but it does worry me ever so slightly.

Right now if a Razor file has:

@using System; @using X = System.DateTime; @using static System.Math;

they will be output into the generated C# verbatim (DotNetInternals), and just accessing the Name property will pull them out as ["System", "System.DateTime", "System.Math"] (SharpLab)

I admit, anything but a simple using statement is unlikely, but we do know that some users have using aliases, as they've reported bugs, and I think this change would mean those would still break under rename. Not sure if we need to worry about this, or wait for feedback, but at the same time I don't think there is harm in just putting back the old code. Or does it not work with the new edit helper?

Okay, I think this is your follow up, #11168. I'll leave the comment though, because that issue mentions the Razor tree being inconsistent, but I'm pretty sure which ever solution we come up with for that would also have to be consistent with this.

Perhaps the answer is just to use the string representation of the entire directive, sans @ in Razor and ; in C#

davidwengier · 2024-11-07T11:03:55Z

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/DynamicFiles/RazorMappingService.cs

+    private readonly IDocumentMappingService _documentMappingService = new DocumentMappingService(loggerFactory);
+    private readonly ILogger _logger = loggerFactory.GetOrCreateLogger<RazorMappingService>();
+
+    public async Task<ImmutableArray<RazorMappedSpanResult>> MapSpansAsync(Document document, IEnumerable<TextSpan> spans, CancellationToken cancellationToken)


I wonder if RazorMappingService should inherit from RazorSpanMappingService so we don't have to re-implement this method, and TryGetMappedSpans. Unless they're different?

I thought about that, but also am just considering removing the RazorSpanMappingService. I'll consider this a follow up cleanup

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Mapping/RazorEditHelperTest.cs

davidwengier · 2024-11-07T11:09:13Z

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Mapping/RazorEditHelperTest.cs

+            {|map1:public int Counter { get; set; }|} 
+        }
+        """,
+        newCSharpSource:


Okay this is the pettiest of nits, but perhaps it would be easier to rearrange the parameters so its C#, new C#, Razor, new Razor. Would make it easier to tell what has changed between new and old I think

...Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/RazorEditHelper.TextChangeBuilder.cs

jjonescz · 2024-11-08T08:50:03Z

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/TextChangeExtensions.cs

This change LGTM

…Mapping/UsingsNodeComparer.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

…Mapping/RazorEditHelper.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

ryzngard · 2024-11-11T20:39:53Z

I'm planning on getting this in today with the follow up items I filed. If you see anything else that seems off please let me know! It's less complicated than my last attempt but I think there's probably more work to simplify even further.

@davidwengier especially has a good comment on #11146 (comment) where I want to follow up and make sure assumptions I had before are true. 3 of the current tests fail but that doesn't mean there can't be further simplifications.

davidwengier · 2024-11-11T20:47:15Z

3 of the current tests fail but that doesn't mean there can't be further simplifications.

Would be curious to know in what fashion they fail too, and whether it's blocking. You're essentially implementing an algorithm that does both sort usings, group system usings first, and honours a users arbitrary grouping. An admirable goal, but perhaps something we don't need to hit.

ryzngard added 4 commits November 3, 2024 20:25

Add mapped edits helper

7c647e8

Add endpoint registration

ad48741

Change name

ca4d979

Cleanup

a411f87

ryzngard changed the title ~~[DRAFt] Add mapped edits helper~~ Add mapped edits helper Nov 5, 2024

ryzngard marked this pull request as ready for review November 5, 2024 02:26

ryzngard requested a review from a team as a code owner November 5, 2024 02:26

Naming

ef830e5

davidwengier reviewed Nov 5, 2024

View reviewed changes

ryzngard added 2 commits November 5, 2024 11:52

PR Feedback

da1bb46

Unify the 'blocking' behavior of finding usings and add a few more tests

398fbf7

DustinCampbell reviewed Nov 5, 2024

View reviewed changes

Cleanup based on PR feedback

626112f

ryzngard mentioned this pull request Nov 6, 2024

Rationalize how to compare namespaces across razor/roslyn #11168

Open

ryzngard added 5 commits November 5, 2024 16:31

Add skipped test

358e69e

Cleanup some more

0ae4bdb

Nullable stuff

2d883c9

Change to use RazorTextChange

5b6a83f

Log failures. Fix namespace finding from csharp. Fix LSP endpoint

579b12f

Feedback, formatting, and usings

880cc24

davidwengier approved these changes Nov 7, 2024

View reviewed changes

🤷

94a3d75

ryzngard requested a review from a team as a code owner November 7, 2024 22:33

🤷

acc6f71

chsienki approved these changes Nov 8, 2024

View reviewed changes

jjonescz approved these changes Nov 8, 2024

View reviewed changes

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/TextChangeExtensions.cs Outdated

Copy link

Member

jjonescz Nov 8, 2024

Choose a reason for hiding this comment

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

This change LGTM

ryzngard and others added 2 commits November 11, 2024 12:16

Update src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Document…

7565f89

…Mapping/UsingsNodeComparer.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

Update src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Document…

67ab783

…Mapping/RazorEditHelper.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

ryzngard mentioned this pull request Nov 11, 2024

Use new RazorEditHelper for all CSharp edits #11196

Open

PR Feedback

99d61ab

ryzngard added 4 commits November 11, 2024 15:14

I hate mocks

0197cc0

More mocks...

674cc43

Remove more mocks because I don't know the correct way to do out args

fa9741b

😢

5cd1b61

ryzngard merged commit 1f24912 into dotnet:main Nov 12, 2024
11 of 12 checks passed

dotnet-policy-service bot added this to the Next milestone Nov 12, 2024

ryzngard mentioned this pull request Nov 13, 2024

C# extension stopped working suddenly dotnet/vscode-csharp#7734

Open

This was referenced Nov 14, 2024

[Automated] PRs inserted in VS build main-35514.04 #11210

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35514.242 #11216

Closed

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35518.109 #11227

Closed

jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mapped edits helper #11146

Add mapped edits helper #11146

ryzngard commented Nov 4, 2024 •

edited

Loading

davidwengier left a comment

davidwengier Nov 5, 2024 •

edited

Loading

ryzngard Nov 5, 2024

ryzngard Nov 7, 2024

davidwengier Nov 11, 2024

DustinCampbell left a comment

DustinCampbell Nov 5, 2024

ryzngard Nov 5, 2024

DustinCampbell Nov 5, 2024

ryzngard commented Nov 7, 2024

davidwengier commented Nov 7, 2024

davidwengier left a comment

davidwengier Nov 7, 2024

davidwengier Nov 7, 2024

davidwengier Nov 7, 2024

davidwengier Nov 7, 2024

ryzngard Nov 11, 2024

davidwengier Nov 7, 2024

jjonescz Nov 8, 2024

ryzngard commented Nov 11, 2024

davidwengier commented Nov 11, 2024

Add mapped edits helper #11146

Add mapped edits helper #11146

Conversation

ryzngard commented Nov 4, 2024 • edited Loading

davidwengier left a comment

Choose a reason for hiding this comment

davidwengier Nov 5, 2024 • edited Loading

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

DustinCampbell 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

ryzngard commented Nov 7, 2024

davidwengier commented Nov 7, 2024

davidwengier 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

ryzngard commented Nov 11, 2024

davidwengier commented Nov 11, 2024

ryzngard commented Nov 4, 2024 •

edited

Loading

davidwengier Nov 5, 2024 •

edited

Loading