Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement equality semantics for async tagger tags. #64802

Merged
merged 41 commits into from
Oct 19, 2022

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Oct 18, 2022

Fixes #64797

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

@CyrusNajmabadi CyrusNajmabadi changed the title WIP Implemen equality semantics for async tagger tags. Oct 18, 2022
/// </para>
/// </summary>
protected override bool Equals(InlineDiagnosticsTag tag1, InlineDiagnosticsTag tag2)
=> false;
Copy link
Member Author

Choose a reason for hiding this comment

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

/// comparisons here.
/// </summary>
protected override bool Equals(LineSeparatorTag tag1, LineSeparatorTag tag2)
=> tag1 == tag2;
Copy link
Member Author

Choose a reason for hiding this comment

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

many simple tags just need something like this.

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

Choose a reason for hiding this comment

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

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

{
internal partial class AbstractDiagnosticsAdornmentTaggerProvider<TTag>
{
protected sealed class RoslynErrorTag : ErrorTag, IEquatable<RoslynErrorTag>
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}

public override int GetHashCode()
=> throw ExceptionUtilities.Unreachable();
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

/// <summary>
/// The snapshot this tag was created against.
/// </summary>
public readonly ITextSnapshot Snapshot;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

return oldTagTree.GetSpans(snapshot).Except(tagSpansToInvalidate, comparer: this);
return oldTagTree.GetSpans(snapshot).Except(
spansToInvalidate.SelectMany(oldTagTree.GetIntersectingSpans),
comparer: this);
Copy link
Member Author

Choose a reason for hiding this comment

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

same as before, just no intermediary list computation.

=> obj is InheritanceMarginItem item && Equals(item);

public bool Equals(InheritanceMarginItem other)
=> this.LineNumber == other.LineNumber &&
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

Should we get a bug tracking this?

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review October 18, 2022 20:25
@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners October 18, 2022 20:25
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

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.

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

public bool Equals(StructureTag? other)
Copy link
Member

Choose a reason for hiding this comment

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

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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

src/Features/Core/Portable/Common/TaggedText.cs Outdated Show resolved Hide resolved
=> obj is InheritanceMarginItem item && Equals(item);

public bool Equals(InheritanceMarginItem other)
=> this.LineNumber == other.LineNumber &&
Copy link
Member

Choose a reason for hiding this comment

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

Should we get a bug tracking this?

Comment on lines +60 to +62
if (this.Hint.ReplacementTextChange != null &&
other.Hint.ReplacementTextChange != null &&
!_provider.SpanEquals(_snapshot, this.Hint.ReplacementTextChange.Value.Span, other._snapshot, other.Hint.ReplacementTextChange.Value.Span))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

intentional for that reason.


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. that seems reasonable.

{
return other != null &&
this.ErrorType == other.ErrorType &&
this._data.GetValidHelpLinkUri() == other._data.GetValidHelpLinkUri() &&
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -121,5 +121,19 @@ protected internal override bool IncludeDiagnostic(DiagnosticData diagnostic)
return null;
}
}

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

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

@CyrusNajmabadi CyrusNajmabadi changed the title Implemen equality semantics for async tagger tags. Implement equality semantics for async tagger tags. Oct 19, 2022
@CyrusNajmabadi CyrusNajmabadi merged commit c9118d3 into dotnet:main Oct 19, 2022
@ghost ghost added this to the Next milestone Oct 19, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the tagEquality branch October 19, 2022 20:19
@@ -58,7 +59,7 @@ internal abstract partial class AbstractAsynchronousTaggerProvider<TTag> where T
/// created for a previous version of a document that are mapped forward by the async
/// tagging architecture. This value cannot be <see cref="SpanTrackingMode.Custom"/>.
/// </summary>
protected virtual SpanTrackingMode SpanTrackingMode => SpanTrackingMode.EdgeExclusive;
public virtual SpanTrackingMode SpanTrackingMode => SpanTrackingMode.EdgeExclusive;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in followup.

@@ -59,6 +61,25 @@ public StructureTag(AbstractStructureTaggerProvider tagProvider, BlockSpan block
public bool IsDefaultCollapsed { get; }
public bool IsImplementation { get; }

public override int GetHashCode()
=> throw ExceptionUtilities.Unreachable();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in followup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async taggers must override equality checks when comparing tags.
5 participants