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

added ability to clear all diagnostics reproted from a IDiagnosticUpd… #33805

Merged
merged 2 commits into from
Mar 2, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public EditAndContinueDiagnosticUpdateSource(IDiagnosticUpdateSourceRegistration
public bool SupportGetDiagnostics => false;

public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;
public event EventHandler Cleared { add { } remove { } }

public ImmutableArray<DiagnosticData> GetDiagnostics(Workspace workspace, ProjectId projectId, DocumentId documentId, object id, bool includeSuppressedDiagnostics = false, CancellationToken cancellationToken = default)
{
Expand Down
68 changes: 68 additions & 0 deletions src/EditorFeatures/Test/Diagnostics/DiagnosticServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,68 @@ public void TestGetDiagnostics2()
}
}

[Fact, Trait(Traits.Feature, Traits.Features.Diagnostics)]
public void TestCleared()
{
using (var workspace = new TestWorkspace(TestExportProvider.ExportProviderWithCSharpAndVisualBasic))
{
var set = new ManualResetEvent(false);
Copy link
Member

Choose a reason for hiding this comment

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

set [](start = 20, length = 3)

set is an odd name for an event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so.. wait handle then?

var document = workspace.CurrentSolution.AddProject("TestProject", "TestProject", LanguageNames.CSharp).AddDocument("TestDocument", string.Empty);
var document2 = document.Project.AddDocument("TestDocument2", string.Empty);

var diagnosticService = new DiagnosticService(AsynchronousOperationListenerProvider.NullProvider);

var source1 = new TestDiagnosticUpdateSource(support: false, diagnosticData: null);
diagnosticService.Register(source1);

var source2 = new TestDiagnosticUpdateSource(support: false, diagnosticData: null);
diagnosticService.Register(source2);

diagnosticService.DiagnosticsUpdated += MarkSet;

// add bunch of data to the service for both sources
RaiseDiagnosticEvent(set, source1, workspace, document.Project.Id, document.Id, Tuple.Create(workspace, document));
RaiseDiagnosticEvent(set, source1, workspace, document.Project.Id, document.Id, Tuple.Create(workspace, document.Project, document));
RaiseDiagnosticEvent(set, source1, workspace, document2.Project.Id, document2.Id, Tuple.Create(workspace, document2));

RaiseDiagnosticEvent(set, source2, workspace, document.Project.Id, null, Tuple.Create(workspace, document.Project));
RaiseDiagnosticEvent(set, source2, workspace, null, null, Tuple.Create(workspace));

// confirm data is there.
var data1 = diagnosticService.GetDiagnostics(workspace, null, null, null, false, CancellationToken.None);
Assert.Equal(5, data1.Count());

diagnosticService.DiagnosticsUpdated -= MarkSet;

// confirm clear for a source
set.Reset();
var count = 0;
diagnosticService.DiagnosticsUpdated += MarkCalled;

source1.RaiseClearedEvent();

set.WaitOne();

// confirm there are 2 data left
var data2 = diagnosticService.GetDiagnostics(workspace, null, null, null, false, CancellationToken.None);
Assert.Equal(2, data2.Count());

void MarkCalled(object sender, DiagnosticsUpdatedArgs args)
{
// event is serialized. no concurrent call
if (++count == 3)
{
set.Set();
}
}

void MarkSet(object sender, DiagnosticsUpdatedArgs args)
{
set.Set();
}
}
}

private static DiagnosticData RaiseDiagnosticEvent(ManualResetEvent set, TestDiagnosticUpdateSource source, TestWorkspace workspace, ProjectId project, DocumentId document, object id)
{
set.Reset();
Expand Down Expand Up @@ -126,6 +188,7 @@ public TestDiagnosticUpdateSource(bool support, DiagnosticData[] diagnosticData)

public bool SupportGetDiagnostics { get { return _support; } }
public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;
public event EventHandler Cleared;

public ImmutableArray<DiagnosticData> GetDiagnostics(Workspace workspace, ProjectId projectId, DocumentId documentId, object id, bool includeSuppressedDiagnostics = false, CancellationToken cancellationToken = default)
{
Expand All @@ -136,6 +199,11 @@ public void RaiseUpdateEvent(DiagnosticsUpdatedArgs args)
{
DiagnosticsUpdated?.Invoke(this, args);
}

public void RaiseClearedEvent()
{
Cleared?.Invoke(this, EventArgs.Empty);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public void RaiseDiagnosticsUpdated(DiagnosticsUpdatedArgs args)
}

public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;
public event EventHandler Cleared { add { } remove { } }

public bool SupportGetDiagnostics => false;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public ImmutableArray<DiagnosticData> GetDiagnostics(Workspace workspace, Projec
}

public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;
public event EventHandler Cleared { add { } remove { } }

public void RaiseDiagnosticsUpdated(DiagnosticsUpdatedArgs args)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public IIncrementalAnalyzer CreateIncrementalAnalyzer(Workspace workspace)
}

public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;
public event EventHandler Cleared { add { } remove { } }

// this only support push model, pull model will be provided by DiagnosticService by caching everything this one pushed
public bool SupportGetDiagnostics => false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,19 @@ public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated
}
}

public event EventHandler Cleared
{
add
{
// don't do anything. this update source doesn't use cleared event
}

remove
{
// don't do anything. this update source doesn't use cleared event
}
}

internal void RaiseDiagnosticsUpdated(DiagnosticsUpdatedArgs args)
{
// all diagnostics events are serialized.
Expand Down
67 changes: 67 additions & 0 deletions src/Features/Core/Portable/Diagnostics/DiagnosticService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,37 @@ private void RaiseDiagnosticsUpdated(object sender, DiagnosticsUpdatedArgs args)
}).CompletesAsyncOperation(eventToken);
}

private void RaiseDiagnosticsCleared(object sender)
{
Contract.ThrowIfNull(sender);
var source = (IDiagnosticUpdateSource)sender;
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this to the caller and strongly type the parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.


var ev = _eventMap.GetEventHandlers<EventHandler<DiagnosticsUpdatedArgs>>(DiagnosticsUpdatedEventName);
if (!RequireRunningEventTasks(source, ev))
{
return;
}

var eventToken = _listener.BeginAsyncOperation(DiagnosticsUpdatedEventName);
_eventQueue.ScheduleTask(() =>
{
using (var pooledObject = SharedPools.Default<List<DiagnosticsUpdatedArgs>>().GetPooledObject())
{
var removed = pooledObject.Object;
if (!ClearDiagnosticsFromDataMapFor(source, removed))
{
// there is no change, nothing to raise events for.
return;
}

foreach (var args in removed)
{
ev.RaiseEvent(handler => handler(sender, args));
}
}
}).CompletesAsyncOperation(eventToken);
}

private bool RequireRunningEventTasks(
IDiagnosticUpdateSource source, EventMap.EventHandlerSet<EventHandler<DiagnosticsUpdatedArgs>> ev)
{
Expand Down Expand Up @@ -150,12 +181,48 @@ private bool UpdateDataMap(IDiagnosticUpdateSource source, DiagnosticsUpdatedArg
}
}

private bool ClearDiagnosticsFromDataMapFor(IDiagnosticUpdateSource source, List<DiagnosticsUpdatedArgs> removed)
Copy link
Member

Choose a reason for hiding this comment

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

ClearDiagnosticsFromDataMapFor [](start = 21, length = 30)

Wouldn't ClearDiagnosticsReportedBySource be better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

{
// we expect source who uses this ability to have small number of diagnostics.
lock (_gate)
{
Debug.Assert(_updateSources.Contains(source));

// 2 different workspaces (ex, PreviewWorkspaces) can return same Args.Id, we need to
// distinguish them. so we separate diagnostics per workspace map.
if (!_map.TryGetValue(source, out var workspaceMap))
{
return false;
}

foreach (var (workspace, map) in workspaceMap)
{
foreach (var (id, data) in map)
{
removed.Add(DiagnosticsUpdatedArgs.DiagnosticsRemoved(id, data.Workspace, solution: null, data.ProjectId, data.DocumentId));
}
}

// all diagnostics from the source is cleared
_map.Remove(source);
return true;
}
}

private void OnDiagnosticsUpdated(object sender, DiagnosticsUpdatedArgs e)
{
AssertIfNull(e.Diagnostics);

// all events are serialized by async event handler
RaiseDiagnosticsUpdated(sender, e);
}

private void OnCleared(object sender, EventArgs e)
{
// all events are serialized by async event handler
RaiseDiagnosticsCleared(sender);
}

public IEnumerable<DiagnosticData> GetDiagnostics(
Workspace workspace, ProjectId projectId, DocumentId documentId, object id, bool includeSuppressedDiagnostics, CancellationToken cancellationToken)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ public void Register(IDiagnosticUpdateSource source)
}

_updateSources = _updateSources.Add(source);

source.DiagnosticsUpdated += OnDiagnosticsUpdated;
source.Cleared += OnCleared;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ internal interface IDiagnosticUpdateSource
/// </summary>
event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;

/// <summary>
/// Raise this when all diagnostics reported from this update source has cleared
/// </summary>
event EventHandler Cleared;

/// <summary>
/// Return true if the source supports GetDiagnostics API otherwise, return false so that the engine can cache data from DiagnosticsUpdated in memory
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ internal ExternalErrorDiagnosticUpdateSource(

public event EventHandler<bool> BuildStarted;
public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;
public event EventHandler Cleared { add { } remove { } }

public bool IsInProgress => BuildInprogressState != null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.Diagnostics
End Property

Public Event DiagnosticsUpdated As EventHandler(Of DiagnosticsUpdatedArgs) Implements IDiagnosticUpdateSource.DiagnosticsUpdated
Public Event Cleared As EventHandler Implements IDiagnosticUpdateSource.Cleared

Public Function GetDiagnostics(workspace As Microsoft.CodeAnalysis.Workspace, projectId As ProjectId, documentId As DocumentId, id As Object, includeSuppressedDiagnostics As Boolean, cancellationToken As CancellationToken) As ImmutableArray(Of DiagnosticData) Implements IDiagnosticUpdateSource.GetDiagnostics
Return If(includeSuppressedDiagnostics, _data, _data.WhereAsArray(Function(d) Not d.IsSuppressed))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.EditAndContinue
End Property

Public Event DiagnosticsUpdated As EventHandler(Of DiagnosticsUpdatedArgs) Implements IDiagnosticUpdateSource.DiagnosticsUpdated
Public Event Cleared As EventHandler Implements IDiagnosticUpdateSource.Cleared

Public Function GetDiagnostics(workspace As Microsoft.CodeAnalysis.Workspace, projectId As ProjectId, documentId As DocumentId, id As Object, includeSuppressedDiagnostics As Boolean, cancellationToken As CancellationToken) As ImmutableArray(Of DiagnosticData) Implements IDiagnosticUpdateSource.GetDiagnostics
Return If(includeSuppressedDiagnostics, _data, _data.WhereAsArray(Function(d) Not d.IsSuppressed))
Expand Down
1 change: 0 additions & 1 deletion src/Workspaces/Core/Portable/Diagnostics/DiagnosticData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,6 @@ private static ImmutableDictionary<string, string> GetProperties(
return additionalProperties == null
? properties
: properties.AddRange(additionalProperties);
throw new NotImplementedException();
}

/// <summary>
Expand Down