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.

Sign up for GitHub

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

Reenable LSP pull diagnostics #48328

Merged
merged 41 commits into from
Oct 20, 2020
Merged

Reenable LSP pull diagnostics #48328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
1cadaee
Revert "Revert "Merge pull request #47914 from CyrusNajmabadi/lspDiag…
CyrusNajmabadi Oct 5, 2020
a7c6236
Merge remote-tracking branch 'upstream/master' into lspDiagnostics2
CyrusNajmabadi Oct 8, 2020
af83f65
Merge remote-tracking branch 'upstream/master' into lspDiagnostics2
CyrusNajmabadi Oct 13, 2020
55cb2e1
Remove unnecessary fields/methods for lsp push diagnostics
CyrusNajmabadi Oct 14, 2020
88bfa0d
Remove unnecessary fields/methods for lsp push diagnostics
CyrusNajmabadi Oct 14, 2020
b35167c
Add experimental flag
CyrusNajmabadi Oct 14, 2020
54502f5
Remove
CyrusNajmabadi Oct 15, 2020
0b0cbf0
Add option to switch between push/pull diagnostics.
CyrusNajmabadi Oct 15, 2020
e4e5f89
Reanme
CyrusNajmabadi Oct 15, 2020
650e11a
add ocmments
CyrusNajmabadi Oct 15, 2020
b25608c
Add tests
CyrusNajmabadi Oct 15, 2020
8ea42bd
UI option
CyrusNajmabadi Oct 15, 2020
fef7c2a
Remove test accessor
CyrusNajmabadi Oct 15, 2020
07d1dca
Cleanup usings
CyrusNajmabadi Oct 15, 2020
1b1ca2c
Merge remote-tracking branch 'upstream/master' into lspDiagnostics2
CyrusNajmabadi Oct 18, 2020
d9d7e82
Remove using
CyrusNajmabadi Oct 18, 2020
5e334c1
Add language client for diagnostics
CyrusNajmabadi Oct 18, 2020
49542f0
Merge remote-tracking branch 'upstream/master' into lspDiagnostics2
CyrusNajmabadi Oct 18, 2020
90bef7c
Make non-optional
CyrusNajmabadi Oct 18, 2020
4737fcd
Add comments
CyrusNajmabadi Oct 18, 2020
c1fdd37
Revert
CyrusNajmabadi Oct 18, 2020
fbe9c65
Add an abstraction for either reporting an array or results, or strea…
CyrusNajmabadi Oct 18, 2020
7cba654
Make threadsafe
CyrusNajmabadi Oct 19, 2020
67111a1
Remove #nullable enable
CyrusNajmabadi Oct 19, 2020
f422ad5
Docs
CyrusNajmabadi Oct 19, 2020
162fd21
Merge branch 'bufferedProgress' into lspDiagnostics2
CyrusNajmabadi Oct 19, 2020
455a733
Tweak the progress type to better support arrays and scalars.
CyrusNajmabadi Oct 19, 2020
c00d729
Merge branch 'bufferedProgress' into lspDiagnostics2
CyrusNajmabadi Oct 19, 2020
b084ea7
Merge branch 'extractMore' into lspDiagnostics2
CyrusNajmabadi Oct 19, 2020
21d4761
Merge remote-tracking branch 'upstream/master' into lspDiagnostics2
CyrusNajmabadi Oct 19, 2020
a0ed107
Remove
CyrusNajmabadi Oct 19, 2020
0a78828
Update
CyrusNajmabadi Oct 19, 2020
9a82574
Simplify
CyrusNajmabadi Oct 19, 2020
573e342
Fixup tests
CyrusNajmabadi Oct 19, 2020
2338c37
Merge remote-tracking branch 'upstream/master' into lspDiagnostics2
CyrusNajmabadi Oct 19, 2020
06b8ea0
Update src/Features/Core/Portable/Diagnostics/DiagnosticsUpdatedArgs.cs
CyrusNajmabadi Oct 19, 2020
07bbaae
Fix test
CyrusNajmabadi Oct 19, 2020
130c913
Merge branch 'lspDiagnostics2' of https://github.com/CyrusNajmabadi/r…
CyrusNajmabadi Oct 19, 2020
3c7fdb6
Fix test
CyrusNajmabadi Oct 19, 2020
c90b3ac
Fix test
CyrusNajmabadi Oct 19, 2020
5e0ee01
only enable diagnostics based on option
CyrusNajmabadi Oct 19, 2020
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
5 changes: 3 additions & 2 deletions src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ private void ProduceTags(TaggerContext<TTag> context, DocumentSnapshotSpan spanT
buffer?.Properties.TryGetProperty(PredefinedPreviewTaggerKeys.SuppressDiagnosticsSpansKey, out suppressedDiagnosticsSpans);

var buckets = _diagnosticService.GetDiagnosticBuckets(
workspace, document.Project.Id, document.Id, context.CancellationToken);
workspace, document.Project.Id, document.Id, forPullDiagnostics: false, context.CancellationToken);
Copy link
Member Author

Choose a reason for hiding this comment

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

an intermediary concept this PR introduces is that the two major clients of the diagnostics subsystem now pass in what behavior they want when calling into the DiagnosticService singleton.

i.e. the normal push features call in saying forPullDiagnostics: false and the lsp system calls in saying forPullDiagnostics: true. Internally we see which mode we're actually in and we either pass along the diagnostics, or pass along empty. in essence only one side will work, and the other will see absolutely no diagnostics ever.

This is not pretty, but it was:

  1. the cleanest way i could find to keep both systems around
  2. required minimal and mechanical changes to both systems.
  3. ensured that every entrypoint was guarded so diagnostics wouldn't accidentally sneak into the wrong client.

Once we are entirely on pull, all the push components can go away (as can tehse booleans).


foreach (var bucket in buckets)
{
Expand All @@ -171,7 +171,8 @@ private void ProduceTags(
{
var id = bucket.Id;
var diagnostics = _diagnosticService.GetDiagnostics(
workspace, document.Project.Id, document.Id, id, includeSuppressedDiagnostics: false, cancellationToken);
workspace, document.Project.Id, document.Id, id,
includeSuppressedDiagnostics: false, forPullDiagnostics: false, cancellationToken);

var isLiveUpdate = id is ISupportLiveUpdate;

Expand Down
48 changes: 32 additions & 16 deletions src/EditorFeatures/Test/Diagnostics/DiagnosticAnalyzerServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ public async Task TestHasSuccessfullyLoadedBeingFalse()

// listen to events
// check empty since this could be called to clear up existing diagnostics
service.DiagnosticsUpdated += (s, a) => Assert.Empty(a.Diagnostics);
service.DiagnosticsUpdated += (s, a) =>
{
var diagnostics = a.GetDiagnostics(workspace, forPullDiagnostics: false);
Assert.Empty(diagnostics);
};

// now call each analyze method. none of them should run.
await RunAllAnalysisAsync(analyzer, document).ConfigureAwait(false);
Expand Down Expand Up @@ -188,7 +192,8 @@ public async Task TestDisabledByDefaultAnalyzerEnabledWithEditorConfig(bool enab
var compilationDiagnostic = false;
service.DiagnosticsUpdated += (s, a) =>
{
var diagnostic = Assert.Single(a.Diagnostics);
var diagnostics = a.GetDiagnostics(workspace, forPullDiagnostics: false);
var diagnostic = Assert.Single(diagnostics);
Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity);

if (diagnostic.Id == DisabledByDefaultAnalyzer.s_syntaxRule.Id)
Expand Down Expand Up @@ -236,7 +241,8 @@ private static async Task TestAnalyzerAsync(
// listen to events
service.DiagnosticsUpdated += (s, a) =>
{
(syntax, semantic) = resultSetter(syntax, semantic, a.Diagnostics);
var diagnostics = a.GetDiagnostics(workspace, forPullDiagnostics: false);
(syntax, semantic) = resultSetter(syntax, semantic, diagnostics);
};

// now call each analyze method. none of them should run.
Expand Down Expand Up @@ -277,16 +283,18 @@ public async Task TestOpenFileOnlyAnalyzerDiagnostics()
{
if (workspace.IsDocumentOpen(a.DocumentId))
{
var diagnostics = a.GetDiagnostics(workspace, forPullDiagnostics: false);
// check the diagnostics are reported
Assert.Equal(document.Id, a.DocumentId);
Assert.Equal(1, a.Diagnostics.Length);
Assert.Equal(OpenFileOnlyAnalyzer.s_syntaxRule.Id, a.Diagnostics[0].Id);
Assert.Equal(1, diagnostics.Length);
Assert.Equal(OpenFileOnlyAnalyzer.s_syntaxRule.Id, diagnostics[0].Id);
}

if (a.DocumentId == document.Id && !workspace.IsDocumentOpen(a.DocumentId))
{
// check the diagnostics reported are cleared
Assert.Equal(0, a.Diagnostics.Length);
var diagnostics = a.GetDiagnostics(workspace, forPullDiagnostics: false);
Assert.Equal(0, diagnostics.Length);
}
};

Expand Down Expand Up @@ -342,12 +350,13 @@ public async Task TestSynchronizeWithBuild()
// listen to events
service.DiagnosticsUpdated += (s, a) =>
{
switch (a.Diagnostics.Length)
var diagnostics = a.GetDiagnostics(workspace, forPullDiagnostics: false);
switch (diagnostics.Length)
{
case 0:
return;
case 1:
syntax |= a.Diagnostics[0].Id == NoNameAnalyzer.s_syntaxRule.Id;
syntax |= diagnostics[0].Id == NoNameAnalyzer.s_syntaxRule.Id;
return;
default:
AssertEx.Fail("shouldn't reach here");
Expand Down Expand Up @@ -453,7 +462,8 @@ public async Task TestHostAnalyzerErrorNotLeaking()
var called = false;
service.DiagnosticsUpdated += (s, e) =>
{
if (e.Diagnostics.Length == 0)
var diagnostics = e.GetDiagnostics(workspace, forPullDiagnostics: false);
if (diagnostics.Length == 0)
{
return;
}
Expand Down Expand Up @@ -554,7 +564,8 @@ private static async Task TestFullSolutionAnalysisForProjectAsync(AdhocWorkspace
var called = false;
service.DiagnosticsUpdated += (s, e) =>
{
if (e.Diagnostics.Length == 0)
var diagnostics = e.GetDiagnostics(workspace, forPullDiagnostics: false);
if (diagnostics.Length == 0)
{
return;
}
Expand Down Expand Up @@ -609,7 +620,7 @@ internal async Task TestAdditionalFileAnalyzer(bool registerFromInitialize, bool
var diagnostics = new ConcurrentSet<DiagnosticData>();
service.DiagnosticsUpdated += (s, e) =>
{
diagnostics.AddRange(e.Diagnostics);
diagnostics.AddRange(e.GetDiagnostics(workspace, forPullDiagnostics: false));
};

var incrementalAnalyzer = (DiagnosticIncrementalAnalyzer)service.CreateIncrementalAnalyzer(workspace);
Expand Down Expand Up @@ -702,12 +713,13 @@ internal async Task TestDiagnosticSuppressor(bool includeAnalyzer, bool includeS
DiagnosticData diagnostic = null;
service.DiagnosticsUpdated += (s, e) =>
{
if (e.Diagnostics.Length == 0)
var diagnostics = e.GetDiagnostics(workspace, forPullDiagnostics: false);
if (diagnostics.Length == 0)
{
return;
}

diagnostic = Assert.Single(e.Diagnostics);
diagnostic = Assert.Single(diagnostics);
};

var incrementalAnalyzer = (DiagnosticIncrementalAnalyzer)service.CreateIncrementalAnalyzer(workspace);
Expand Down Expand Up @@ -805,7 +817,10 @@ void M()
var diagnostics = ArrayBuilder<DiagnosticData>.GetInstance();
service.DiagnosticsUpdated += (s, e) =>
{
diagnostics.AddRange(e.Diagnostics.Where(d => d.Id == IDEDiagnosticIds.RemoveUnnecessarySuppressionDiagnosticId).OrderBy(d => d.GetTextSpan()));
diagnostics.AddRange(
e.GetDiagnostics(workspace, forPullDiagnostics: false)
.Where(d => d.Id == IDEDiagnosticIds.RemoveUnnecessarySuppressionDiagnosticId)
.OrderBy(d => d.GetTextSpan()));
};

var incrementalAnalyzer = (DiagnosticIncrementalAnalyzer)service.CreateIncrementalAnalyzer(workspace);
Expand Down Expand Up @@ -880,13 +895,14 @@ void M()
DiagnosticData diagnostic = null;
service.DiagnosticsUpdated += (s, e) =>
{
if (e.Diagnostics.IsEmpty)
var diagnostics = e.GetDiagnostics(workspace, forPullDiagnostics: false);
if (diagnostics.IsEmpty)
{
return;
}

Assert.Null(diagnostic);
diagnostic = Assert.Single(e.Diagnostics);
diagnostic = Assert.Single(diagnostics);
};

var incrementalAnalyzer = (DiagnosticIncrementalAnalyzer)service.CreateIncrementalAnalyzer(workspace);
Expand Down
22 changes: 11 additions & 11 deletions src/EditorFeatures/Test/Diagnostics/DiagnosticServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,16 @@ public void TestGetDiagnostics1()
var id = Tuple.Create(workspace, document);
var diagnostic = RaiseDiagnosticEvent(mutex, source, workspace, document.Project.Id, document.Id, id);

var data1 = diagnosticService.GetDiagnostics(workspace, null, null, null, false, CancellationToken.None);
var data1 = diagnosticService.GetDiagnostics(workspace, null, null, null, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(diagnostic, data1.Single());

var data2 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, null, null, false, CancellationToken.None);
var data2 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, null, null, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(diagnostic, data2.Single());

var data3 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, null, false, CancellationToken.None);
var data3 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, null, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(diagnostic, data3.Single());

var data4 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, id, false, CancellationToken.None);
var data4 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, id, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(diagnostic, data4.Single());
}

Expand Down Expand Up @@ -85,19 +85,19 @@ public void TestGetDiagnostics2()
RaiseDiagnosticEvent(mutex, source, workspace, document.Project.Id, null, id3);
RaiseDiagnosticEvent(mutex, source, workspace, null, null, Tuple.Create(workspace));

var data1 = diagnosticService.GetDiagnostics(workspace, null, null, null, false, CancellationToken.None);
var data1 = diagnosticService.GetDiagnostics(workspace, null, null, null, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(5, data1.Count());

var data2 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, null, null, false, CancellationToken.None);
var data2 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, null, null, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(4, data2.Count());

var data3 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, null, id3, false, CancellationToken.None);
var data3 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, null, id3, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(1, data3.Count());

var data4 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, null, false, CancellationToken.None);
var data4 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, null, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(2, data4.Count());

var data5 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, id, false, CancellationToken.None);
var data5 = diagnosticService.GetDiagnostics(workspace, document.Project.Id, document.Id, id, includeSuppressedDiagnostics: false, forPullDiagnostics: false, CancellationToken.None);
Assert.Equal(1, data5.Count());
}

Expand Down Expand Up @@ -128,7 +128,7 @@ public void TestCleared()
RaiseDiagnosticEvent(mutex, source2, workspace, null, null, Tuple.Create(workspace));

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

diagnosticService.DiagnosticsUpdated -= MarkSet;
Expand All @@ -143,7 +143,7 @@ public void TestCleared()
mutex.WaitOne();

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

void MarkCalled(object sender, DiagnosticsUpdatedArgs args)
Expand Down
4 changes: 2 additions & 2 deletions src/EditorFeatures/Test/Diagnostics/MockDiagnosticService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ public MockDiagnosticService()
{
}

public ImmutableArray<DiagnosticData> GetDiagnostics(Workspace workspace, ProjectId? projectId, DocumentId? documentId, object? id, bool includeSuppressedDiagnostics, CancellationToken cancellationToken)
public ImmutableArray<DiagnosticData> GetDiagnostics(Workspace workspace, ProjectId? projectId, DocumentId? documentId, object? id, bool includeSuppressedDiagnostics, bool forPullDiagnostics, CancellationToken cancellationToken)
{
Assert.Equal(projectId, GetProjectId(workspace));
Assert.Equal(documentId, GetDocumentId(workspace));

return _diagnostic == null ? ImmutableArray<DiagnosticData>.Empty : ImmutableArray.Create(_diagnostic);
}

public ImmutableArray<DiagnosticBucket> GetDiagnosticBuckets(Workspace workspace, ProjectId? projectId, DocumentId? documentId, CancellationToken cancellationToken)
public ImmutableArray<DiagnosticBucket> GetDiagnosticBuckets(Workspace workspace, ProjectId? projectId, DocumentId? documentId, bool forPullDiagnostics, CancellationToken cancellationToken)
{
Assert.Equal(projectId, GetProjectId(workspace));
Assert.Equal(documentId, GetDocumentId(workspace));
Expand Down
10 changes: 7 additions & 3 deletions src/EditorFeatures/Test/EditAndContinue/EditAndContinueDiagnosticUpdateSourceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,6 @@ public void ReportDiagnostics()

var updates = new List<string>();

source.DiagnosticsUpdated += (object sender, DiagnosticsUpdatedArgs e)
=> updates.Add($"{e.Kind} p={e.ProjectId} d={e.DocumentId}: {string.Join(",", e.Diagnostics.Select(d => d.Id.ToString()))}");

var srcC1 = "class C1 {}";
var srcC2 = "class C2 {}";
var srcD1 = "class D1 {}";
Expand All @@ -41,6 +38,13 @@ public void ReportDiagnostics()
var docD2 = new TestHostDocument(srcD2, displayName: "DocD2");

var workspace = new TestWorkspace();

source.DiagnosticsUpdated += (object sender, DiagnosticsUpdatedArgs e) =>
{
var diagnostics = e.GetDiagnostics(workspace, forPullDiagnostics: false);
updates.Add($"{e.Kind} p={e.ProjectId} d={e.DocumentId}: {string.Join(",", diagnostics.Select(d => d.Id.ToString()))}");
};

var projC = new TestHostProject(workspace, "ProjC");
projC.AddDocument(docC1);
projC.AddDocument(docC2);
Expand Down
Loading