Add support for additional file diagnostics in workspace pull#61930
Add support for additional file diagnostics in workspace pull#61930dibarbet merged 3 commits intodotnet:mainfrom
Conversation
| static LSP.Diagnostic[]? GetApplicableDiagnostics(LSP.CodeActionContext context, IUnifiedSuggestedAction action) | ||
| { | ||
| if (action is UnifiedCodeFixSuggestedAction codeFixAction) | ||
| if (action is UnifiedCodeFixSuggestedAction codeFixAction && context.Diagnostics != null) |
There was a problem hiding this comment.
real bug - if the client didn't pass us diagnostics with the code action request we would throw. found during test cleanup
| { | ||
| public abstract partial class AbstractLanguageServerProtocolTests | ||
| { | ||
| internal record struct InitializationOptions() |
There was a problem hiding this comment.
struct to cleanup all the test method overloads
|
|
||
| protected virtual TestComposition Composition => s_composition; | ||
|
|
||
| private protected virtual TestAnalyzerReferenceByLanguage TestAnalyzerReferences |
There was a problem hiding this comment.
moved diagnostics initialization into the base so that individual tests don't have to do something special to get diagnostics working.
| /// </summary> | ||
| protected Task<TestLspServer> CreateTestLspServerAsync(string[] markups, LSP.ClientCapabilities? clientCapabilities = null) | ||
| => CreateTestLspServerAsync(markups, Array.Empty<string>(), LanguageNames.CSharp, clientCapabilities); | ||
| private protected Task<TestLspServer> CreateTestLspServerAsync(string markup, InitializationOptions? initializationOptions = null) |
There was a problem hiding this comment.
overload cleanup using initialization options
| return new ProjectOrDocumentId(additionalDocument.Id); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
does this indicate a bug?
There was a problem hiding this comment.
Nope! It is a valid scenario when a document is removed / deleted for example.
| var workspaceConfigurationService = exportProvider.GetExportedValue<TestWorkspaceConfigurationService>(); | ||
| workspaceConfigurationService.Options = new WorkspaceConfigurationOptions(EnableOpeningSourceGeneratedFiles: true); | ||
|
|
||
| if (lspOptions.OptionUpdater != null) |
There was a problem hiding this comment.
bug fix - previously the solution crawler was created before we set the LSP options. So even when pull diagnostics was on in the tests we also turned on solution crawler
| => new WorkspaceDiagnosticReport(new[] | ||
| { | ||
| new WorkspaceDocumentDiagnosticReport(new WorkspaceUnchangedDocumentDiagnosticReport(identifier.Uri, resultId, version: null)) | ||
| }); |
There was a problem hiding this comment.
what's the expected future for the PullDiags vs ExperimentalPullDiags apis?
There was a problem hiding this comment.
the future is experimental pull diagnostics. And the VS one will be deprecated. The experimental APIs were just added to the official spec recently
| var documentDiagnostics = await diagnosticAnalyzerService.GetDiagnosticsForSpanAsync(Document, range: null, cancellationToken: cancellationToken).ConfigureAwait(false); | ||
| // We call GetDiagnosticsForIdsAsync as we want to ensure we get the full set of diagnostics for this document | ||
| // including those reported as a compilation end diagnostic. These are not included in document pull (uses GetDiagnosticsForSpan) due to cost. | ||
| // However we can include them as a part of workspace pull when FSA is on. |
There was a problem hiding this comment.
this make sense... but where is the FSA check done?
There was a problem hiding this comment.
FSA check is done before we add the diagnostic source here - https://github.com/dotnet/roslyn/pull/61930/files/5cef8f96e1d169290397810e16884eb007420749#diff-477ea689f972f6e36265ae4e8f2680cc204a317208115123023cbffdc6cf3664R129
| /// Creates the appropriate LSP type to report a new set of diagnostics and resultId. | ||
| /// </summary> | ||
| protected abstract TReport CreateReport(TextDocumentIdentifier identifier, LSP.Diagnostic[]? diagnostics, string? resultId); | ||
| protected abstract TReport CreateReport(TextDocumentIdentifier identifier, LSP.Diagnostic[] diagnostics, string resultId); |
There was a problem hiding this comment.
result of bug fixing - for VSCode diagnostics we were previously creating unchanged reports for removed documents. This splits it out so we know exactly which is being called and can create appropriate types
| + '|' + string.Format(FeaturesResources.Introduce_constant_for_0, "1")); | ||
|
|
||
| var topLevelAction = Assert.Single(results.Where(action => action.Title == FeaturesResources.Introduce_constant)); | ||
| var expectedChildActionTitle = FeaturesResources.Introduce_constant + '|' + string.Format(FeaturesResources.Introduce_constant_for_0, "1"); |
There was a problem hiding this comment.
test fallout of always having diagnostics be enabled in LSP tests
…dotnet#61930)" This reverts commit e4d1ce6.
Looks like
Along the way I cleaned up a bunch of the testing methods and fixed a couple bugs I discovered during cleanup.