Implement equality semantics for async tagger tags. by CyrusNajmabadi · Pull Request #64802 · dotnet/roslyn

CyrusNajmabadi · 2022-10-18T19:14:28Z

This is necessary so that we can properly diff old/new tags and tell the editor the minimal change made.

CyrusNajmabadi · 2022-10-18T20:02:01Z

src/EditorFeatures/Core.Wpf/InlineDiagnostics/InlineDiagnosticsTaggerProvider.cs

+        /// </para>
+        /// </summary>
+        protected override bool Equals(InlineDiagnosticsTag tag1, InlineDiagnosticsTag tag2)
+            => false;


CyrusNajmabadi · 2022-10-18T20:02:18Z

src/EditorFeatures/Core.Wpf/LineSeparators/LineSeparatorTaggerProvider.cs

+        /// comparisons here.
+        /// </summary>
+        protected override bool Equals(LineSeparatorTag tag1, LineSeparatorTag tag2)
+            => tag1 == tag2;


many simple tags just need something like this.

CyrusNajmabadi · 2022-10-18T20:02:39Z

src/EditorFeatures/Core.Wpf/StringIndentation/StringIndentationTag.cs

+            // Map span1 over to span2
+            var span1 = snapshotSpan1.TranslateTo(snapshotSpan2.Snapshot, _provider.SpanTrackingMode);
+            return span1.Span == snapshotSpan2.Span;
+        }


more complex tags have to normalize themselves to do the equality comparison properly.

CyrusNajmabadi · 2022-10-18T20:03:26Z

...EditorFeatures/Core/Diagnostics/AbstractDiagnosticsAdornmentTaggerProvider.RoslynErrorTag.cs

+{
+    internal partial class AbstractDiagnosticsAdornmentTaggerProvider<TTag>
+    {
+        protected sealed class RoslynErrorTag : ErrorTag, IEquatable<RoslynErrorTag>


by creating our own subclass, we can hold onto the data we used to create the actual ErrorTag. we can then use that data to compare old/new tags.

CyrusNajmabadi · 2022-10-18T20:04:27Z

...EditorFeatures/Core/Diagnostics/AbstractDiagnosticsAdornmentTaggerProvider.RoslynErrorTag.cs

+            }
+
+            public override int GetHashCode()
+                => throw ExceptionUtilities.Unreachable();


while we implement Equals, we have no expectation that we will hash tags themselves. i explicitly made our tags throw so we catch if anything tries to do that.

src/EditorFeatures/Core/EditAndContinue/ActiveStatementTaggerProvider.cs

…rovider.cs

CyrusNajmabadi · 2022-10-18T20:05:21Z

src/EditorFeatures/Core/InlineHints/InlineHintDataTag.cs

+        /// <summary>
+        /// The snapshot this tag was created against.
+        /// </summary>
+        public readonly ITextSnapshot Snapshot;


needed to add this so we can compare tags created against different snapshots.

CyrusNajmabadi · 2022-10-18T20:21:06Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

-                return oldTagTree.GetSpans(snapshot).Except(tagSpansToInvalidate, comparer: this);
+                return oldTagTree.GetSpans(snapshot).Except(
+                    spansToInvalidate.SelectMany(oldTagTree.GetIntersectingSpans),
+                    comparer: this);


same as before, just no intermediary list computation.

CyrusNajmabadi · 2022-10-18T20:25:27Z

src/Features/Core/Portable/InheritanceMargin/InheritanceMarginItem.cs

+            => obj is InheritanceMarginItem item && Equals(item);
+
+        public bool Equals(InheritanceMarginItem other)
+            => this.LineNumber == other.LineNumber &&


@Cosifne It would be good to not have LineNumber be part of hte data side of this. It is positional, and shouldn't cause us to teardown/recreate tags. In other words, if a member moves down one line, we should still be able to use its existing tags insteading of having to remove hte old one and create a new one.

teh general pattern for this is to have ITagSpan in the tagger store the position, and the ITag itself it position-free.

Should we get a bug tracking this?

jasonmalinowski

Approach seems generally fine but I do wish we'd just push the editor's tags for implement equality semantics than us doing it ourselves. Seems like it'd be better for other teams (C++, etc.) and also means if the editor ever adds additional properties we don't have to update, etc.

jasonmalinowski · 2022-10-18T20:40:09Z

src/EditorFeatures/Core/Structure/StructureTag.cs

+        public override bool Equals(object? obj)
+            => Equals(obj as StructureTag);
+
+        public bool Equals(StructureTag? other)


Can we ask the editor to implement equals on their side?

jasonmalinowski · 2022-10-18T20:42:13Z

...ditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_IEqualityComparer.cs

-            public bool Equals(ITagSpan<TTag> x, ITagSpan<TTag> y)
-                => x.Span == y.Span && EqualityComparer<TTag>.Default.Equals(x.Tag, y.Tag);
+            public bool Equals(ITagSpan<TTag>? x, ITagSpan<TTag>? y)
+                => x != null && y != null && x.Span == y.Span && _dataSource.Equals(x.Tag, y.Tag);


Why not just have the underlying tab objects implement equality here? I get that'll require some editor team work, but it seems like that scales across multiple teams (C++ doesn't have to reimplement this).

i found that too easy to miss. this way (being abstract) forced all impls to have to consider what to do here. :-/

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.cs

src/Features/Core/Portable/Common/TaggedText.cs

jasonmalinowski · 2022-10-18T21:04:51Z

src/Features/Core/Portable/InheritanceMargin/InheritanceMarginItem.cs

+            => obj is InheritanceMarginItem item && Equals(item);
+
+        public bool Equals(InheritanceMarginItem other)
+            => this.LineNumber == other.LineNumber &&


Should we get a bug tracking this?

jasonmalinowski · 2022-10-18T21:34:45Z

src/EditorFeatures/Core/InlineHints/InlineHintDataTag.cs

+            if (this.Hint.ReplacementTextChange != null &&
+                other.Hint.ReplacementTextChange != null &&
+                !_provider.SpanEquals(_snapshot, this.Hint.ReplacementTextChange.Value.Span, other._snapshot, other.Hint.ReplacementTextChange.Value.Span))


Should this be grouped with the other replacement text change checks, or was this intentional to sort cheapest to most expensive?

intentional for that reason.

src/EditorFeatures/Core/InlineHints/InlineHintDataTag.cs

jasonmalinowski · 2022-10-18T21:36:26Z

src/EditorFeatures/Core/Diagnostics/DiagnosticsSquiggleTaggerProvider.cs

+
+        protected override bool Equals(IErrorTag tag1, IErrorTag tag2)
+        {
+            return tag1 is RoslynErrorTag errorTag1 &&


Should this do a hard cast, since we don't expect error tags to not be a RoslynErrorTag?

yes. that seems reasonable.

jasonmalinowski · 2022-10-18T21:38:45Z

...EditorFeatures/Core/Diagnostics/AbstractDiagnosticsAdornmentTaggerProvider.RoslynErrorTag.cs

+            {
+                return other != null &&
+                    this.ErrorType == other.ErrorType &&
+                    this._data.GetValidHelpLinkUri() == other._data.GetValidHelpLinkUri() &&


Is this expensive? I see there's some URI parsing but if the only input is looks to be the descriptor's HelpLinkUri can we just compare that?

my goal was to have this match whatever we do when creating the actual final tag content. but i can def look into seeing about making all of them cheaper/conssitent

jasonmalinowski · 2022-10-18T21:47:14Z

src/EditorFeatures/Core.Wpf/InlineDiagnostics/InlineDiagnosticsTaggerProvider.cs

        }
+
+        /// <summary>
+        /// TODO: is there anything we can do better here? Inline diagnostic tags are not really data, but more UI


I think what the comment says makes sense, separating the data tagging and creating UI tags from the data tags. I would be happy to change that in a follow-up PR.

genlu · 2022-10-20T18:29:20Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.cs

        /// tagging architecture.  This value cannot be <see cref="SpanTrackingMode.Custom"/>.
        /// </summary>
-        protected virtual SpanTrackingMode SpanTrackingMode => SpanTrackingMode.EdgeExclusive;
+        public virtual SpanTrackingMode SpanTrackingMode => SpanTrackingMode.EdgeExclusive;


This is a breaking change for TS
http://index/?query=Microsoft.CodeAnalysis.Editor.TypeScript.Features.Classifier.SemanticClassificationTaggerProvider&rightProject=Microsoft.CodeAnalysis.TypeScript.EditorFeatures&file=Features%5CClassifier%5CSemanticClassificationTaggerProvider.cs&line=43

fixed in followup.

genlu · 2022-10-20T18:45:15Z

src/EditorFeatures/Core/Structure/StructureTag.cs

        public bool IsImplementation { get; }

+        public override int GetHashCode()
+            => throw ExceptionUtilities.Unreachable();


This is called in editor

https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/VS-Platform?path=/src/Editor/Text/Impl/Structure/StructureSpanningTree/StructureSpanningTree.cs&version=GBmain&line=308&lineEnd=309&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

fixed in followup.

CyrusNajmabadi added 11 commits October 18, 2022 11:24

Add tag equality comparers

9deff2c

Add tag equality comparers

7148d87

Add tag equality comparers

7fac8ca

Add tag equality comparers

48becee

In progress

5bf93ed

Switch to simpler model

5f70bc4

Switch to simpler model

edc4588

Switch to simpler model

67489aa

Fix

9d2e026

In progress

4bb2764

Testing

901959a

ghost added the Area-IDE label Oct 18, 2022

CyrusNajmabadi added 9 commits October 18, 2022 12:18

Add back comparer

49a0641

simplify

727c00c

simplify

3edfeee

in progress

a71492e

Add equality

307060f

Continued equality

4a33241

Doc

ea9a7e4

Doc

73d4c8b

End to end

e14f0d0

CyrusNajmabadi changed the title ~~WIP~~ Implemen equality semantics for async tagger tags. Oct 18, 2022

CyrusNajmabadi commented Oct 18, 2022

View reviewed changes

src/EditorFeatures/Core/EditAndContinue/ActiveStatementTaggerProvider.cs Outdated Show resolved Hide resolved

Update src/EditorFeatures/Core/EditAndContinue/ActiveStatementTaggerP…

23c5089

…rovider.cs

CyrusNajmabadi commented Oct 18, 2022

View reviewed changes

OBsolete

c34bfc4

CyrusNajmabadi commented Oct 18, 2022

View reviewed changes

CyrusNajmabadi requested a review from jasonmalinowski October 18, 2022 20:25

CyrusNajmabadi marked this pull request as ready for review October 18, 2022 20:25

CyrusNajmabadi requested a review from a team as a code owner October 18, 2022 20:25

CyrusNajmabadi requested a review from a team October 18, 2022 20:25

jasonmalinowski approved these changes Oct 18, 2022

View reviewed changes

CyrusNajmabadi added 8 commits October 18, 2022 16:04

Rename

1cf970f

Doc

7e959bc

Use a record instead

2b61433

Use a record instead

b2d3dee

Add simplifying assumptions

da1bef8

Add simplifying assumptions

2f451de

Renames

0128f0e

rename

deb93fa

CyrusNajmabadi enabled auto-merge October 18, 2022 23:18

build-analysis bot mentioned this pull request Oct 19, 2022

Nuget restore error caused by 503s dotnet/arcade#11239

Closed

2 tasks

CyrusNajmabadi changed the title ~~Implemen equality semantics for async tagger tags.~~ Implement equality semantics for async tagger tags. Oct 19, 2022

CyrusNajmabadi added 2 commits October 18, 2022 23:01

Merge remote-tracking branch 'upstream/main' into tagEquality

53d28dc

Fix

1647dce

CyrusNajmabadi merged commit c9118d3 into dotnet:main Oct 19, 2022

ghost added this to the Next milestone Oct 19, 2022

CyrusNajmabadi deleted the tagEquality branch October 19, 2022 20:19

genlu reviewed Oct 20, 2022

View reviewed changes

genlu mentioned this pull request Oct 20, 2022

Revert "Implement equality semantics for async tagger tags." #64867

Merged

genlu reviewed Oct 20, 2022

View reviewed changes

allisonchou mentioned this pull request Oct 22, 2022

[Automated] PRs inserted in VS build main-33021.259 #64913

Closed

RikkiGibson modified the milestones: Next, 17.5 P1 Oct 24, 2022

Conversation

CyrusNajmabadi commented Oct 18, 2022 • 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!

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!

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!

jasonmalinowski 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!

Uh oh!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CyrusNajmabadi commented Oct 18, 2022 •

edited

Loading