-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from 31 commits
9deff2c
7148d87
7fac8ca
48becee
5bf93ed
5f70bc4
edc4588
67489aa
9d2e026
4bb2764
901959a
49a0641
727c00c
3edfeee
a71492e
307060f
4a33241
ea9a7e4
73d4c8b
e14f0d0
23c5089
9ad9b5d
eff6de0
a497bcd
ff54861
343d0af
5d5bdec
71d07eb
6c8c140
0e3a38c
c34bfc4
1cf970f
7e959bc
2b61433
b2d3dee
da1bef8
2f451de
0128f0e
deb93fa
53d28dc
1647dce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,5 +121,19 @@ diagnostic.Severity is DiagnosticSeverity.Warning or DiagnosticSeverity.Error && | |
return null; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// TODO: is there anything we can do better here? Inline diagnostic tags are not really data, but more UI | ||
/// elements with specific constrols, positions and events attached to them. There doesn't seem to be a safe | ||
/// way to reuse any of these currently. Ideally we could do something similar to inline-hints where there's a | ||
/// data tagger portion (which is async and has clean equality semantics), and then the UI portion which just | ||
/// translates those data-tags to the UI tags. | ||
/// <para> | ||
/// Returning false here means we'll always end up regenerating all tags. But hopefully there won't be that | ||
/// many in a document to matter. | ||
/// </para> | ||
/// </summary> | ||
protected override bool Equals(InlineDiagnosticsTag tag1, InlineDiagnosticsTag tag2) | ||
=> false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,5 +114,12 @@ protected override async Task ProduceTagsAsync( | |
context.AddTag(new TagSpan<LineSeparatorTag>(span.ToSnapshotSpan(snapshotSpan.Snapshot), tag)); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// We create and cache a separator tag to use (unless the format mapping changes). So we can just use identity | ||
/// comparisons here. | ||
/// </summary> | ||
protected override bool Equals(LineSeparatorTag tag1, LineSeparatorTag tag2) | ||
=> tag1 == tag2; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. many simple tags just need something like this. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using Microsoft.CodeAnalysis.Classification; | ||
using Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.QuickInfo; | ||
using Microsoft.VisualStudio.Text.Adornments; | ||
using Microsoft.VisualStudio.Text.Tagging; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.Diagnostics | ||
{ | ||
internal partial class AbstractDiagnosticsAdornmentTaggerProvider<TTag> | ||
{ | ||
protected sealed class RoslynErrorTag : ErrorTag, IEquatable<RoslynErrorTag> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
{ | ||
private readonly DiagnosticData _data; | ||
|
||
public RoslynErrorTag(string errorType, Workspace workspace, DiagnosticData data) | ||
: base(errorType, CreateToolTipContent(workspace, data)) | ||
{ | ||
_data = data; | ||
} | ||
|
||
private static object CreateToolTipContent(Workspace workspace, DiagnosticData diagnostic) | ||
{ | ||
Action? navigationAction = null; | ||
string? tooltip = null; | ||
if (workspace != null) | ||
{ | ||
var helpLinkUri = diagnostic.GetValidHelpLinkUri(); | ||
if (helpLinkUri != null) | ||
{ | ||
navigationAction = new QuickInfoHyperLink(workspace, helpLinkUri).NavigationAction; | ||
tooltip = diagnostic.HelpLink; | ||
} | ||
} | ||
|
||
var diagnosticIdTextRun = navigationAction is null | ||
? new ClassifiedTextRun(ClassificationTypeNames.Text, diagnostic.Id) | ||
: new ClassifiedTextRun(ClassificationTypeNames.Text, diagnostic.Id, navigationAction, tooltip); | ||
|
||
return new ContainerElement( | ||
ContainerElementStyle.Wrapped, | ||
new ClassifiedTextElement( | ||
diagnosticIdTextRun, | ||
new ClassifiedTextRun(ClassificationTypeNames.Punctuation, ":"), | ||
new ClassifiedTextRun(ClassificationTypeNames.WhiteSpace, " "), | ||
new ClassifiedTextRun(ClassificationTypeNames.Text, diagnostic.Message))); | ||
} | ||
|
||
public override bool Equals(object? obj) | ||
=> Equals(obj as RoslynErrorTag); | ||
|
||
public bool Equals(RoslynErrorTag? other) | ||
{ | ||
return other != null && | ||
this.ErrorType == other.ErrorType && | ||
this._data.GetValidHelpLinkUri() == other._data.GetValidHelpLinkUri() && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
this._data.Id == other._data.Id && | ||
this._data.Message == other._data.Message; | ||
} | ||
|
||
public override int GetHashCode() | ||
=> throw ExceptionUtilities.Unreachable(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
using Microsoft.CodeAnalysis.Options; | ||
using Microsoft.CodeAnalysis.Shared.TestHooks; | ||
using Microsoft.CodeAnalysis.Workspaces; | ||
using Microsoft.VisualStudio.Text; | ||
using Microsoft.VisualStudio.Text.Adornments; | ||
using Microsoft.VisualStudio.Text.Tagging; | ||
using Microsoft.VisualStudio.Utilities; | ||
|
@@ -73,7 +74,7 @@ protected internal override bool IncludeDiagnostic(DiagnosticData diagnostic) | |
return null; | ||
} | ||
|
||
return new ErrorTag(errorType, CreateToolTipContent(workspace, diagnostic)); | ||
return new RoslynErrorTag(errorType, workspace, diagnostic); | ||
} | ||
|
||
private static string? GetErrorTypeFromDiagnostic(DiagnosticData diagnostic) | ||
|
@@ -126,5 +127,12 @@ protected internal override bool IncludeDiagnostic(DiagnosticData diagnostic) | |
return PredefinedErrorTypeNames.OtherError; | ||
} | ||
} | ||
|
||
protected override bool Equals(IErrorTag tag1, IErrorTag tag2) | ||
{ | ||
return tag1 is RoslynErrorTag errorTag1 && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. that seems reasonable. |
||
tag2 is RoslynErrorTag errorTag2 && | ||
errorTag1.Equals(errorTag2); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akhera99?
There was a problem hiding this comment.
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.