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

[LSP] More correctly respect background analysis scope #61392

Merged
merged 1 commit into from
May 19, 2022
Merged

[LSP] More correctly respect background analysis scope #61392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 12 additions & 14 deletions src/Features/LanguageServer/Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
Expand Down Expand Up @@ -120,24 +122,20 @@ async Task AddDocumentsFromProjectAsync(Project? project, ImmutableArray<string>
return;
}

// if the project doesn't necessarily have an open file in it, then only include it if the user has full
// solution analysis on.
if (!isOpen)
var isFSAOn = globalOptions.GetBackgroundAnalysisScope(project.Language) == BackgroundAnalysisScope.FullSolution;
var documents = ImmutableArray<Document>.Empty;
// If FSA is on, then add all the documents in the project. Other analysis scopes are handled by the document pull handler.
Copy link
Member

Choose a reason for hiding this comment

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

is that ok though? won't this mean workspace diags and document diags for the same file might start conflicting with each other?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, LSP open documents / razor documents are filtered out later

if (isFSAOn)
{
if (globalOptions.GetBackgroundAnalysisScope(project.Language) != BackgroundAnalysisScope.FullSolution)
{
context.TraceInformation($"Skipping project '{project.Name}' as it has no open document and Full Solution Analysis is off");
return;
}
documents = documents.AddRange(project.Documents);
Copy link
Member

Choose a reason for hiding this comment

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

don't really need the .AddRange since you set it to empty above. but nbd

}

// Otherwise, if the user has an open file from this project, or FSA is on, then include all the
Copy link
Member Author

Choose a reason for hiding this comment

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

neither the old comment nor old behavior match what non-LSP does.

// documents from it. If all features are enabled for source generated documents, make sure they are
// included as well.
var documents = project.Documents;
if (solution.Workspace.Services.GetService<IWorkspaceConfigurationService>()?.Options.EnableOpeningSourceGeneratedFiles == true)
// If all features are enabled for source generated documents, make sure they are included when FSA is on or a file in the project is open.
// This is done because for either scenario we've already run generators, so there shouldn't be much cost in getting the diagnostics.
if ((isFSAOn || isOpen) && solution.Workspace.Services.GetService<IWorkspaceConfigurationService>()?.Options.EnableOpeningSourceGeneratedFiles == true)
{
documents = documents.Concat(await project.GetSourceGeneratedDocumentsAsync(cancellationToken).ConfigureAwait(false));
var sourceGeneratedDocuments = await project.GetSourceGeneratedDocumentsAsync(cancellationToken).ConfigureAwait(false);
documents = documents.AddRange(sourceGeneratedDocuments);
}

foreach (var document in documents)
Expand Down
53 changes: 17 additions & 36 deletions src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,23 @@ public async Task TestWorkspaceDiagnosticsForClosedFilesWithFSAOn(bool useVSDiag
Assert.Empty(results[1].Diagnostics);
}

[Theory, CombinatorialData]
public async Task TestNoWorkspaceDiagnosticsForClosedFilesWithFSAOffWithFileInProjectOpen(bool useVSDiagnostics)
{
var markup1 =
@"class A {";
var markup2 = "";
using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync(
new[] { markup1, markup2 }, BackgroundAnalysisScope.OpenFiles, useVSDiagnostics, pullDiagnostics: false);

var firstDocument = testLspServer.GetCurrentSolution().Projects.Single().Documents.First();
await OpenDocumentAsync(testLspServer, firstDocument);

var results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics);

Assert.Empty(results);
}

[Theory, CombinatorialData]
public async Task TestWorkspaceDiagnosticsIncludesSourceGeneratorDiagnosticsClosedFSAOn(bool useVSDiagnostics)
{
Expand Down Expand Up @@ -617,42 +634,6 @@ public async Task TestWorkspaceDiagnosticsDoesNotIncludeSourceGeneratorDiagnosti
Assert.Empty(results);
}

[Theory, CombinatorialData]
public async Task TestWorkspaceDiagnosticsIncludesSourceGeneratorDiagnosticsClosedFSAOffAndFilesOpen(bool useVSDiagnostics)
Copy link
Member Author

Choose a reason for hiding this comment

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

this was testing the old behavior - that closed files in a project with an open file reported generator diagnostics. This should not be true with the new behavior

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 have a test taht shows that you don't get diagnostics now?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, see the test added above ^

{
// In this case, even though one file is open, we can still have diagnostics for the other file in the project, since by
// nature the generator had to run regardless.
var markup = "// Hello, World";
using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync(new[] { markup, markup }, BackgroundAnalysisScope.OpenFiles, useVSDiagnostics, pullDiagnostics: true);

var documentWithDiagnostics = testLspServer.GetCurrentSolution().Projects.Single().Documents.First();
var documentToOpen = testLspServer.GetCurrentSolution().Projects.Single().Documents.Last();

Assert.NotSame(documentWithDiagnostics, documentToOpen);

// Calling GetTextBuffer will effectively open the file.
testLspServer.TestWorkspace.Documents.Single(id => id.Id == documentToOpen.Id).GetTextBuffer();

var documentTrackingService = (TestDocumentTrackingService)testLspServer.TestWorkspace.Services.GetRequiredService<IDocumentTrackingService>();
documentTrackingService.SetActiveDocument(documentToOpen.Id);

var generator = new DiagnosticProducingGenerator(
context => Location.Create(
context.Compilation.SyntaxTrees.Single(t => t.FilePath == documentWithDiagnostics.FilePath),
new TextSpan(0, 10)));

testLspServer.TestWorkspace.OnAnalyzerReferenceAdded(
documentWithDiagnostics.Project.Id,
new TestGeneratorReference(generator));

await OpenDocumentAsync(testLspServer, documentToOpen);

var results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics);

var diagnostic = Assert.Single(results.Single().Diagnostics);
Assert.Equal(DiagnosticProducingGenerator.Descriptor.Id, diagnostic.Code);
}

[Theory, CombinatorialData]
public async Task TestNoWorkspaceDiagnosticsForClosedFilesWithFSAOnAndInPushMode(bool useVSDiagnostics)
{
Expand Down