Replace the algorithm used to calculate semantic diff #2126

ryanbrandenburg · 2020-06-27T00:14:34Z

It turns out there are cases which cause the algorithm seen here to fail (Index out of bounds exception). So we need a new way to diff our old and new results. Thankfully @ajaybhargavb wrote his TextDiffer with an excelent abstraction which let me plug my only-kinda-related scenario in and re-use it with only a relatively thin shim.

I tested this in VSCode and VS. VSCode worked without trouble, but VS ran into an issue (When I would remove one token actualy 5 get remove). Talking with @gundermanc we think it's a problem on their end that he's already sent a fix for, but we might want to double check that that's true before merging this.

This is the draftiest PR. I was "finishing up" right at EOD Friday, so feel free to tear it apart.

ryanbrandenburg · 2020-06-27T00:17:16Z

Oh, and I know it will fail some tests, I need to change their expectations on Monday.

gundermanc · 2020-06-27T00:40:53Z

VS implementation is still being stabilized, so there are some rough edges.

Here's the fix for the bug in question: https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/257407

ajaybhargavb

Added some suggestion for better factoring. Otherwise the design LGTM. Ready for the non-draft version 👍

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/SyntaxTokenToSemanticTokensEditHelper.cs

gundermanc · 2020-06-27T03:27:37Z

@ryanbrandenburg I think I found a bug:

I'm getting a response in the VS client that is inserting 11 integers in the diff (2 tokens and 1 superfluous int) but not deleting anything.

My understanding/expectation would be that in order to insert 11 integers, you'd have to have a complimentary deleteCount of 1 to ensure that the patched array is a multiple of 5 (entirely composed of whole tokens).

Does that sound right?

Repro:

gundermanc · 2020-06-27T03:37:09Z

Ah, nevermind, I see what's going on: your edits aren't self contained. One edit is deleting integers and the other is inserting new partial tokens... both at the same location.

If I actually use an array instead of the incremental TagSpan tree updates we use in the VS editor, you'd get the desired result.

Does VS Code handle this scenario correctly? On the one hand, I'd expect edits at a particular location should combine inserts and deletes, and on the other hand, if it's a scenario VS Code supports gracefully, then VS should as well.

Edit
I see @ajaybhargavb called this out above.

This is currently returning 1 edit per diff even though contiguous ones can be collated. The plan is to do that as a follow up.

I'm curious as to whether this is something we anticipate being an important scenario for compatibility or collation is something that's implied/required by the protocol.

@alexdima, @aeschli does the protocol specify one way or another what is/isn't allowed?

ajaybhargavb · 2020-06-27T04:10:33Z

I'm curious as to whether this is something we anticipate being an important scenario for compatibility or collation is something that's implied/required by the protocol.

The requirement is for the edits to not overlap. Collation shouldn't be required but is definitely something we should do.

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/SyntaxTokenToSemanticTokensEditHelper.cs

ryanbrandenburg · 2020-06-29T17:41:51Z

Does VS Code handle this scenario correctly?

I believe that VSCode handles this correctly. I do intend to make Delete's and "new data" coalesce if they start at the same position, but I should point out that the algo I'm using gives zero credence to the "i % 5 == 0" barrier, so it's entirely likely (and even accidentally encountered in one of my test cases) that a single-character edit will produce multiple SemanticTokensEdits.

...src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticTokensEditsDiffer.cs

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/SyntaxTokenToSemanticTokensEditHelper.cs

src/Razor/test/testapps/ComponentApp/Components/Shared/NavMenu.razor

TanayParikh · 2020-06-29T22:52:52Z

...src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticTokensEditsDiffer.cs

+                switch (diff.Operation)
+                {
+                    case DiffEdit.Type.Delete:
+                        if (current != null &&


Possibly break out the logic in each case into a separate method? If you prefer it this way, that's fine too :)

That wouldn't be my preference, the function fits on my screen in one go, is accomplishing a single task, and has interaction with the loop ("current" or results.Add). I will admit it's pushing it though, if it got much larger I'd probably agree with you.

...ft.AspNetCore.Razor.LanguageServer.Test/Semantic/DefaultRazorSemanticTokenInfoServiceTest.cs

ajaybhargavb

🎉 . Just minor comments

ajaybhargavb · 2020-06-29T22:45:46Z

...src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticTokensEditsDiffer.cs

+using System;
+using System.Collections.Generic;
+using Microsoft.AspNetCore.Razor.LanguageServer.Semantic.Models;
+using System.Linq;


ajaybhargavb · 2020-06-29T22:51:13Z

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/DefaultRazorSemanticTokensInfoService.cs

@@ -58,7 +58,12 @@ public override SemanticTokensOrSemanticTokensEdits GetSemanticTokensEdits(Razor
            }
            var newTokens = ConvertSemanticRangesToSemanticTokens(semanticRanges, codeDocument);

-            var semanticEdits = SyntaxTokenToSemanticTokensEditHelper.ConvertSyntaxTokensToSemanticEdits(newTokens, previousResults);
+            if (previousResults is null)


Can remove this if if you move line 59 above line 50

What if I convert it to a conditional expression instead? previousResultId == null isn't the only way to get previousResults == null, it also happens if the previousResult was evicted from the cache (most likely scenario for that is if someone was working on a different document for a while).

Ah. Didn't realize _semanticTokensCache.Get(previousResultId); could return null. In case you touch it in the future, making that a TryGet would be more inline with how other cache APIs work.

ajaybhargavb · 2020-06-29T22:51:51Z

...src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticTokensEditsDiffer.cs

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


nit: newline

ajaybhargavb · 2020-06-29T22:52:13Z

...src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticTokensEditsDiffer.cs

+        {
+            if (oldArray is null)
+            {
+                throw new ArgumentNullException();


Suggested change

throw new ArgumentNullException();

throw new ArgumentNullException(nameof(oldArray));

Ditto below

ajaybhargavb · 2020-06-30T00:16:11Z

...src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticTokensEditsDiffer.cs

+            var results = new List<SemanticTokensEdit>();
+            foreach (var diff in diffs)
+            {
+                var current = results.Any() ? results.Last() : null;


I'd avoid using Linq when possible because this is a hot path,

Suggested change

var current = results.Any() ? results.Last() : null;

var current = results.Count > 0 ? results[results.Count - 1] : null;

gundermanc · 2020-06-30T01:43:10Z

Another possible bug? Doing a move lines (Alt+Up) in VS triggers a textDocument/didChange notification and an /edits request, but it looks like it comes back empty, so the transplanted line isn't colorized.

ryanbrandenburg · 2020-06-30T16:47:17Z

@gundermanc I can repro that, but I feel like I should deal with it in a separate PR so we can get this one moving. I suspect it's a syncing issue rather than a problem with the diff algo.

ryanbrandenburg added 4 commits June 26, 2020 15:53

Fix semantic diff algo

3a71582

Need it for now

af436d4

progress

f3108dc

Fix the coalesce

898219a

ryanbrandenburg requested review from ajaybhargavb, NTaylorMullen, noahbkim and TanayParikh June 27, 2020 00:14

ajaybhargavb suggested changes Jun 27, 2020

View reviewed changes

gundermanc reviewed Jun 28, 2020

View reviewed changes

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/SyntaxTokenToSemanticTokensEditHelper.cs Outdated Show resolved Hide resolved

ryanbrandenburg added 2 commits June 29, 2020 11:25

PR feedback and cleanup

3b0a132

Christian found a bug

c92117a

ajaybhargavb reviewed Jun 29, 2020

View reviewed changes

PR feedback

90ee0dc

ryanbrandenburg requested review from ajaybhargavb and gundermanc June 29, 2020 21:53

ryanbrandenburg marked this pull request as ready for review June 29, 2020 22:42

TanayParikh approved these changes Jun 29, 2020

View reviewed changes

ryanbrandenburg added 2 commits June 29, 2020 15:54

Remove unneeded cgmanifest

948398c

Clean up

441610e

gundermanc reviewed Jun 29, 2020

View reviewed changes

...ft.AspNetCore.Razor.LanguageServer.Test/Semantic/DefaultRazorSemanticTokenInfoServiceTest.cs Outdated Show resolved Hide resolved

Use Array.Empty

741578a

ajaybhargavb approved these changes Jun 30, 2020

View reviewed changes

PR feedback

2ba084c

ryanbrandenburg merged commit 7f3c4a6 into master Jun 30, 2020

ryanbrandenburg deleted the rybrande/SemanticDiffAlgo branch June 30, 2020 17:33

ryanbrandenburg mentioned this pull request Jul 10, 2020

Update Razor to include various improvments dotnet/vscode-csharp#3906

Merged

ryanbrandenburg mentioned this pull request Oct 9, 2021

VS Semantic Tokens ALT+UP behavior broken #5330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the algorithm used to calculate semantic diff #2126

Replace the algorithm used to calculate semantic diff #2126

ryanbrandenburg commented Jun 27, 2020

ryanbrandenburg commented Jun 27, 2020

gundermanc commented Jun 27, 2020

ajaybhargavb left a comment

gundermanc commented Jun 27, 2020 •

edited

gundermanc commented Jun 27, 2020 •

edited

ajaybhargavb commented Jun 27, 2020 •

edited

ryanbrandenburg commented Jun 29, 2020

TanayParikh Jun 29, 2020

ryanbrandenburg Jun 29, 2020

ajaybhargavb left a comment

ajaybhargavb Jun 29, 2020

ajaybhargavb Jun 29, 2020

ryanbrandenburg Jun 30, 2020

ajaybhargavb Jun 30, 2020

ajaybhargavb Jun 29, 2020

ajaybhargavb Jun 29, 2020

ajaybhargavb Jun 29, 2020

ajaybhargavb Jun 30, 2020

gundermanc commented Jun 30, 2020

ryanbrandenburg commented Jun 30, 2020

	throw new ArgumentNullException();
	throw new ArgumentNullException(nameof(oldArray));

	var current = results.Any() ? results.Last() : null;
	var current = results.Count > 0 ? results[results.Count - 1] : null;

Replace the algorithm used to calculate semantic diff #2126

Replace the algorithm used to calculate semantic diff #2126

Conversation

ryanbrandenburg commented Jun 27, 2020

ryanbrandenburg commented Jun 27, 2020

gundermanc commented Jun 27, 2020

ajaybhargavb left a comment

Choose a reason for hiding this comment

gundermanc commented Jun 27, 2020 • edited

gundermanc commented Jun 27, 2020 • edited

ajaybhargavb commented Jun 27, 2020 • edited

ryanbrandenburg commented Jun 29, 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

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

gundermanc commented Jun 30, 2020

ryanbrandenburg commented Jun 30, 2020

gundermanc commented Jun 27, 2020 •

edited

gundermanc commented Jun 27, 2020 •

edited

ajaybhargavb commented Jun 27, 2020 •

edited