From 8aa8252a231cfc49a03ab0b12800ea38a9ef18e2 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Sun, 27 Sep 2020 23:11:11 -0700 Subject: [PATCH 1/5] Decouple FixAll from the workspace Today, the RunFixAllInCodeActionService is tightly coupled to the current workspace, applying its changes to the workspace as it processes the request and counting on the updates from doing so to invalidate diagnostic caches. While this does work, it's has 2 major downsides: 1. It's a race condition waiting to happen, where the propagation of the changed documents invalidating the diagnostic provider's caches races with the next fix all operation attempting to get the diagnostics it needs to fix. 2. It has bad interations with the buffer updater from clients. The service returns the needed changes to clients, who then apply the changes. The client will then see that changes have been applied, and send them back to the server. The server's workspace has already been updated with these changes, so it ends up double applying the changes, leaving the server's workspace in a bad state with garbage in many files. This commit breaks the dependency of the FixAll service on the current workspace. We now have the fixes occur on top of the previous fix's changed solution. At the end, if the client requested the changes be applied on the workspace side, then we apply the changes to the workspace at that point. In order for this work, I had to modify the diagnostic workers a bit to add an option to force a document to be analyzed, skipping the cache entirely. This is necessary because the cache is only invalidated on document add/remove/change in the workspace itself. The documents we work on linearly in the fix all provider do not get added/changed/removed in the workspace, so we need to analyze them again, right then and there. To support this, the AnalyzerWorkQueue is now based on documents, not document ids, though the caches in the workers are still based on ids. --- .../Models/v1/FixAll/RunFixAllRequest.cs | 2 + .../Helpers/CodeFixProviderExtensions.cs | 29 +++ .../Helpers/DiagnosticExtensions.cs | 2 +- .../Services/Diagnostics/CodeCheckService.cs | 2 +- ...tWithFixProvidersAndMatchingDiagnostics.cs | 102 ---------- .../Refactoring/GetFixAllCodeActionService.cs | 31 ++- .../Refactoring/RunFixAllCodeActionService.cs | 170 +++++++++++----- .../Refactoring/V2/BaseCodeActionService.cs | 68 ++++--- .../V2/CachingCodeFixProviderForProjects.cs | 10 +- .../Workers/Diagnostics/AnalyzerWorkQueue.cs | 22 +-- .../Diagnostics/CSharpDiagnosticWorker.cs | 17 +- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 184 +++++++++++------- .../CsharpDiagnosticWorkerComposer.cs | 5 + .../Diagnostics/DocumentDiagnostics.cs | 13 +- .../Diagnostics/ICsDiagnosticWorker.cs | 3 +- .../AnalyzerWorkerQueueFacts.cs | 49 ++--- .../FixAllFacts.cs | 147 ++++++++++++-- 17 files changed, 529 insertions(+), 327 deletions(-) create mode 100644 src/OmniSharp.Roslyn.CSharp/Helpers/CodeFixProviderExtensions.cs delete mode 100644 src/OmniSharp.Roslyn.CSharp/Services/Refactoring/DocumentWithFixProvidersAndMatchingDiagnostics.cs diff --git a/src/OmniSharp.Abstractions/Models/v1/FixAll/RunFixAllRequest.cs b/src/OmniSharp.Abstractions/Models/v1/FixAll/RunFixAllRequest.cs index aef5e65cd4..5180f474c9 100644 --- a/src/OmniSharp.Abstractions/Models/v1/FixAll/RunFixAllRequest.cs +++ b/src/OmniSharp.Abstractions/Models/v1/FixAll/RunFixAllRequest.cs @@ -13,5 +13,7 @@ public class RunFixAllRequest : SimpleFileRequest public int Timeout { get; set; } = 3000; public bool WantsAllCodeActionOperations { get; set; } public bool WantsTextChanges { get; set; } + // Nullable for backcompat: null == true, for requests that don't set it + public bool? ApplyChanges { get; set; } } } diff --git a/src/OmniSharp.Roslyn.CSharp/Helpers/CodeFixProviderExtensions.cs b/src/OmniSharp.Roslyn.CSharp/Helpers/CodeFixProviderExtensions.cs new file mode 100644 index 0000000000..e16b97e8e7 --- /dev/null +++ b/src/OmniSharp.Roslyn.CSharp/Helpers/CodeFixProviderExtensions.cs @@ -0,0 +1,29 @@ +using System.Collections.Generic; +using System.Linq; +using Microsoft.CodeAnalysis.CodeFixes; + +namespace OmniSharp.Roslyn.CSharp.Helpers +{ + internal static class CodeFixProviderExtensions + { + // http://sourceroslyn.io/#Microsoft.VisualStudio.LanguageServices.CSharp/LanguageService/CSharpCodeCleanupFixer.cs,d9a375db0f1e430e,references + // CS8019 isn't directly used (via roslyn) but has an analyzer that report different diagnostic based on CS8019 to improve user experience. + private static readonly Dictionary _customDiagVsFixMap = new Dictionary + { + { "CS8019", "RemoveUnnecessaryImportsFixable" } + }; + + // Theres specific filterings between what is shown and what is fixed because of some custom mappings + // between diagnostics and their fixers. We dont want to show text 'RemoveUnnecessaryImportsFixable: ...' + // but instead 'CS8019: ...' where actual fixer is RemoveUnnecessaryImportsFixable behind the scenes. + public static bool HasFixForId(this CodeFixProvider provider, string diagnosticId) + { + return provider.FixableDiagnosticIds.Any(id => id == diagnosticId) && !_customDiagVsFixMap.ContainsKey(diagnosticId) || HasMappedFixAvailable(diagnosticId, provider); + } + + private static bool HasMappedFixAvailable(string diagnosticId, CodeFixProvider provider) + { + return (_customDiagVsFixMap.ContainsKey(diagnosticId) && provider.FixableDiagnosticIds.Any(id => id == _customDiagVsFixMap[diagnosticId])); + } + } +} diff --git a/src/OmniSharp.Roslyn.CSharp/Helpers/DiagnosticExtensions.cs b/src/OmniSharp.Roslyn.CSharp/Helpers/DiagnosticExtensions.cs index 623e81463f..7ba8f29cc1 100644 --- a/src/OmniSharp.Roslyn.CSharp/Helpers/DiagnosticExtensions.cs +++ b/src/OmniSharp.Roslyn.CSharp/Helpers/DiagnosticExtensions.cs @@ -35,7 +35,7 @@ internal static DiagnosticLocation ToDiagnosticLocation(this Diagnostic diagnost internal static IEnumerable DistinctDiagnosticLocationsByProject(this IEnumerable documentDiagnostic) { return documentDiagnostic - .SelectMany(x => x.Diagnostics, (parent, child) => (projectName: parent.ProjectName, diagnostic: child)) + .SelectMany(x => x.Diagnostics, (parent, child) => (projectName: parent.Project.Name, diagnostic: child)) .Select(x => new { location = x.diagnostic.ToDiagnosticLocation(), diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Diagnostics/CodeCheckService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Diagnostics/CodeCheckService.cs index 2c8ec78556..252f442dc2 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Diagnostics/CodeCheckService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Diagnostics/CodeCheckService.cs @@ -47,7 +47,7 @@ private static QuickFixResponse GetResponseFromDiagnostics(ImmutableArray string.IsNullOrEmpty(fileName) - || x.DocumentPath == fileName) + || x.Document.FilePath == fileName) .DistinctDiagnosticLocationsByProject() .Where(x => x.FileName != null); diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/DocumentWithFixProvidersAndMatchingDiagnostics.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/DocumentWithFixProvidersAndMatchingDiagnostics.cs deleted file mode 100644 index ab51ee46d5..0000000000 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/DocumentWithFixProvidersAndMatchingDiagnostics.cs +++ /dev/null @@ -1,102 +0,0 @@ -using System.Collections.Generic; -using System.Collections.Immutable; -using System.Linq; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CodeActions; -using Microsoft.CodeAnalysis.CodeFixes; -using OmniSharp.Roslyn.CSharp.Services.Diagnostics; - -namespace OmniSharp.Roslyn.CSharp.Services.Refactoring -{ - - public class DocumentWithFixProvidersAndMatchingDiagnostics - { - private readonly DocumentDiagnostics _documentDiagnostics; - - // http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices.CSharp/LanguageService/CSharpCodeCleanupFixer.cs,d9a375db0f1e430e,references - // CS8019 isn't directly used (via roslyn) but has an analyzer that report different diagnostic based on CS8019 to improve user experience. - private static readonly Dictionary _customDiagVsFixMap = new Dictionary - { - { "CS8019", "RemoveUnnecessaryImportsFixable" } - }; - - private DocumentWithFixProvidersAndMatchingDiagnostics(CodeFixProvider provider, DocumentDiagnostics documentDiagnostics) - { - CodeFixProvider = provider; - _documentDiagnostics = documentDiagnostics; - FixAllProvider = provider.GetFixAllProvider(); - } - - public CodeFixProvider CodeFixProvider { get; } - public FixAllProvider FixAllProvider { get; } - public DocumentId DocumentId => _documentDiagnostics.DocumentId; - public ProjectId ProjectId => _documentDiagnostics.ProjectId; - public string DocumentPath => _documentDiagnostics.DocumentPath; - - public IEnumerable<(string id, string messsage)> FixableDiagnostics => _documentDiagnostics.Diagnostics - .Where(x => HasFixToShow(x.Id)) - .Select(x => (x.Id, x.GetMessage())); - - // Theres specific filterings between what is shown and what is fixed because of some custom mappings - // between diagnostics and their fixers. We dont want to show text 'RemoveUnnecessaryImportsFixable: ...' - // but instead 'CS8019: ...' where actual fixer is RemoveUnnecessaryImportsFixable behind the scenes. - private bool HasFixToShow(string diagnosticId) - { - return CodeFixProvider.FixableDiagnosticIds.Any(id => id == diagnosticId) && !_customDiagVsFixMap.ContainsValue(diagnosticId) || HasMappedFixAvailable(diagnosticId); - } - - public bool HasFixForId(string diagnosticId) - { - return CodeFixProvider.FixableDiagnosticIds.Any(id => id == diagnosticId && !_customDiagVsFixMap.ContainsKey(diagnosticId)) || HasMappedFixAvailable(diagnosticId); - } - - private bool HasMappedFixAvailable(string diagnosticId) - { - return (_customDiagVsFixMap.ContainsKey(diagnosticId) && CodeFixProvider.FixableDiagnosticIds.Any(id => id == _customDiagVsFixMap[diagnosticId])); - } - - public static ImmutableArray CreateWithMatchingProviders(ImmutableArray providers, DocumentDiagnostics documentDiagnostics) - { - return - providers - .Select(provider => new DocumentWithFixProvidersAndMatchingDiagnostics(provider, documentDiagnostics)) - .Where(x => x._documentDiagnostics.Diagnostics.Any(d => x.HasFixToShow(d.Id)) || x._documentDiagnostics.Diagnostics.Any(d => x.HasFixForId(d.Id))) - .Where(x => x.FixAllProvider != null) - .ToImmutableArray(); - } - - public async Task<(CodeAction action, ImmutableArray idsToFix)> RegisterCodeFixesOrDefault(Document document) - { - CodeAction action = null; - - var fixableDiagnostics = _documentDiagnostics - .Diagnostics - .Where(x => HasFixForId(x.Id)) - .ToImmutableArray(); - - foreach (var diagnostic in fixableDiagnostics) - { - var context = new CodeFixContext( - document, - diagnostic, - (a, _) => - { - if (action == null) - { - action = a; - } - }, - CancellationToken.None); - - await CodeFixProvider.RegisterCodeFixesAsync(context).ConfigureAwait(false); - } - - if(action == null) - return default; - - return (action, fixableDiagnostics.Select(x => x.Id).Distinct().ToImmutableArray()); - } - } -} diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/GetFixAllCodeActionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/GetFixAllCodeActionService.cs index 24d516b324..73fe279472 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/GetFixAllCodeActionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/GetFixAllCodeActionService.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Collections.Immutable; using System.Composition; using System.Linq; using System.Threading.Tasks; @@ -6,6 +7,7 @@ using Microsoft.Extensions.Logging; using OmniSharp.Abstractions.Models.V1.FixAll; using OmniSharp.Mef; +using OmniSharp.Roslyn.CSharp.Helpers; using OmniSharp.Roslyn.CSharp.Services.Refactoring.V2; using OmniSharp.Roslyn.CSharp.Workers.Diagnostics; using OmniSharp.Services; @@ -28,17 +30,30 @@ CachingCodeFixProviderForProjects codeFixesForProject public override async Task Handle(GetFixAllRequest request) { - var availableFixes = await GetDiagnosticsMappedWithFixAllProviders(request.Scope, request.FileName); + var document = Workspace.GetDocument(request.FileName); + if (document is null) + { + Logger.LogWarning("Could not find document for file {0}", request.FileName); + return new GetFixAllResponse(ImmutableArray.Empty); + } - var distinctDiagnosticsThatCanBeFixed = availableFixes - .SelectMany(x => x.FixableDiagnostics) - .GroupBy(x => x.id) // Distinct isn't good fit here since theres cases where Id has multiple different messages based on location, just show one of them. - .Select(x => x.First()) - .Select(x => new FixAllItem(x.id, x.messsage)) + var allDiagnostics = await GetDiagnosticsAsync(request.Scope, document); + var validFixes = allDiagnostics + .GroupBy(docAndDiag => docAndDiag.Document.Project) + .SelectMany(grouping => + { + var projectFixProviders = GetCodeFixProviders(grouping.Key); + return grouping + .SelectMany(docAndDiag => docAndDiag.Diagnostics) + .Where(diag => projectFixProviders.Any(provider => provider.HasFixForId(diag.Id))); + }) + .GroupBy(diag => diag.Id) + .Select(grouping => grouping.First()) + .Select(x => new FixAllItem(x.Id, x.GetMessage())) .OrderBy(x => x.Id) .ToArray(); - return new GetFixAllResponse(distinctDiagnosticsThatCanBeFixed); + return new GetFixAllResponse(validFixes); } } -} \ No newline at end of file +} diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs index 1bde6dd6a7..4d55a43f29 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; +using System.Diagnostics; using System.IO; using System.Linq; using System.Threading; @@ -16,6 +17,7 @@ using OmniSharp.Roslyn.CSharp.Workers.Diagnostics; using OmniSharp.Services; using FixAllScope = OmniSharp.Abstractions.Models.V1.FixAll.FixAllScope; +using RoslynFixAllScope = Microsoft.CodeAnalysis.CodeFixes.FixAllScope; namespace OmniSharp.Roslyn.CSharp.Services.Refactoring { @@ -44,76 +46,140 @@ public RunFixAllCodeActionService(ICsDiagnosticWorker diagnosticWorker, public async override Task Handle(RunFixAllRequest request) { - if (request.Scope != FixAllScope.Document && request.FixAllFilter == null) - throw new NotImplementedException($"Only scope '{nameof(FixAllScope.Document)}' is currently supported when filter '{nameof(request.FixAllFilter)}' is not set."); - var solutionBeforeChanges = Workspace.CurrentSolution; + if (!(Workspace.GetDocument(request.FileName) is Document document)) + { + _logger.LogWarning("Requested fix all for document {0} that does not exist!", request.FileName); + return new RunFixAllResponse(); + } - var mappedProvidersWithDiagnostics = await GetDiagnosticsMappedWithFixAllProviders(request.Scope, request.FileName); + var cancellationSource = new CancellationTokenSource(TimeSpan.FromMilliseconds(request.Timeout)); + switch (request) + { + case { Scope: FixAllScope.Document, FixAllFilter: null }: + var allDiagnosticsInFile = await GetDiagnosticsAsync(FixAllScope.Document, document); + if (allDiagnosticsInFile.IsDefaultOrEmpty) + { + break; + } - var filteredProvidersWithFix = mappedProvidersWithDiagnostics - .Where(diagWithFix => - { - if (request.FixAllFilter == null) - return true; + if (allDiagnosticsInFile.Length > 1) + { + Debug.Fail("Not expecting to get back more documents than were passed in"); + break; + } - return request.FixAllFilter.Any(x => diagWithFix.HasFixForId(x.Id)); - }); + _logger.LogInformation("Found {0} diagnostics to fix.", allDiagnosticsInFile[0].Diagnostics.Length); + foreach (var diagnostic in allDiagnosticsInFile[0].Diagnostics) + { + document = await FixSpecificDiagnosticIdAsync(document, diagnostic.Id, FixAllScope.Document, CancellationToken.None); + } - var cancellationSource = new CancellationTokenSource(TimeSpan.FromMilliseconds(request.Timeout)); + break; - foreach (var singleFixableProviderWithDocument in filteredProvidersWithFix) - { - try - { - var document = Workspace.CurrentSolution.GetDocument(singleFixableProviderWithDocument.DocumentId); - - var fixer = singleFixableProviderWithDocument.FixAllProvider; + case { FixAllFilter: { } filters }: + foreach (var filter in filters) + { + document = await FixSpecificDiagnosticIdAsync(document, filter.Id, request.Scope, cancellationSource.Token); + } - var (action, fixableDiagnosticIds) = await singleFixableProviderWithDocument.RegisterCodeFixesOrDefault(document); + break; - if (action == null) - continue; + default: + throw new NotImplementedException($"Only scope '{nameof(FixAllScope.Document)}' is currently supported when filter '{nameof(request.FixAllFilter)}' is not set."); + } - var fixAllContext = new FixAllContext( - document, - singleFixableProviderWithDocument.CodeFixProvider, - Microsoft.CodeAnalysis.CodeFixes.FixAllScope.Project, - action.EquivalenceKey, - fixableDiagnosticIds, - _fixAllDiagnosticProvider, - cancellationSource.Token - ); + var solutionAfterChanges = document.Project.Solution; + if (request.ApplyChanges != false) + { + _logger.LogInformation("Applying changes from the fixers."); + Workspace.TryApplyChanges(solutionAfterChanges); + } - var fixes = await singleFixableProviderWithDocument.FixAllProvider.GetFixAsync(fixAllContext); + var changes = await GetFileChangesAsync(document.Project.Solution, solutionBeforeChanges, Path.GetDirectoryName(request.FileName), request.WantsTextChanges, request.WantsAllCodeActionOperations); - if (fixes == null) - continue; + return new RunFixAllResponse + { + Changes = changes.FileChanges + }; + } - var operations = await fixes.GetOperationsAsync(cancellationSource.Token); + private async Task FixSpecificDiagnosticIdAsync(Document document, string diagnosticId, FixAllScope scope, CancellationToken cancellationToken) + { + _logger.LogInformation("Fixing {0}.", diagnosticId); + var originalDoc = document; + var codeFixProvider = GetCodeFixProviderForId(document, diagnosticId); + if (codeFixProvider is null || !(codeFixProvider.GetFixAllProvider() is FixAllProvider fixAllProvider)) + { + _logger.LogInformation("Could not find a codefix provider or a fixall provider for {0}.", diagnosticId); + return originalDoc; + } - foreach (var o in operations) - { - _logger.LogInformation($"Applying operation {o.ToString()} from fix all with fix provider {singleFixableProviderWithDocument.CodeFixProvider} to workspace document {document.FilePath}."); + _logger.LogTrace("Determing if {0} is still present in the document.", diagnosticId); + var (diagnosticDocId, primaryDiagnostic) = await GetDocumentIdAndDiagnosticForGivenId(scope, document, diagnosticId); + cancellationToken.ThrowIfCancellationRequested(); + if (primaryDiagnostic is null) + { + _logger.LogInformation("No diagnostic locations found for {0}.", diagnosticId); + return originalDoc; + } - if (o is ApplyChangesOperation applyChangesOperation) - { - applyChangesOperation.Apply(Workspace, cancellationSource.Token); - } - } - } - catch (Exception ex) + if (document.Id != diagnosticDocId) + { + document = document.Project.Solution.GetDocument(diagnosticDocId); + if (document is null) { - _logger.LogError($"Running fix all action {singleFixableProviderWithDocument} in document {singleFixableProviderWithDocument.DocumentPath} prevented by error: {ex}"); + throw new InvalidOperationException("Could not find the document with the diagnostic in the solution"); } } - var changes = await GetFileChangesAsync(Workspace.CurrentSolution, solutionBeforeChanges, Path.GetDirectoryName(request.FileName), request.WantsTextChanges, request.WantsAllCodeActionOperations); + _logger.LogTrace("{0} is still present in the document. Getting fixes.", diagnosticId); + CodeAction action = null; + var context = new CodeFixContext(document, primaryDiagnostic, + (a, _) => + { + if (action == null) + { + action = a; + } + }, + cancellationToken); + + await codeFixProvider.RegisterCodeFixesAsync(context).ConfigureAwait(false); - return new RunFixAllResponse + var roslynScope = scope switch { - Changes = changes.FileChanges + FixAllScope.Document => RoslynFixAllScope.Document, + FixAllScope.Project => RoslynFixAllScope.Project, + FixAllScope.Solution => RoslynFixAllScope.Solution, + _ => throw new InvalidOperationException() }; + + var fixAllContext = new FixAllContext(document, codeFixProvider, roslynScope, action.EquivalenceKey, ImmutableArray.Create(diagnosticId), _fixAllDiagnosticProvider, cancellationToken); + + _logger.LogTrace("Finding FixAll fix for {0}.", diagnosticId); + var fixes = await fixAllProvider.GetFixAsync(fixAllContext); + if (fixes == null) + { + _logger.LogInformation("FixAll not found for {0}.", diagnosticId); + return originalDoc; + } + + _logger.LogTrace("Getting FixAll operations for {0}.", diagnosticId); + var operations = await fixes.GetOperationsAsync(cancellationToken); + + // Currently, there are no roslyn changes that will result in multiple ApplyChangesOperations + Debug.Assert(operations.OfType().Count() < 2); + if (operations.OfType().FirstOrDefault() is ApplyChangesOperation applyChangesOperation) + { + _logger.LogTrace("Found apply changes operation for {0}.", diagnosticId); + return applyChangesOperation.ChangedSolution.GetDocument(originalDoc.Id); + } + else + { + _logger.LogTrace("No apply changes operation for {0}.", diagnosticId); + return originalDoc; + } } private class FixAllDiagnosticProvider : FixAllContext.DiagnosticProvider @@ -127,13 +193,13 @@ public FixAllDiagnosticProvider(ICsDiagnosticWorker diagnosticWorker) public override async Task> GetAllDiagnosticsAsync(Project project, CancellationToken cancellationToken) { - var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.Select(x => x.FilePath).ToImmutableArray()); + var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.ToImmutableArray(), skipCache: true); return diagnostics.SelectMany(x => x.Diagnostics); } public override async Task> GetDocumentDiagnosticsAsync(Document document, CancellationToken cancellationToken) { - var documentDiagnostics = await _diagnosticWorker.GetDiagnostics(ImmutableArray.Create(document.FilePath)); + var documentDiagnostics = await _diagnosticWorker.GetDiagnostics(ImmutableArray.Create(document), skipCache: true); if (!documentDiagnostics.Any()) return new Diagnostic[] { }; @@ -143,7 +209,7 @@ public override async Task> GetDocumentDiagnosticsAsync( public override async Task> GetProjectDiagnosticsAsync(Project project, CancellationToken cancellationToken) { - var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.Select(x => x.FilePath).ToImmutableArray()); + var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.ToImmutableArray(), skipCache: true); return diagnostics.SelectMany(x => x.Diagnostics); } } diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs index 62622c71b9..3b128d5836 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Diagnostics; using System.IO; using System.Linq; using System.Reflection; @@ -16,6 +17,7 @@ using OmniSharp.Mef; using OmniSharp.Models; using OmniSharp.Models.V2.CodeActions; +using OmniSharp.Roslyn.CSharp.Helpers; using OmniSharp.Roslyn.CSharp.Services.Diagnostics; using OmniSharp.Roslyn.CSharp.Workers.Diagnostics; using OmniSharp.Roslyn.Utilities; @@ -23,6 +25,8 @@ using OmniSharp.Utilities; using FixAllScope = OmniSharp.Abstractions.Models.V1.FixAll.FixAllScope; +#nullable enable + namespace OmniSharp.Roslyn.CSharp.Services.Refactoring.V2 { public abstract class BaseCodeActionService : IRequestHandler @@ -30,8 +34,8 @@ public abstract class BaseCodeActionService : IRequestHandl protected readonly OmniSharpWorkspace Workspace; protected readonly IEnumerable Providers; protected readonly ILogger Logger; - private readonly ICsDiagnosticWorker diagnostics; - private readonly CachingCodeFixProviderForProjects codeFixesForProject; + private readonly ICsDiagnosticWorker _diagnostics; + private readonly CachingCodeFixProviderForProjects _codeFixesForProject; private readonly MethodInfo _getNestedCodeActions; protected Lazy> OrderedCodeRefactoringProviders; @@ -52,8 +56,8 @@ protected BaseCodeActionService( Workspace = workspace; Providers = providers; Logger = logger; - this.diagnostics = diagnostics; - this.codeFixesForProject = codeFixesForProject; + _diagnostics = diagnostics; + _codeFixesForProject = codeFixesForProject; OrderedCodeRefactoringProviders = new Lazy>(() => GetSortedCodeRefactoringProviders()); // Sadly, the CodeAction.NestedCodeActions property is still internal. @@ -123,7 +127,7 @@ private TextSpan GetTextSpan(ICodeActionRequest request, SourceText sourceText) private async Task CollectCodeFixesActions(Document document, TextSpan span, List codeActions) { - var diagnosticsWithProjects = await diagnostics.GetDiagnostics(ImmutableArray.Create(document.FilePath)); + var diagnosticsWithProjects = await _diagnostics.GetDiagnostics(ImmutableArray.Create(document), skipCache: false); var groupedBySpan = diagnosticsWithProjects .SelectMany(x => x.Diagnostics) @@ -163,7 +167,7 @@ private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerabl private List GetSortedCodeFixProviders(Document document) { - return ExtensionOrderer.GetOrderedOrUnorderedList(codeFixesForProject.GetAllCodeFixesForProject(document.Project.Id), attribute => attribute.Name).ToList(); + return ExtensionOrderer.GetOrderedOrUnorderedList(_codeFixesForProject.GetAllCodeFixesForProject(document.Project), attribute => attribute.Name).ToList(); } private List GetSortedCodeRefactoringProviders() @@ -211,33 +215,47 @@ private IEnumerable ConvertToAvailableCodeAction(IEnumerabl }); } - // Mapping means: each mapped item has one document that has one code fix provider and it's corresponding diagnostics. - // If same document has multiple codefixers (diagnostics with different fixers) will them be mapped as separate items. - protected async Task> GetDiagnosticsMappedWithFixAllProviders(FixAllScope scope, string fileName) + protected async Task<(DocumentId DocumentId, Diagnostic Diagnostic)> GetDocumentIdAndDiagnosticForGivenId(FixAllScope scope, Document document, string diagnosticId) { - ImmutableArray allDiagnostics = await GetCorrectDiagnosticsInScope(scope, fileName); + var allDocumentDiagnostics = await GetDiagnosticsAsync(scope, document); - var mappedProvidersWithDiagnostics = allDiagnostics - .SelectMany(diagnosticsInDocument => - DocumentWithFixProvidersAndMatchingDiagnostics.CreateWithMatchingProviders(codeFixesForProject.GetAllCodeFixesForProject(diagnosticsInDocument.ProjectId), diagnosticsInDocument)); + foreach (var documentAndDiagnostics in allDocumentDiagnostics) + { + if (documentAndDiagnostics.Diagnostics.FirstOrDefault(d => d.Id == diagnosticId) is Diagnostic diagnostic) + { + return (documentAndDiagnostics.Document.Id, diagnostic); + } + } - return mappedProvidersWithDiagnostics.ToImmutableArray(); + return default; + } + + protected ImmutableArray GetCodeFixProviders(Project project) + { + return _codeFixesForProject.GetAllCodeFixesForProject(project); + } + + protected CodeFixProvider? GetCodeFixProviderForId(Document document, string id) + { + // If Roslyn ever comes up with a UI for selecting what provider the user prefers, we might consider replicating. + // https://github.com/dotnet/roslyn/issues/27066 + return _codeFixesForProject.GetAllCodeFixesForProject(document.Project).FirstOrDefault(provider => provider.HasFixForId(id)); } - private async Task> GetCorrectDiagnosticsInScope(FixAllScope scope, string fileName) + protected async Task> GetDiagnosticsAsync(FixAllScope scope, Document document) { switch (scope) { case FixAllScope.Solution: - var documentsInSolution = Workspace.CurrentSolution.Projects.SelectMany(x => x.Documents).Select(x => x.FilePath).ToImmutableArray(); - return await diagnostics.GetDiagnostics(documentsInSolution); + var documentsInSolution = document.Project.Solution.Projects.SelectMany(p => p.Documents).ToImmutableArray(); + return await _diagnostics.GetDiagnostics(documentsInSolution, skipCache: true); case FixAllScope.Project: - var documentsInProject = Workspace.GetDocument(fileName).Project.Documents.Select(x => x.FilePath).ToImmutableArray(); - return await diagnostics.GetDiagnostics(documentsInProject); + var documensInProject = document.Project.Documents.ToImmutableArray(); + return await _diagnostics.GetDiagnostics(documensInProject, skipCache: true); case FixAllScope.Document: - return await diagnostics.GetDiagnostics(ImmutableArray.Create(fileName)); + return await _diagnostics.GetDiagnostics(ImmutableArray.Create(document), skipCache: true); default: - throw new NotImplementedException(); + throw new InvalidOperationException(); } } @@ -253,7 +271,7 @@ private async Task> GetCorrectDiagnosticsInS foreach (var documentId in projectChange.GetAddedDocuments()) { var newDocument = newSolution.GetDocument(documentId); - var text = await newDocument.GetTextAsync(); + var text = await newDocument!.GetTextAsync(); var newFilePath = newDocument.FilePath == null || !Path.IsPathRooted(newDocument.FilePath) ? Path.Combine(directory, newDocument.Name) @@ -307,14 +325,16 @@ private async Task> GetCorrectDiagnosticsInS { var newDocument = newSolution.GetDocument(documentId); var oldDocument = oldSolution.GetDocument(documentId); - var filePath = newDocument.FilePath; + Debug.Assert(oldDocument!.FilePath != null); + Debug.Assert(newDocument!.FilePath != null); + string filePath = newDocument.FilePath!; // file rename if (oldDocument != null && newDocument.Name != oldDocument.Name) { if (wantsAllCodeActionOperations) { - var newFilePath = GetNewFilePath(newDocument.Name, oldDocument.FilePath); + var newFilePath = GetNewFilePath(newDocument.Name, oldDocument.FilePath!); var text = await oldDocument.GetTextAsync(); var temp = solution.RemoveDocument(documentId); solution = temp.AddDocument(DocumentId.CreateNewId(oldDocument.Project.Id, newDocument.Name), newDocument.Name, text, oldDocument.Folders, newFilePath); diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/CachingCodeFixProviderForProjects.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/CachingCodeFixProviderForProjects.cs index ea230f0a16..e3d919f6ef 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/CachingCodeFixProviderForProjects.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/CachingCodeFixProviderForProjects.cs @@ -41,16 +41,14 @@ public CachingCodeFixProviderForProjects(ILoggerFactory loggerFactory, OmniSharp }; } - public ImmutableArray GetAllCodeFixesForProject(ProjectId projectId) + public ImmutableArray GetAllCodeFixesForProject(Project project) { - if (_cache.ContainsKey(projectId)) - return _cache[projectId]; - - var project = _workspace.CurrentSolution.GetProject(projectId); + if (_cache.ContainsKey(project.Id)) + return _cache[project.Id]; if (project == null) { - _cache.TryRemove(projectId, out _); + _cache.TryRemove(project.Id, out _); return ImmutableArray.Empty; } diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs index 0559544976..7a82148abf 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs @@ -17,8 +17,8 @@ public Queue(TimeSpan throttling) Throttling = throttling; } - public ImmutableHashSet WorkWaitingToExecute { get; set; } = ImmutableHashSet.Empty; - public ImmutableHashSet WorkExecuting { get; set; } = ImmutableHashSet.Empty; + public ImmutableHashSet WorkWaitingToExecute { get; set; } = ImmutableHashSet.Empty; + public ImmutableHashSet WorkExecuting { get; set; } = ImmutableHashSet.Empty; public DateTime LastThrottlingBegan { get; set; } = DateTime.UtcNow; public TimeSpan Throttling { get; } public CancellationTokenSource WorkPendingToken { get; set; } @@ -44,7 +44,7 @@ public AnalyzerWorkQueue(ILoggerFactory loggerFactory, int timeoutForPendingWork _maximumDelayWhenWaitingForResults = timeoutForPendingWorkMs; } - public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkType workType) + public void PutWork(IReadOnlyCollection documents, AnalyzerWorkType workType) { lock (_queueLock) { @@ -56,21 +56,21 @@ public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkTyp if (queue.WorkPendingToken == null) queue.WorkPendingToken = new CancellationTokenSource(); - queue.WorkWaitingToExecute = queue.WorkWaitingToExecute.Union(documentIds); + queue.WorkWaitingToExecute = queue.WorkWaitingToExecute.Union(documents); } } - public IReadOnlyCollection TakeWork(AnalyzerWorkType workType) + public IReadOnlyCollection TakeWork(AnalyzerWorkType workType) { lock (_queueLock) { var queue = _queues[workType]; if (IsThrottlingActive(queue) || queue.WorkWaitingToExecute.IsEmpty) - return ImmutableHashSet.Empty; + return ImmutableHashSet.Empty; queue.WorkExecuting = queue.WorkWaitingToExecute; - queue.WorkWaitingToExecute = ImmutableHashSet.Empty; + queue.WorkWaitingToExecute = ImmutableHashSet.Empty; return queue.WorkExecuting; } } @@ -89,7 +89,7 @@ public void WorkComplete(AnalyzerWorkType workType) _queues[workType].WorkPendingToken?.Cancel(); _queues[workType].WorkPendingToken = null; - _queues[workType].WorkExecuting = ImmutableHashSet.Empty; + _queues[workType].WorkExecuting = ImmutableHashSet.Empty; } } @@ -107,11 +107,11 @@ public Task WaitForegroundWorkComplete() .ContinueWith(task => LogTimeouts(task)); } - public bool TryPromote(DocumentId id) + public bool TryPromote(Document document) { - if (_queues[AnalyzerWorkType.Background].WorkWaitingToExecute.Contains(id) || _queues[AnalyzerWorkType.Background].WorkExecuting.Contains(id)) + if (_queues[AnalyzerWorkType.Background].WorkWaitingToExecute.Contains(document) || _queues[AnalyzerWorkType.Background].WorkExecuting.Contains(document)) { - PutWork(new[] { id }, AnalyzerWorkType.Foreground); + PutWork(new[] { document }, AnalyzerWorkType.Foreground); return true; } diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs index fc63dc1981..0f2ee4ba8a 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs @@ -145,6 +145,17 @@ public async Task> GetDiagnostics(ImmutableA .Select(docPath => _workspace.GetDocumentsFromFullProjectModelAsync(docPath))) ).SelectMany(s => s); + return await GetDiagnostics(documents); + } + + public Task> GetDiagnostics(ImmutableArray documents, bool skipCache) + { + return GetDiagnostics(documents); + } + + private async Task> GetDiagnostics(IEnumerable documents) + { + var results = new List(); foreach (var document in documents) { if(document?.Project?.Name == null) @@ -152,7 +163,7 @@ public async Task> GetDiagnostics(ImmutableA var projectName = document.Project.Name; var diagnostics = await GetDiagnosticsForDocument(document, projectName); - results.Add(new DocumentDiagnostics(document.Id, document.FilePath, document.Project.Id, document.Project.Name, diagnostics)); + results.Add(new DocumentDiagnostics(document, diagnostics)); } return results.ToImmutableArray(); @@ -178,14 +189,14 @@ public ImmutableArray QueueDocumentsForDiagnostics() { var documents = _workspace.CurrentSolution.Projects.SelectMany(x => x.Documents); QueueForDiagnosis(documents.Select(x => x.FilePath).ToImmutableArray()); - return documents.Select(x => x.Id).ToImmutableArray(); + return documents.Select(d => d.Id).ToImmutableArray(); } public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) { var documents = projectIds.SelectMany(projectId => _workspace.CurrentSolution.GetProject(projectId).Documents); QueueForDiagnosis(documents.Select(x => x.FilePath).ToImmutableArray()); - return documents.Select(x => x.Id).ToImmutableArray(); + return documents.Select(d => d.Id).ToImmutableArray(); } public Task> GetAllDiagnosticsAsync() diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index be2d7cbe08..68760742b4 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -9,7 +9,6 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; -using Microsoft.CodeAnalysis.Options; using Microsoft.Extensions.Logging; using OmniSharp.Helpers; using OmniSharp.Models.Diagnostics; @@ -17,6 +16,7 @@ using OmniSharp.Options; using OmniSharp.Roslyn.CSharp.Workers.Diagnostics; using OmniSharp.Services; +using OmniSharp.Utilities; namespace OmniSharp.Roslyn.CSharp.Services.Diagnostics { @@ -83,11 +83,37 @@ public async Task> GetDiagnostics(ImmutableA return await GetDiagnosticsByDocumentIds(documentIds, waitForDocuments: true); } - private async Task> GetDiagnosticsByDocumentIds(ImmutableArray documentIds, bool waitForDocuments) + public async Task> GetDiagnostics(ImmutableArray documents, bool skipCache) + { + if (skipCache) + { + var resultsBuilder = ImmutableArray.CreateBuilder(); + foreach (var grouping in documents.GroupBy(doc => doc.Project)) + { + var project = grouping.Key; + var analyzers = GetProjectAnalyzers(project); + var compilation = await project.GetCompilationAsync(); + var workspaceAnalyzerOptions = GetWorkspaceAnalyzerOptions(project); + + foreach (var document in grouping) + { + resultsBuilder.Add(new DocumentDiagnostics(document, await AnalyzeDocument(project, analyzers, compilation, workspaceAnalyzerOptions, document))); + } + } + + return resultsBuilder.ToImmutable(); + } + + var documentIds = documents.SelectAsArray(d => d.Id); + + return await GetDiagnosticsByDocumentIds(documents, waitForDocuments: true); + } + + private async Task> GetDiagnosticsByDocumentIds(ImmutableArray documents, bool waitForDocuments) { if (waitForDocuments) { - foreach (var documentId in documentIds) + foreach (var documentId in documents) { _workQueue.TryPromote(documentId); } @@ -95,16 +121,16 @@ private async Task> GetDiagnosticsByDocument await _workQueue.WaitForegroundWorkComplete(); } - return documentIds - .Where(x => _currentDiagnosticResultLookup.ContainsKey(x)) - .Select(x => _currentDiagnosticResultLookup[x]) + return documents + .Where(x => _currentDiagnosticResultLookup.ContainsKey(x.Id)) + .Select(x => _currentDiagnosticResultLookup[x.Id]) .ToImmutableArray(); } - private ImmutableArray GetDocumentIdsFromPaths(ImmutableArray documentPaths) + private ImmutableArray GetDocumentIdsFromPaths(ImmutableArray documentPaths) { return documentPaths - .Select(docPath => _workspace.GetDocumentId(docPath)) + .Select(docPath => _workspace.GetDocument(docPath)) .Where(x => x != default) .ToImmutableArray(); } @@ -119,9 +145,9 @@ private async Task Worker(AnalyzerWorkType workType) var currentWorkGroupedByProjects = _workQueue .TakeWork(workType) - .Select(documentId => (projectId: solution.GetDocument(documentId)?.Project?.Id, documentId)) + .Select(document => (projectId: document.Project?.Id, document)) .Where(x => x.projectId != null) - .GroupBy(x => x.projectId, x => x.documentId) + .GroupBy(x => x.projectId, x => x.document.Id) .ToImmutableArray(); foreach (var projectGroup in currentWorkGroupedByProjects) @@ -152,7 +178,7 @@ private void EventIfBackgroundWork(AnalyzerWorkType workType, string projectPath _forwarder.ProjectAnalyzedInBackground(projectPath, status); } - private void QueueForAnalysis(ImmutableArray documentIds, AnalyzerWorkType workType) + private void QueueForAnalysis(ImmutableArray documentIds, AnalyzerWorkType workType) { _workQueue.PutWork(documentIds, workType); } @@ -165,7 +191,7 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs changeEv case WorkspaceChangeKind.DocumentAdded: case WorkspaceChangeKind.DocumentReloaded: case WorkspaceChangeKind.DocumentInfoChanged: - QueueForAnalysis(ImmutableArray.Create(changeEvent.DocumentId), AnalyzerWorkType.Foreground); + QueueForAnalysis(ImmutableArray.Create(changeEvent.NewSolution.GetDocument(changeEvent.DocumentId)), AnalyzerWorkType.Foreground); break; case WorkspaceChangeKind.DocumentRemoved: if (!_currentDiagnosticResultLookup.TryRemove(changeEvent.DocumentId, out _)) @@ -177,7 +203,7 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs changeEv case WorkspaceChangeKind.ProjectChanged: case WorkspaceChangeKind.ProjectReloaded: _logger.LogDebug($"Project {changeEvent.ProjectId} updated, reanalyzing its diagnostics."); - var projectDocumentIds = _workspace.CurrentSolution.GetProject(changeEvent.ProjectId).Documents.Select(x => x.Id).ToImmutableArray(); + var projectDocumentIds = _workspace.CurrentSolution.GetProject(changeEvent.ProjectId).Documents.ToImmutableArray(); QueueForAnalysis(projectDocumentIds, AnalyzerWorkType.Background); break; case WorkspaceChangeKind.SolutionAdded: @@ -194,21 +220,16 @@ private async Task AnalyzeProject(Solution solution, IGrouping allAnalyzers = GetProjectAnalyzers(project); - var allAnalyzers = _providers - .SelectMany(x => x.CodeDiagnosticAnalyzerProviders) - .Concat(project.AnalyzerReferences.SelectMany(x => x.GetAnalyzers(project.Language))) - .ToImmutableArray(); - - var compiled = await project - .GetCompilationAsync(); + var compilation = await project.GetCompilationAsync(); - var workspaceAnalyzerOptions = (AnalyzerOptions) _workspaceAnalyzerOptionsConstructor.Invoke(new object[] {project.AnalyzerOptions, project.Solution}); + var workspaceAnalyzerOptions = GetWorkspaceAnalyzerOptions(project); foreach (var documentId in documentsGroupedByProject) { var document = project.GetDocument(documentId); - await AnalyzeDocument(project, allAnalyzers, compiled, workspaceAnalyzerOptions, document); + await AnalyzeAndUpdateDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); } } catch (Exception ex) @@ -217,56 +238,75 @@ private async Task AnalyzeProject(Solution solution, IGrouping allAnalyzers, Compilation compiled, AnalyzerOptions workspaceAnalyzerOptions, Document document) + private AnalyzerOptions GetWorkspaceAnalyzerOptions(Project project) + { + return (AnalyzerOptions)_workspaceAnalyzerOptionsConstructor.Invoke(new object[] { project.AnalyzerOptions, project.Solution }); + } + + private ImmutableArray GetProjectAnalyzers(Project project) + { + return _providers + .SelectMany(x => x.CodeDiagnosticAnalyzerProviders) + .Concat(project.AnalyzerReferences.SelectMany(x => x.GetAnalyzers(project.Language))) + .ToImmutableArray(); + } + + private async Task AnalyzeAndUpdateDocument(Project project, ImmutableArray allAnalyzers, Compilation compilation, AnalyzerOptions workspaceAnalyzerOptions, Document document) { try { - // There's real possibility that bug in analyzer causes analysis hang at document. - var perDocumentTimeout = - new CancellationTokenSource(_options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs); + ImmutableArray diagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); + UpdateCurrentDiagnostics(document, diagnostics); + } + catch (Exception ex) + { + _logger.LogError($"Analysis of document {document.Name} failed or cancelled by timeout: {ex.Message}, analysers: {string.Join(", ", allAnalyzers)}"); + } + } - var documentSemanticModel = await document.GetSemanticModelAsync(perDocumentTimeout.Token); + private async Task> AnalyzeDocument(Project project, ImmutableArray allAnalyzers, Compilation compilation, AnalyzerOptions workspaceAnalyzerOptions, Document document) + { + // There's real possibility that bug in analyzer causes analysis hang at document. + var perDocumentTimeout = + new CancellationTokenSource(_options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs); - var diagnostics = ImmutableArray.Empty; + var documentSemanticModel = await document.GetSemanticModelAsync(perDocumentTimeout.Token); - // Only basic syntax check is available if file is miscellanous like orphan .cs file. - // Those projects are on hard coded virtual project - if (project.Name == $"{Configuration.OmniSharpMiscProjectName}.csproj") - { - var syntaxTree = await document.GetSyntaxTreeAsync(); - diagnostics = syntaxTree.GetDiagnostics().ToImmutableArray(); - } - else if (allAnalyzers.Any()) // Analyzers cannot be called with empty analyzer list. - { - var compilationWithAnalyzers = compiled.WithAnalyzers(allAnalyzers, new CompilationWithAnalyzersOptions( - workspaceAnalyzerOptions, - onAnalyzerException: OnAnalyzerException, - concurrentAnalysis: false, - logAnalyzerExecutionTime: false, - reportSuppressedDiagnostics: false)); - - var semanticDiagnosticsWithAnalyzers = await compilationWithAnalyzers - .GetAnalyzerSemanticDiagnosticsAsync(documentSemanticModel, filterSpan: null, perDocumentTimeout.Token); - - var syntaxDiagnosticsWithAnalyzers = await compilationWithAnalyzers - .GetAnalyzerSyntaxDiagnosticsAsync(documentSemanticModel.SyntaxTree, perDocumentTimeout.Token); - - diagnostics = semanticDiagnosticsWithAnalyzers - .Concat(syntaxDiagnosticsWithAnalyzers) - .Concat(documentSemanticModel.GetDiagnostics()) - .ToImmutableArray(); - } - else - { - diagnostics = documentSemanticModel.GetDiagnostics().ToImmutableArray(); - } + var diagnostics = ImmutableArray.Empty; - UpdateCurrentDiagnostics(project, document, diagnostics); + // Only basic syntax check is available if file is miscellanous like orphan .cs file. + // Those projects are on hard coded virtual project + if (project.Name == $"{Configuration.OmniSharpMiscProjectName}.csproj") + { + var syntaxTree = await document.GetSyntaxTreeAsync(); + diagnostics = syntaxTree.GetDiagnostics().ToImmutableArray(); } - catch (Exception ex) + else if (allAnalyzers.Any()) // Analyzers cannot be called with empty analyzer list. { - _logger.LogError($"Analysis of document {document.Name} failed or cancelled by timeout: {ex.Message}, analysers: {string.Join(", ", allAnalyzers)}"); + var compilationWithAnalyzers = compilation.WithAnalyzers(allAnalyzers, new CompilationWithAnalyzersOptions( + workspaceAnalyzerOptions, + onAnalyzerException: OnAnalyzerException, + concurrentAnalysis: false, + logAnalyzerExecutionTime: false, + reportSuppressedDiagnostics: false)); + + var semanticDiagnosticsWithAnalyzers = await compilationWithAnalyzers + .GetAnalyzerSemanticDiagnosticsAsync(documentSemanticModel, filterSpan: null, perDocumentTimeout.Token); + + var syntaxDiagnosticsWithAnalyzers = await compilationWithAnalyzers + .GetAnalyzerSyntaxDiagnosticsAsync(documentSemanticModel.SyntaxTree, perDocumentTimeout.Token); + + diagnostics = semanticDiagnosticsWithAnalyzers + .Concat(syntaxDiagnosticsWithAnalyzers) + .Concat(documentSemanticModel.GetDiagnostics()) + .ToImmutableArray(); + } + else + { + diagnostics = documentSemanticModel.GetDiagnostics().ToImmutableArray(); } + + return diagnostics; } private void OnAnalyzerException(Exception ex, DiagnosticAnalyzer analyzer, Diagnostic diagnostic) @@ -277,9 +317,9 @@ private void OnAnalyzerException(Exception ex, DiagnosticAnalyzer analyzer, Diag $"\n exception: {ex.Message}"); } - private void UpdateCurrentDiagnostics(Project project, Document document, ImmutableArray diagnosticsWithAnalyzers) + private void UpdateCurrentDiagnostics(Document document, ImmutableArray diagnosticsWithAnalyzers) { - _currentDiagnosticResultLookup[document.Id] = new DocumentDiagnostics(document.Id, document.FilePath, project.Id, project.Name, diagnosticsWithAnalyzers); + _currentDiagnosticResultLookup[document.Id] = new DocumentDiagnostics(document, diagnosticsWithAnalyzers); EmitDiagnostics(_currentDiagnosticResultLookup[document.Id]); } @@ -291,7 +331,7 @@ private void EmitDiagnostics(DocumentDiagnostics results) { new DiagnosticResult { - FileName = results.DocumentPath, QuickFixes = results.Diagnostics + FileName = results.Document.FilePath, QuickFixes = results.Diagnostics .Select(x => x.ToDiagnosticLocation()) .ToList() } @@ -301,24 +341,24 @@ private void EmitDiagnostics(DocumentDiagnostics results) public ImmutableArray QueueDocumentsForDiagnostics() { - var documentIds = _workspace.CurrentSolution.Projects.SelectMany(x => x.DocumentIds).ToImmutableArray(); - QueueForAnalysis(documentIds, AnalyzerWorkType.Background); - return documentIds; + var documents = _workspace.CurrentSolution.Projects.SelectMany(x => x.Documents).ToImmutableArray(); + QueueForAnalysis(documents, AnalyzerWorkType.Background); + return documents.SelectAsArray(d => d.Id); } public async Task> GetAllDiagnosticsAsync() { - var allDocumentsIds = _workspace.CurrentSolution.Projects.SelectMany(x => x.DocumentIds).ToImmutableArray(); - return await GetDiagnosticsByDocumentIds(allDocumentsIds, waitForDocuments: false); + var allDocuments = _workspace.CurrentSolution.Projects.SelectMany(x => x.Documents).ToImmutableArray(); + return await GetDiagnosticsByDocumentIds(allDocuments, waitForDocuments: false); } public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) { var documentIds = projectIds - .SelectMany(projectId => _workspace.CurrentSolution.GetProject(projectId).Documents.Select(x => x.Id)) + .SelectMany(projectId => _workspace.CurrentSolution.GetProject(projectId).Documents) .ToImmutableArray(); QueueForAnalysis(documentIds, AnalyzerWorkType.Background); - return documentIds; + return documentIds.SelectAsArray(d => d.Id); } public void Dispose() diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs index 9916347c3c..ae1a442e12 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs @@ -77,6 +77,11 @@ public Task> GetDiagnostics(ImmutableArray> GetDiagnostics(ImmutableArray documents, bool skipCache) + { + return _implementation.GetDiagnostics(documents, skipCache); + } + public ImmutableArray QueueDocumentsForDiagnostics() { return _implementation.QueueDocumentsForDiagnostics(); diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/DocumentDiagnostics.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/DocumentDiagnostics.cs index d4b7d2c6c2..7b23396866 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/DocumentDiagnostics.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/DocumentDiagnostics.cs @@ -5,19 +5,14 @@ namespace OmniSharp.Roslyn.CSharp.Services.Diagnostics { public class DocumentDiagnostics { - public DocumentDiagnostics(DocumentId documentId, string documentPath, ProjectId projectId, string projectName, ImmutableArray diagnostics) + public DocumentDiagnostics(Document document, ImmutableArray diagnostics) { - DocumentId = documentId; - DocumentPath = documentPath; - ProjectId = projectId; - ProjectName = projectName; Diagnostics = diagnostics; + Document = document; } - public DocumentId DocumentId { get; } - public ProjectId ProjectId { get; } - public string ProjectName { get; } - public string DocumentPath { get; } + public Document Document { get; } + public Project Project => Document.Project; public ImmutableArray Diagnostics { get; } } } diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs index 2297d04b3f..50ebef18b3 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs @@ -8,8 +8,9 @@ namespace OmniSharp.Roslyn.CSharp.Workers.Diagnostics public interface ICsDiagnosticWorker { Task> GetDiagnostics(ImmutableArray documentPaths); + Task> GetDiagnostics(ImmutableArray documents, bool skipCache); Task> GetAllDiagnosticsAsync(); ImmutableArray QueueDocumentsForDiagnostics(); ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectId); } -} \ No newline at end of file +} diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs index 7742c82186..9d46410252 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs @@ -6,12 +6,19 @@ using Microsoft.CodeAnalysis; using Microsoft.Extensions.Logging; using OmniSharp.Roslyn.CSharp.Workers.Diagnostics; +using TestUtility; using Xunit; +using Xunit.Abstractions; namespace OmniSharp.Roslyn.CSharp.Tests { - public class AnalyzerWorkerQueueFacts + public class AnalyzerWorkerQueueFacts : AbstractTestFixture { + public AnalyzerWorkerQueueFacts(ITestOutputHelper output, SharedOmniSharpHostFixture sharedOmniSharpHostFixture) + : base(output, sharedOmniSharpHostFixture) + { + } + private class Logger : ILogger { public IDisposable BeginScope(TState state) @@ -29,7 +36,7 @@ public void Log(LogLevel logLevel, EventId eventId, TState state, Except public ImmutableArray RecordedMessages { get; set; } = ImmutableArray.Create(); } - private class LoggerFactory : ILoggerFactory + private class TestLoggerFactory : ILoggerFactory { public Logger Logger { get; } = new Logger(); @@ -53,7 +60,7 @@ public void Dispose() public void WhenItemsAreAddedButThrotlingIsntOverNoWorkShouldBeReturned(AnalyzerWorkType workType) { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, workType); @@ -66,7 +73,7 @@ public void WhenItemsAreAddedButThrotlingIsntOverNoWorkShouldBeReturned(Analyzer public void WhenWorksIsAddedToQueueThenTheyWillBeReturned(AnalyzerWorkType workType) { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, workType); @@ -84,7 +91,7 @@ public void WhenWorksIsAddedToQueueThenTheyWillBeReturned(AnalyzerWorkType workT public void WhenSameItemIsAddedMultipleTimesInRowThenThrottleItemAsOne(AnalyzerWorkType workType) { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, workType); @@ -105,7 +112,7 @@ public void WhenSameItemIsAddedMultipleTimesInRowThenThrottleItemAsOne(AnalyzerW public void WhenForegroundWorkIsAddedThenWaitNextIterationOfItReady() { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 500); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 500); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); @@ -127,7 +134,7 @@ public void WhenForegroundWorkIsAddedThenWaitNextIterationOfItReady() public void WhenForegroundWorkIsUnderAnalysisOutFromQueueThenWaitUntilNextIterationOfItIsReady() { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 500); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 500); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); @@ -149,7 +156,7 @@ public void WhenForegroundWorkIsUnderAnalysisOutFromQueueThenWaitUntilNextIterat public void WhenWorkIsWaitedButTimeoutForWaitIsExceededAllowContinue() { var now = DateTime.UtcNow; - var loggerFactory = new LoggerFactory(); + var loggerFactory = new TestLoggerFactory(); var queue = new AnalyzerWorkQueue(loggerFactory, utcNow: () => now, timeoutForPendingWorkMs: 20); var document = CreateTestDocumentId(); @@ -172,7 +179,7 @@ public async Task WhenMultipleThreadsAreConsumingAnalyzerWorkerQueueItWorksAsExp { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 1000); var parallelQueues = Enumerable.Range(0, 10) @@ -204,7 +211,7 @@ public async Task WhenMultipleThreadsAreConsumingAnalyzerWorkerQueueItWorksAsExp public async Task WhenWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnotherOneToGetReady() { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); @@ -232,7 +239,7 @@ public async Task WhenWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnothe [Fact] public void WhenBackgroundWorkIsAdded_DontWaitIt() { - var queue = new AnalyzerWorkQueue(new LoggerFactory(), timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, AnalyzerWorkType.Background); @@ -244,7 +251,7 @@ public void WhenBackgroundWorkIsAdded_DontWaitIt() public void WhenSingleFileIsPromoted_ThenPromoteItFromBackgroundQueueToForeground() { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, AnalyzerWorkType.Background); @@ -260,7 +267,7 @@ public void WhenSingleFileIsPromoted_ThenPromoteItFromBackgroundQueueToForegroun public void WhenFileIsntAtBackgroundQueueAndTriedToBePromoted_ThenDontDoNothing() { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.TryPromote(document); @@ -274,7 +281,7 @@ public void WhenFileIsntAtBackgroundQueueAndTriedToBePromoted_ThenDontDoNothing( public void WhenFileIsProcessingInBackgroundQueue_ThenPromoteItAsForeground() { var now = DateTime.UtcNow; - var queue = new AnalyzerWorkQueue(new LoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); + var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); queue.PutWork(new[] { document }, AnalyzerWorkType.Background); @@ -293,16 +300,12 @@ public void WhenFileIsProcessingInBackgroundQueue_ThenPromoteItAsForeground() Assert.NotEmpty(activeWork); } - private DocumentId CreateTestDocumentId() + private Document CreateTestDocumentId() { - var projectInfo = ProjectInfo.Create( - id: ProjectId.CreateNewId(), - version: VersionStamp.Create(), - name: "testProject", - assemblyName: "AssemblyName", - language: LanguageNames.CSharp); - - return DocumentId.CreateNewId(projectInfo.Id); + string fileName = $"{Guid.NewGuid()}"; + var newFile = new TestFile(fileName, ""); + _ = SharedOmniSharpTestHost.AddFilesToWorkspace(newFile).Single(); + return SharedOmniSharpTestHost.Workspace.GetDocument(fileName); } } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs index 2d7bb81940..a5ef78ffd3 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs @@ -47,7 +47,7 @@ public async Task WhenFileContainsFixableIssuesWithAnalyzersEnabled_ThenFixThemA }); string textAfterFix = await GetContentOfDocumentFromWorkspace(host, testFilePath); - AssertUtils.AssertIgnoringIndent(textAfterFix, expectedText); + AssertUtils.AssertIgnoringIndent(expectedText, textAfterFix); var internalClassChange = response.Changes.OfType().Single().Changes.Single(x => x.NewText == "internal "); @@ -77,16 +77,13 @@ class C{} // If filtering isn't set, this should also add 'internal' etc which // should not appear now as result. - var expectedText = - @" - class C { } - "; + var expectedText = "\nclass C { }\n"; var testFilePath = CreateTestProjectWithDocument(host, originalText); var handler = host.GetRequestHandler(OmniSharpEndpoints.RunFixAll); - await handler.Handle(new RunFixAllRequest + var response = await handler.Handle(new RunFixAllRequest { Scope = FixAllScope.Document, FileName = testFilePath, @@ -97,14 +94,16 @@ await handler.Handle(new RunFixAllRequest string textAfterFix = await GetContentOfDocumentFromWorkspace(host, testFilePath); - AssertUtils.AssertIgnoringIndent(expectedText, textAfterFix); + Assert.Equal(expectedText, textAfterFix); + + Assert.Equal(expectedText, ((ModifiedFileResponse)response.Changes.Single()).Changes.Single().NewText); } } [Theory] [InlineData(FixAllScope.Document)] [InlineData(FixAllScope.Project)] - public async Task WhenFixAllIsScopedToDocumentAndProject_ThenOnlyFixInScopeInsteadOfEverything(FixAllScope scope) + public async Task WhenFixAllIsScopedToDocumentAndProject_ThenOnlyFixInScopeInsteadOfEverything_DifferentProjects(FixAllScope scope) { using (var host = GetHost(true)) { @@ -141,12 +140,106 @@ await handler.Handle(new RunFixAllRequest } } - [Fact(Skip = @"Fails on windows only inside roslyn -System.ArgumentOutOfRangeException -Specified argument was out of the range of valid values. -Parameter name: start -... -")] + [Fact] + public async Task WhenFixAllIsScopedToDocumentAndProject_ThenOnlyFixInScopeInsteadOfEverything_DifferentDocuments() + { + const string UnformattedString = @" +partial class C {} +"; + + const string FormattedString = @" +internal partial class C {} +"; + + const string File1 = "file1.cs"; + const string File2 = "file2.cs"; + + using var host = GetHost(true); + var projectId = host.AddFilesToWorkspace(new TestFile(File1, UnformattedString), new TestFile(File2, UnformattedString)).First(); + var project = host.Workspace.CurrentSolution.GetProject(projectId); + + var handler = host.GetRequestHandler(OmniSharpEndpoints.RunFixAll); + + await handler.Handle(new RunFixAllRequest + { + Scope = FixAllScope.Document, + FileName = File1, + FixAllFilter = new[] { new FixAllItem("IDE0040", "Fix formatting") }, + WantsTextChanges = true, + WantsAllCodeActionOperations = true + }); + + var file1Content = await GetContentOfDocumentFromWorkspace(host, File1); + Assert.Equal(FormattedString, file1Content); + + var file2Content = await GetContentOfDocumentFromWorkspace(host, File2); + Assert.Equal(UnformattedString, file2Content); + } + + [Fact] + public async Task WhenFixAllIsScopedToSolution_ThenFixInSolution() + { + + const string UnformattedString = @" +partial class C {} +"; + + const string FormattedString = @" +internal partial class C {} +"; + + using var host = GetHost(true); + var file1 = CreateTestProjectWithDocument(host, UnformattedString); + var file2 = CreateTestProjectWithDocument(host, UnformattedString); + + var handler = host.GetRequestHandler(OmniSharpEndpoints.RunFixAll); + + await handler.Handle(new RunFixAllRequest + { + Scope = FixAllScope.Solution, + FileName = file1, + FixAllFilter = new[] { new FixAllItem("IDE0040", "Fix formatting") }, + WantsTextChanges = true, + WantsAllCodeActionOperations = true + }); + + var file1Content = await GetContentOfDocumentFromWorkspace(host, file1); + Assert.Equal(FormattedString, file1Content); + + var file2Content = await GetContentOfDocumentFromWorkspace(host, file2); + Assert.Equal(FormattedString, file2Content); + } + + [Fact] + public async Task IntersectingFixAllLocations_ApplyCorrectly() + { + using var host = GetHost(true); + + var file = CreateTestProjectWithDocument(host, @" +class C +{ + object o = 1; +}"); + + var handler = host.GetRequestHandler(OmniSharpEndpoints.RunFixAll); + + await handler.Handle(new RunFixAllRequest + { + Scope = FixAllScope.Document, + FileName = file, + WantsTextChanges = true, + WantsAllCodeActionOperations = true + }); + + var file1Content = await GetContentOfDocumentFromWorkspace(host, file); + Assert.Equal(@" +internal class C +{ + private readonly object o = 1; +}", file1Content); + } + + [Fact] // This is specifically tested because has custom mapping logic in it. public async Task WhenTextContainsUnusedImports_ThenTheyCanBeAutomaticallyFixed() { @@ -295,6 +388,32 @@ await Assert.ThrowsAsync(async () => await handler.Hand } } + [Fact] + public async Task WhenApplyChangesIsFalseChangesAreNotApplied() + { + const string OriginalContent = "class C { }"; + const string ExpectedResponseContent = "internal "; + using var host = GetHost(roslynAnalyzersEnabled: true); + var file = CreateTestProjectWithDocument(host, OriginalContent); + + var handler = host.GetRequestHandler(OmniSharpEndpoints.RunFixAll); + + var response = await handler.Handle(new RunFixAllRequest + { + Scope = FixAllScope.Document, + FileName = file, + WantsTextChanges = true, + WantsAllCodeActionOperations = true, + FixAllFilter = null, // This means: try fix everything. + ApplyChanges = false + }); + + string textAfterFix = await GetContentOfDocumentFromWorkspace(host, file); + Assert.Equal(OriginalContent, textAfterFix); + + Assert.Equal(ExpectedResponseContent, ((ModifiedFileResponse)response.Changes.Single()).Changes.Single().NewText); + } + private OmniSharpTestHost GetHost(bool roslynAnalyzersEnabled) { return OmniSharpTestHost.Create( From 92084b933a7648020b3eeb84ea0e3e0cab8fb034 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Sun, 27 Sep 2020 23:32:51 -0700 Subject: [PATCH 2/5] Remove platform-dependent newline. --- tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs index a5ef78ffd3..545132bed9 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/FixAllFacts.cs @@ -70,14 +70,10 @@ public async Task WhenFixAllItemsAreDefinedByFilter_ThenFixOnlyFilteredItems() { using (var host = GetHost(true)) { - var originalText = - @" - class C{} - "; + var originalText = " class C{}"; // If filtering isn't set, this should also add 'internal' etc which // should not appear now as result. - var expectedText = "\nclass C { }\n"; var testFilePath = CreateTestProjectWithDocument(host, originalText); @@ -94,9 +90,9 @@ class C{} string textAfterFix = await GetContentOfDocumentFromWorkspace(host, testFilePath); - Assert.Equal(expectedText, textAfterFix); + Assert.Equal("class C { }", textAfterFix); - Assert.Equal(expectedText, ((ModifiedFileResponse)response.Changes.Single()).Changes.Single().NewText); + Assert.Equal("class C { ", ((ModifiedFileResponse)response.Changes.Single()).Changes.Single().NewText); } } From a0c3625d83908f176b41b462e6c07d6ad72c7f88 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Tue, 29 Sep 2020 22:02:16 -0700 Subject: [PATCH 3/5] Rename documentid to document in a few places. --- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 68760742b4..252643df0c 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -78,9 +78,9 @@ public void OnWorkspaceInitialized(bool isInitialized) public async Task> GetDiagnostics(ImmutableArray documentPaths) { - var documentIds = GetDocumentIdsFromPaths(documentPaths); + var documentIds = GetDocumentFromPaths(documentPaths); - return await GetDiagnosticsByDocumentIds(documentIds, waitForDocuments: true); + return await GetDiagnosticsByDocument(documentIds, waitForDocuments: true); } public async Task> GetDiagnostics(ImmutableArray documents, bool skipCache) @@ -106,10 +106,10 @@ public async Task> GetDiagnostics(ImmutableA var documentIds = documents.SelectAsArray(d => d.Id); - return await GetDiagnosticsByDocumentIds(documents, waitForDocuments: true); + return await GetDiagnosticsByDocument(documents, waitForDocuments: true); } - private async Task> GetDiagnosticsByDocumentIds(ImmutableArray documents, bool waitForDocuments) + private async Task> GetDiagnosticsByDocument(ImmutableArray documents, bool waitForDocuments) { if (waitForDocuments) { @@ -127,7 +127,7 @@ private async Task> GetDiagnosticsByDocument .ToImmutableArray(); } - private ImmutableArray GetDocumentIdsFromPaths(ImmutableArray documentPaths) + private ImmutableArray GetDocumentFromPaths(ImmutableArray documentPaths) { return documentPaths .Select(docPath => _workspace.GetDocument(docPath)) @@ -349,16 +349,16 @@ public ImmutableArray QueueDocumentsForDiagnostics() public async Task> GetAllDiagnosticsAsync() { var allDocuments = _workspace.CurrentSolution.Projects.SelectMany(x => x.Documents).ToImmutableArray(); - return await GetDiagnosticsByDocumentIds(allDocuments, waitForDocuments: false); + return await GetDiagnosticsByDocument(allDocuments, waitForDocuments: false); } public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) { - var documentIds = projectIds + var documents = projectIds .SelectMany(projectId => _workspace.CurrentSolution.GetProject(projectId).Documents) .ToImmutableArray(); - QueueForAnalysis(documentIds, AnalyzerWorkType.Background); - return documentIds.SelectAsArray(d => d.Id); + QueueForAnalysis(documents, AnalyzerWorkType.Background); + return documents.SelectAsArray(d => d.Id); } public void Dispose() From 582c818b0e58d54c7a16885ceeb45794fd1e17b2 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 1 Oct 2020 23:02:34 -0700 Subject: [PATCH 4/5] Move the diagnostic worker to cache Document->Diagnostics, rather than DocumentId->Diagnostics. This is done via a ConditionalWeakTable to ensure we don't root Documents and prevent garbage collection. --- .../Refactoring/RunFixAllCodeActionService.cs | 6 +- .../Refactoring/V2/BaseCodeActionService.cs | 10 +- .../Workers/Diagnostics/AnalyzerWorkQueue.cs | 12 +- .../Diagnostics/CSharpDiagnosticWorker.cs | 14 +- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 141 +++++++++--------- .../CsharpDiagnosticWorkerComposer.cs | 8 +- .../Diagnostics/ICsDiagnosticWorker.cs | 6 +- .../AnalyzerWorkerQueueFacts.cs | 12 +- 8 files changed, 102 insertions(+), 107 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs index 4d55a43f29..50ec4cdb02 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs @@ -193,13 +193,13 @@ public FixAllDiagnosticProvider(ICsDiagnosticWorker diagnosticWorker) public override async Task> GetAllDiagnosticsAsync(Project project, CancellationToken cancellationToken) { - var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.ToImmutableArray(), skipCache: true); + var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.ToImmutableArray()); return diagnostics.SelectMany(x => x.Diagnostics); } public override async Task> GetDocumentDiagnosticsAsync(Document document, CancellationToken cancellationToken) { - var documentDiagnostics = await _diagnosticWorker.GetDiagnostics(ImmutableArray.Create(document), skipCache: true); + var documentDiagnostics = await _diagnosticWorker.GetDiagnostics(ImmutableArray.Create(document)); if (!documentDiagnostics.Any()) return new Diagnostic[] { }; @@ -209,7 +209,7 @@ public override async Task> GetDocumentDiagnosticsAsync( public override async Task> GetProjectDiagnosticsAsync(Project project, CancellationToken cancellationToken) { - var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.ToImmutableArray(), skipCache: true); + var diagnostics = await _diagnosticWorker.GetDiagnostics(project.Documents.ToImmutableArray()); return diagnostics.SelectMany(x => x.Diagnostics); } } diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs index 3b128d5836..71576c0c54 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs @@ -127,7 +127,7 @@ private TextSpan GetTextSpan(ICodeActionRequest request, SourceText sourceText) private async Task CollectCodeFixesActions(Document document, TextSpan span, List codeActions) { - var diagnosticsWithProjects = await _diagnostics.GetDiagnostics(ImmutableArray.Create(document), skipCache: false); + var diagnosticsWithProjects = await _diagnostics.GetDiagnostics(ImmutableArray.Create(document)); var groupedBySpan = diagnosticsWithProjects .SelectMany(x => x.Diagnostics) @@ -248,12 +248,12 @@ protected async Task> GetDiagnosticsAsync(Fi { case FixAllScope.Solution: var documentsInSolution = document.Project.Solution.Projects.SelectMany(p => p.Documents).ToImmutableArray(); - return await _diagnostics.GetDiagnostics(documentsInSolution, skipCache: true); + return await _diagnostics.GetDiagnostics(documentsInSolution); case FixAllScope.Project: - var documensInProject = document.Project.Documents.ToImmutableArray(); - return await _diagnostics.GetDiagnostics(documensInProject, skipCache: true); + var documentsInProject = document.Project.Documents.ToImmutableArray(); + return await _diagnostics.GetDiagnostics(documentsInProject); case FixAllScope.Document: - return await _diagnostics.GetDiagnostics(ImmutableArray.Create(document), skipCache: true); + return await _diagnostics.GetDiagnostics(ImmutableArray.Create(document)); default: throw new InvalidOperationException(); } diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs index 7a82148abf..6f1e818e83 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AnalyzerWorkQueue.cs @@ -84,7 +84,7 @@ public void WorkComplete(AnalyzerWorkType workType) { lock (_queueLock) { - if(_queues[workType].WorkExecuting.IsEmpty) + if (_queues[workType].WorkExecuting.IsEmpty) return; _queues[workType].WorkPendingToken?.Cancel(); @@ -107,15 +107,9 @@ public Task WaitForegroundWorkComplete() .ContinueWith(task => LogTimeouts(task)); } - public bool TryPromote(Document document) + public void QueueDocumentForeground(Document document) { - if (_queues[AnalyzerWorkType.Background].WorkWaitingToExecute.Contains(document) || _queues[AnalyzerWorkType.Background].WorkExecuting.Contains(document)) - { - PutWork(new[] { document }, AnalyzerWorkType.Foreground); - return true; - } - - return false; + PutWork(new[] { document }, AnalyzerWorkType.Foreground); } private void LogTimeouts(Task task) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs index 0f2ee4ba8a..fb4b3633c6 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs @@ -148,7 +148,7 @@ public async Task> GetDiagnostics(ImmutableA return await GetDiagnostics(documents); } - public Task> GetDiagnostics(ImmutableArray documents, bool skipCache) + public Task> GetDiagnostics(ImmutableArray documents) { return GetDiagnostics(documents); } @@ -185,18 +185,18 @@ private static async Task> GetDiagnosticsForDocument( } } - public ImmutableArray QueueDocumentsForDiagnostics() + public ImmutableArray QueueDocumentsForDiagnostics() { - var documents = _workspace.CurrentSolution.Projects.SelectMany(x => x.Documents); + var documents = _workspace.CurrentSolution.Projects.SelectMany(x => x.Documents).ToImmutableArray(); QueueForDiagnosis(documents.Select(x => x.FilePath).ToImmutableArray()); - return documents.Select(d => d.Id).ToImmutableArray(); + return documents; } - public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) + public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) { - var documents = projectIds.SelectMany(projectId => _workspace.CurrentSolution.GetProject(projectId).Documents); + var documents = projectIds.SelectMany(projectId => _workspace.CurrentSolution.GetProject(projectId).Documents).ToImmutableArray(); QueueForDiagnosis(documents.Select(x => x.FilePath).ToImmutableArray()); - return documents.Select(d => d.Id).ToImmutableArray(); + return documents; } public Task> GetAllDiagnosticsAsync() diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 252643df0c..13898530a1 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -1,10 +1,11 @@ using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; +using System.Diagnostics; using System.Linq; using System.Reflection; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; @@ -16,7 +17,6 @@ using OmniSharp.Options; using OmniSharp.Roslyn.CSharp.Workers.Diagnostics; using OmniSharp.Services; -using OmniSharp.Utilities; namespace OmniSharp.Roslyn.CSharp.Services.Diagnostics { @@ -25,8 +25,8 @@ public class CSharpDiagnosticWorkerWithAnalyzers : ICsDiagnosticWorker, IDisposa private readonly AnalyzerWorkQueue _workQueue; private readonly ILogger _logger; - private readonly ConcurrentDictionary _currentDiagnosticResultLookup = - new ConcurrentDictionary(); + private readonly ConditionalWeakTable _currentDiagnosticResultLookup = + new ConditionalWeakTable(); private readonly ImmutableArray _providers; private readonly DiagnosticEventForwarder _forwarder; private readonly OmniSharpOptions _options; @@ -71,68 +71,68 @@ public void OnWorkspaceInitialized(bool isInitialized) { if (isInitialized) { - var documentIds = QueueDocumentsForDiagnostics(); - _logger.LogInformation($"Solution initialized -> queue all documents for code analysis. Initial document count: {documentIds.Length}."); + var documents = QueueDocumentsForDiagnostics(); + _logger.LogInformation($"Solution initialized -> queue all documents for code analysis. Initial document count: {documents.Length}."); } } public async Task> GetDiagnostics(ImmutableArray documentPaths) { - var documentIds = GetDocumentFromPaths(documentPaths); + var documents = documentPaths + .Select(docPath => _workspace.GetDocument(docPath)) + .Where(x => x != default) + .ToImmutableArray(); + return await GetDiagnosticsByDocument(documents, waitForDocuments: true); + } - return await GetDiagnosticsByDocument(documentIds, waitForDocuments: true); + public async Task> GetDiagnostics(ImmutableArray documents) + { + return await GetDiagnosticsByDocument(documents, waitForDocuments: true); } - public async Task> GetDiagnostics(ImmutableArray documents, bool skipCache) + private async Task> GetDiagnosticsByDocument(ImmutableArray documents, bool waitForDocuments) { - if (skipCache) + if (documents.IsDefaultOrEmpty) return ImmutableArray.Empty; + + ImmutableArray.Builder resultsBuilder = ImmutableArray.CreateBuilder(documents.Length); + resultsBuilder.Count = documents.Length; + + bool foundAll = true; + + for (int i = 0; i < documents.Length; i++) { - var resultsBuilder = ImmutableArray.CreateBuilder(); - foreach (var grouping in documents.GroupBy(doc => doc.Project)) + if (_currentDiagnosticResultLookup.TryGetValue(documents[i], out var diagnostics)) { - var project = grouping.Key; - var analyzers = GetProjectAnalyzers(project); - var compilation = await project.GetCompilationAsync(); - var workspaceAnalyzerOptions = GetWorkspaceAnalyzerOptions(project); - - foreach (var document in grouping) - { - resultsBuilder.Add(new DocumentDiagnostics(document, await AnalyzeDocument(project, analyzers, compilation, workspaceAnalyzerOptions, document))); - } + resultsBuilder[i] = diagnostics; + } + else + { + _workQueue.QueueDocumentForeground(documents[i]); + foundAll = false; } - - return resultsBuilder.ToImmutable(); } - var documentIds = documents.SelectAsArray(d => d.Id); + if (foundAll) + { + return resultsBuilder.MoveToImmutable(); + } - return await GetDiagnosticsByDocument(documents, waitForDocuments: true); - } + await _workQueue.WaitForegroundWorkComplete(); - private async Task> GetDiagnosticsByDocument(ImmutableArray documents, bool waitForDocuments) - { - if (waitForDocuments) + for (int i = 0; i < documents.Length; i++) { - foreach (var documentId in documents) + if (_currentDiagnosticResultLookup.TryGetValue(documents[i], out var diagnostics)) { - _workQueue.TryPromote(documentId); + resultsBuilder[i] = diagnostics; + } + else + { + Debug.Fail("Should have diagnostics after waiting for work"); + resultsBuilder[i] = new DocumentDiagnostics(documents[i], ImmutableArray.Empty); } - - await _workQueue.WaitForegroundWorkComplete(); } - return documents - .Where(x => _currentDiagnosticResultLookup.ContainsKey(x.Id)) - .Select(x => _currentDiagnosticResultLookup[x.Id]) - .ToImmutableArray(); - } - - private ImmutableArray GetDocumentFromPaths(ImmutableArray documentPaths) - { - return documentPaths - .Select(docPath => _workspace.GetDocument(docPath)) - .Where(x => x != default) - .ToImmutableArray(); + return resultsBuilder.MoveToImmutable(); } private async Task Worker(AnalyzerWorkType workType) @@ -141,22 +141,19 @@ private async Task Worker(AnalyzerWorkType workType) { try { - var solution = _workspace.CurrentSolution; - var currentWorkGroupedByProjects = _workQueue .TakeWork(workType) - .Select(document => (projectId: document.Project?.Id, document)) - .Where(x => x.projectId != null) - .GroupBy(x => x.projectId, x => x.document.Id) + .Select(document => (project: document.Project, document)) + .GroupBy(x => x.project, x => x.document) .ToImmutableArray(); foreach (var projectGroup in currentWorkGroupedByProjects) { - var projectPath = solution.GetProject(projectGroup.Key).FilePath; + var projectPath = projectGroup.Key.FilePath; EventIfBackgroundWork(workType, projectPath, ProjectDiagnosticStatus.Started); - await AnalyzeProject(solution, projectGroup); + await AnalyzeProject(projectGroup); EventIfBackgroundWork(workType, projectPath, ProjectDiagnosticStatus.Ready); } @@ -178,9 +175,9 @@ private void EventIfBackgroundWork(AnalyzerWorkType workType, string projectPath _forwarder.ProjectAnalyzedInBackground(projectPath, status); } - private void QueueForAnalysis(ImmutableArray documentIds, AnalyzerWorkType workType) + private void QueueForAnalysis(ImmutableArray documents, AnalyzerWorkType workType) { - _workQueue.PutWork(documentIds, workType); + _workQueue.PutWork(documents, workType); } private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs changeEvent) @@ -194,17 +191,13 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs changeEv QueueForAnalysis(ImmutableArray.Create(changeEvent.NewSolution.GetDocument(changeEvent.DocumentId)), AnalyzerWorkType.Foreground); break; case WorkspaceChangeKind.DocumentRemoved: - if (!_currentDiagnosticResultLookup.TryRemove(changeEvent.DocumentId, out _)) - { - _logger.LogDebug($"Tried to remove non existent document from analysis, document: {changeEvent.DocumentId}"); - } break; case WorkspaceChangeKind.ProjectAdded: case WorkspaceChangeKind.ProjectChanged: case WorkspaceChangeKind.ProjectReloaded: _logger.LogDebug($"Project {changeEvent.ProjectId} updated, reanalyzing its diagnostics."); - var projectDocumentIds = _workspace.CurrentSolution.GetProject(changeEvent.ProjectId).Documents.ToImmutableArray(); - QueueForAnalysis(projectDocumentIds, AnalyzerWorkType.Background); + var projectDocuments = _workspace.CurrentSolution.GetProject(changeEvent.ProjectId).Documents.ToImmutableArray(); + QueueForAnalysis(projectDocuments, AnalyzerWorkType.Background); break; case WorkspaceChangeKind.SolutionAdded: case WorkspaceChangeKind.SolutionChanged: @@ -215,20 +208,19 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs changeEv } } - private async Task AnalyzeProject(Solution solution, IGrouping documentsGroupedByProject) + private async Task AnalyzeProject(IGrouping documentsGroupedByProject) { try { - var project = solution.GetProject(documentsGroupedByProject.Key); + var project = documentsGroupedByProject.Key; ImmutableArray allAnalyzers = GetProjectAnalyzers(project); var compilation = await project.GetCompilationAsync(); var workspaceAnalyzerOptions = GetWorkspaceAnalyzerOptions(project); - foreach (var documentId in documentsGroupedByProject) + foreach (var document in documentsGroupedByProject) { - var document = project.GetDocument(documentId); await AnalyzeAndUpdateDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); } } @@ -319,8 +311,17 @@ private void OnAnalyzerException(Exception ex, DiagnosticAnalyzer analyzer, Diag private void UpdateCurrentDiagnostics(Document document, ImmutableArray diagnosticsWithAnalyzers) { - _currentDiagnosticResultLookup[document.Id] = new DocumentDiagnostics(document, diagnosticsWithAnalyzers); - EmitDiagnostics(_currentDiagnosticResultLookup[document.Id]); + DocumentDiagnostics documentDiagnostics = new DocumentDiagnostics(document, diagnosticsWithAnalyzers); + try + { + _currentDiagnosticResultLookup.Add(document, documentDiagnostics); + } + catch (ArgumentException) + { + // The work for this document was already done. Solutions (and by extension Documents) are immutable, + // so this is fine to silently swallow, as we'll get the same results every time. + } + EmitDiagnostics(documentDiagnostics); } private void EmitDiagnostics(DocumentDiagnostics results) @@ -339,11 +340,11 @@ private void EmitDiagnostics(DocumentDiagnostics results) }); } - public ImmutableArray QueueDocumentsForDiagnostics() + public ImmutableArray QueueDocumentsForDiagnostics() { var documents = _workspace.CurrentSolution.Projects.SelectMany(x => x.Documents).ToImmutableArray(); QueueForAnalysis(documents, AnalyzerWorkType.Background); - return documents.SelectAsArray(d => d.Id); + return documents; } public async Task> GetAllDiagnosticsAsync() @@ -352,13 +353,13 @@ public async Task> GetAllDiagnosticsAsync() return await GetDiagnosticsByDocument(allDocuments, waitForDocuments: false); } - public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) + public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) { var documents = projectIds .SelectMany(projectId => _workspace.CurrentSolution.GetProject(projectId).Documents) .ToImmutableArray(); QueueForAnalysis(documents, AnalyzerWorkType.Background); - return documents.SelectAsArray(d => d.Id); + return documents; } public void Dispose() diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs index ae1a442e12..f509c2510b 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs @@ -77,17 +77,17 @@ public Task> GetDiagnostics(ImmutableArray> GetDiagnostics(ImmutableArray documents, bool skipCache) + public Task> GetDiagnostics(ImmutableArray documents) { - return _implementation.GetDiagnostics(documents, skipCache); + return _implementation.GetDiagnostics(documents); } - public ImmutableArray QueueDocumentsForDiagnostics() + public ImmutableArray QueueDocumentsForDiagnostics() { return _implementation.QueueDocumentsForDiagnostics(); } - public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) + public ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectIds) { return _implementation.QueueDocumentsForDiagnostics(projectIds); } diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs index 50ebef18b3..afaefcf514 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticWorker.cs @@ -8,9 +8,9 @@ namespace OmniSharp.Roslyn.CSharp.Workers.Diagnostics public interface ICsDiagnosticWorker { Task> GetDiagnostics(ImmutableArray documentPaths); - Task> GetDiagnostics(ImmutableArray documents, bool skipCache); + Task> GetDiagnostics(ImmutableArray documents); Task> GetAllDiagnosticsAsync(); - ImmutableArray QueueDocumentsForDiagnostics(); - ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectId); + ImmutableArray QueueDocumentsForDiagnostics(); + ImmutableArray QueueDocumentsForDiagnostics(ImmutableArray projectId); } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs index 9d46410252..2da3b59acd 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs @@ -248,7 +248,7 @@ public void WhenBackgroundWorkIsAdded_DontWaitIt() } [Fact] - public void WhenSingleFileIsPromoted_ThenPromoteItFromBackgroundQueueToForeground() + public void WhenSingleFileIsQueued_ThenPromoteItFromBackgroundQueueToForeground() { var now = DateTime.UtcNow; var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); @@ -256,7 +256,7 @@ public void WhenSingleFileIsPromoted_ThenPromoteItFromBackgroundQueueToForegroun queue.PutWork(new[] { document }, AnalyzerWorkType.Background); - queue.TryPromote(document); + queue.QueueDocumentForeground(document); now = PassOverThrotlingPeriod(now); @@ -264,17 +264,17 @@ public void WhenSingleFileIsPromoted_ThenPromoteItFromBackgroundQueueToForegroun } [Fact] - public void WhenFileIsntAtBackgroundQueueAndTriedToBePromoted_ThenDontDoNothing() + public void WhenFileIsntAtBackgroundQueueAndTriedToBeQueued_ThenQueue() { var now = DateTime.UtcNow; var queue = new AnalyzerWorkQueue(new TestLoggerFactory(), utcNow: () => now, timeoutForPendingWorkMs: 10*1000); var document = CreateTestDocumentId(); - queue.TryPromote(document); + queue.QueueDocumentForeground(document); now = PassOverThrotlingPeriod(now); - Assert.Empty(queue.TakeWork(AnalyzerWorkType.Foreground)); + Assert.Equal(document, queue.TakeWork(AnalyzerWorkType.Foreground).Single()); } [Fact] @@ -290,7 +290,7 @@ public void WhenFileIsProcessingInBackgroundQueue_ThenPromoteItAsForeground() var activeWork = queue.TakeWork(AnalyzerWorkType.Background); - queue.TryPromote(document); + queue.QueueDocumentForeground(document); now = PassOverThrotlingPeriod(now); From 377bf8437d2c1a2f862e1037b05bffe006bb957e Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Sat, 3 Oct 2020 00:07:51 -0700 Subject: [PATCH 5/5] Fix stackoverflow. --- .../Workers/Diagnostics/CSharpDiagnosticWorker.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs index fb4b3633c6..dab71967f1 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs @@ -150,7 +150,7 @@ public async Task> GetDiagnostics(ImmutableA public Task> GetDiagnostics(ImmutableArray documents) { - return GetDiagnostics(documents); + return GetDiagnostics((IEnumerable)documents); } private async Task> GetDiagnostics(IEnumerable documents)