Rework code action service

src/OmniSharp.Abstractions/Configuration.cs

@@ -11,7 +11,7 @@ internal static class Configuration
         public readonly static string RoslynCSharpFeatures = GetRoslynAssemblyFullName("Microsoft.CodeAnalysis.CSharp.Features");
         public readonly static string RoslynWorkspaces = GetRoslynAssemblyFullName("Microsoft.CodeAnalysis.Workspaces");
 
-        public static string GetRoslynAssemblyFullName(string name)
+        private static string GetRoslynAssemblyFullName(string name)
         {
             return $"{name}, Version={RoslynVersion}, Culture=neutral, PublicKeyToken={RoslynPublicKeyToken}";
         }

src/OmniSharp.Abstractions/Utilities/DictionaryExtensions.cs

@@ -0,0 +1,19 @@
+using System;
+using System.Collections.Generic;
+
+namespace OmniSharp.Utilities
+{
+    internal static class DictionaryExtensions
+    {
+        public static TValue GetOrAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, Func<TKey, TValue> valueGetter)
+        {
+            if (!dictionary.TryGetValue(key, out var value))
+            {
+                value = valueGetter(key);
+                dictionary.Add(key, value);
+            }
+
+            return value;
+        }
+    }
+}

src/OmniSharp.Roslyn.CSharp/CodeActions/ExternalCodeActionProvider.cs

@@ -3,12 +3,13 @@
 
 namespace OmniSharp.Roslyn.CSharp.Services.CodeActions
 {
+    [Shared]
     [Export(typeof(ICodeActionProvider))]
     public class ExternalCodeActionProvider : AbstractCodeActionProvider
     {
         [ImportingConstructor]
-        public ExternalCodeActionProvider(ExternalFeaturesHostServicesProvider featuresHostServicesProvider)
-            : base("ExternalCodeActions", featuresHostServicesProvider.Assemblies)
+        public ExternalCodeActionProvider(ExternalFeaturesHostServicesProvider hostServicesProvider)
+            : base("ExternalCodeActions", hostServicesProvider.Assemblies)
         {
         }
     }

src/OmniSharp.Roslyn.CSharp/CodeActions/RoslynCodeActionProvider.cs

@@ -3,6 +3,7 @@
 
 namespace OmniSharp.Roslyn.CSharp.Services.CodeActions
 {
+    [Shared]
     [Export(typeof(ICodeActionProvider))]
     public class RoslynCodeActionProvider : AbstractCodeActionProvider
     {

src/OmniSharp.Roslyn.CSharp/Services/ExternalFeaturesHostServicesProvider.cs

@@ -7,19 +7,19 @@
 
 namespace OmniSharp.Roslyn.CSharp.Services
 {
+    [Shared]
     [Export(typeof(IHostServicesProvider))]
     [Export(typeof(ExternalFeaturesHostServicesProvider))]
-    [Shared]
     public class ExternalFeaturesHostServicesProvider : IHostServicesProvider
     {
         public ImmutableArray<Assembly> Assemblies { get; }
 
         [ImportingConstructor]
-        public ExternalFeaturesHostServicesProvider(IAssemblyLoader loader, OmniSharpOptions options, IOmniSharpEnvironment env)
+        public ExternalFeaturesHostServicesProvider(IAssemblyLoader loader, OmniSharpOptions options, IOmniSharpEnvironment environment)
         {
             var builder = ImmutableArray.CreateBuilder<Assembly>();
 
-            var roslynExtensionsLocations = options.RoslynExtensionsOptions.GetNormalizedLocationPaths(env);
+            var roslynExtensionsLocations = options.RoslynExtensionsOptions.GetNormalizedLocationPaths(environment);
             if (roslynExtensionsLocations?.Any() == true)
             {
                 foreach (var roslynExtensionsLocation in roslynExtensionsLocations)

src/OmniSharp.Roslyn.CSharp/Services/Refactoring/GetCodeActionService.cs

@@ -60,7 +60,7 @@ private async Task GetContextualCodeActions(CodeRefactoringContext? context)
             {
                 foreach (var provider in _codeActionProviders)
                 {
-                    var providers = provider.Refactorings;
+                    var providers = provider.CodeRefactoringProviders;
 
                     foreach (var codeActionProvider in providers)
                     {

src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunCodeActionService.cs

@@ -93,7 +93,7 @@ private async Task GetContextualCodeActions(CodeRefactoringContext? context)
             {
                 foreach (var provider in _codeActionProviders)
                 {
-                    var providers = provider.Refactorings;
+                    var providers = provider.CodeRefactoringProviders;
 
                     foreach (var codeActionProvider in providers)
                     {

src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs

@@ -26,9 +26,10 @@ public abstract class BaseCodeActionService<TRequest, TResponse> : IRequestHandl
         protected readonly ILogger Logger;
 
         private readonly CodeActionHelper _helper;
-
         private readonly MethodInfo _getNestedCodeActions;
 
+        private static readonly Func<TextSpan, List<Diagnostic>> s_createDiagnosticList = _ => new List<Diagnostic>();
+
         protected BaseCodeActionService(OmniSharpWorkspace workspace, CodeActionHelper helper, IEnumerable<ICodeActionProvider> providers, ILogger logger)
         {
             this.Workspace = workspace;
@@ -54,148 +55,148 @@ protected BaseCodeActionService(OmniSharpWorkspace workspace, CodeActionHelper h
 
         protected async Task<IEnumerable<AvailableCodeAction>> GetAvailableCodeActions(ICodeActionRequest request)
         {
-            var originalDocument = this.Workspace.GetDocument(request.FileName);
-            if (originalDocument == null)
+            var document = this.Workspace.GetDocument(request.FileName);
+            if (document == null)
             {
                 return Array.Empty<AvailableCodeAction>();
             }
 
-            var actions = new List<CodeAction>();
+            var codeActions = new List<CodeAction>();
 
-            var codeFixContext = await GetCodeFixContext(originalDocument, request, actions);
-            if (codeFixContext != null)
-            {
-                await CollectCodeFixActions(codeFixContext.Value);
-            }
+            var sourceText = await document.GetTextAsync();
+            var span = GetTextSpan(request, sourceText);
 
-            var refactoringContext = await GetRefactoringContext(originalDocument, request, actions);
-            if (refactoringContext != null)
-            {
-                await CollectRefactoringActions(refactoringContext.Value);
-            }
+            await CollectCodeFixesActions(document, span, codeActions);
+            await CollectRefactoringActions(document, span, codeActions);
 
             // TODO: Determine good way to order code actions.
-            actions.Reverse();
+            codeActions.Reverse();
 
             // Be sure to filter out any code actions that inherit from CodeActionWithOptions.
             // This isn't a great solution and might need changing later, but every Roslyn code action
             // derived from this type tries to display a dialog. For now, this is a reasonable solution.
-            var availableActions = ConvertToAvailableCodeAction(actions)
+            var availableActions = ConvertToAvailableCodeAction(codeActions)
                 .Where(a => !a.CodeAction.GetType().GetTypeInfo().IsSubclassOf(typeof(CodeActionWithOptions)));
 
             return availableActions;
         }
 
-        private async Task<CodeRefactoringContext?> GetRefactoringContext(Document originalDocument, ICodeActionRequest request, List<CodeAction> actionsDestination)
+        private TextSpan GetTextSpan(ICodeActionRequest request, SourceText sourceText)
         {
-            var sourceText = await originalDocument.GetTextAsync();
-            var location = GetTextSpan(request, sourceText);
-            return new CodeRefactoringContext(originalDocument, location, (a) => actionsDestination.Add(a), CancellationToken.None);
+            if (request.Selection != null)
+            {
+                return TextSpan.FromBounds(
+                    sourceText.Lines.GetPosition(new LinePosition(request.Selection.Start.Line, request.Selection.Start.Column)),
+                    sourceText.Lines.GetPosition(new LinePosition(request.Selection.End.Line, request.Selection.End.Column)));
+            }
+
+            var position = sourceText.Lines.GetPosition(new LinePosition(request.Line, request.Column));
+            return new TextSpan(position, length: 0);
         }
 
-        private async Task<CodeFixContext?> GetCodeFixContext(Document originalDocument, ICodeActionRequest request, List<CodeAction> actionsDestination)
+        private async Task CollectCodeFixesActions(Document document, TextSpan span, List<CodeAction> codeActions)
         {
-            var sourceText = await originalDocument.GetTextAsync();
-            var semanticModel = await originalDocument.GetSemanticModelAsync();
-            var diagnostics = semanticModel.GetDiagnostics();
-            var span = GetTextSpan(request, sourceText);
+            Dictionary<TextSpan, List<Diagnostic>> aggregatedDiagnostics = null;
 
-            // Try to find exact match
-            var pointDiagnostics = diagnostics.Where(d => d.Location.SourceSpan.Equals(span)).ToImmutableArray();
-            // No exact match found, try approximate match instead
-            if (pointDiagnostics.Length == 0)
+            var semanticModel = await document.GetSemanticModelAsync();
+
+            foreach (var diagnostic in semanticModel.GetDiagnostics())
             {
-                var firstMatchingDiagnostic = diagnostics.FirstOrDefault(d => d.Location.SourceSpan.Contains(span));
-                // Try to find other matches with the same exact span as the first approximate match
-                if (firstMatchingDiagnostic != null)
+                if (!span.IntersectsWith(diagnostic.Location.SourceSpan))
                 {
-                    pointDiagnostics = diagnostics.Where(d => d.Location.SourceSpan.Equals(firstMatchingDiagnostic.Location.SourceSpan)).ToImmutableArray();
+                    continue;
                 }
+
+                aggregatedDiagnostics = aggregatedDiagnostics ?? new Dictionary<TextSpan, List<Diagnostic>>();
+                var list = aggregatedDiagnostics.GetOrAdd(diagnostic.Location.SourceSpan, s_createDiagnosticList);
+                list.Add(diagnostic);
             }
-            // At this point all pointDiagnostics guaranteed to have the same span
-            if (pointDiagnostics.Length > 0)
+
+            if (aggregatedDiagnostics == null)
             {
-                return new CodeFixContext(originalDocument, pointDiagnostics[0].Location.SourceSpan, pointDiagnostics, (a, d) => actionsDestination.Add(a), CancellationToken.None);
+                return;
             }
 
-            return null;
-        }
-
-        private TextSpan GetTextSpan(ICodeActionRequest request, SourceText sourceText)
-        {
-            if (request.Selection != null)
+            foreach (var kvp in aggregatedDiagnostics)
             {
-                var startPosition = sourceText.Lines.GetPosition(new LinePosition(request.Selection.Start.Line, request.Selection.Start.Column));
-                var endPosition = sourceText.Lines.GetPosition(new LinePosition(request.Selection.End.Line, request.Selection.End.Column));
-                return TextSpan.FromBounds(startPosition, endPosition);
-            }
+                var diagnosticSpan = kvp.Key;
+                var diagnosticsWithSameSpan = kvp.Value.OrderByDescending(d => d.Severity);
 
-            var position = sourceText.Lines.GetPosition(new LinePosition(request.Line, request.Column));
-            return new TextSpan(position, 1);
+                await AppendFixesAsync(document, diagnosticSpan, diagnosticsWithSameSpan, codeActions);
+            }
         }
 
-        private async Task CollectCodeFixActions(CodeFixContext context)
+        private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerable<Diagnostic> diagnostics, List<CodeAction> codeActions)
         {
-            var diagnosticIds = context.Diagnostics.Select(d => d.Id).ToArray();
-
             foreach (var provider in this.Providers)
             {
-                foreach (var codeFix in provider.CodeFixes)
+                foreach (var codeFixProvider in provider.CodeFixProviders)
                 {
-                    if (_helper.IsDisallowed(codeFix))
+                    var fixableDiagnostics = diagnostics.Where(d => HasFix(codeFixProvider, d.Id)).ToImmutableArray();
+                    if (fixableDiagnostics.Length > 0)
                     {
-                        continue;
-                    }
-
-                    var typeName = codeFix.GetType().FullName;
+                        var context = new CodeFixContext(document, span, fixableDiagnostics, (a, _) => codeActions.Add(a), CancellationToken.None);
 
-                    // TODO: This is a horrible hack! However, remove unnecessary usings only
-                    // responds for diagnostics that are produced by its diagnostic analyzer.
-                    // We need to provide a *real* diagnostic engine to address this.
-                    if (typeName != CodeActionHelper.RemoveUnnecessaryUsingsProviderName)
-                    {
-                        if (!diagnosticIds.Any(id => codeFix.FixableDiagnosticIds.Contains(id)))
+                        try
                         {
-                            continue;
+                            await codeFixProvider.RegisterCodeFixesAsync(context);
+                        }
+                        catch (Exception ex)
+                        {
+                            this.Logger.LogError(ex, $"Error registering code fixes for {codeFixProvider.GetType().FullName}");
                         }
                     }
+                }
+            }
+        }
 
-                    if (typeName == CodeActionHelper.RemoveUnnecessaryUsingsProviderName &&
-                        !diagnosticIds.Contains("CS8019")) // ErrorCode.HDN_UnusedUsingDirective
-                    {
-                        continue;
-                    }
+        private bool HasFix(CodeFixProvider codeFixProvider, string diagnosticId)
+        {
+            var typeName = codeFixProvider.GetType().FullName;
 
-                    try
-                    {
-                        await codeFix.RegisterCodeFixesAsync(context);
-                    }
-                    catch (Exception ex)
-                    {
-                        this.Logger.LogError(ex, $"Error registering code fixes for {typeName}");
-                    }
+            if (_helper.IsDisallowed(typeName))
+            {
+                return false;
+            }
+
+            // TODO: This is a horrible hack! However, remove unnecessary usings only
+            // responds for diagnostics that are produced by its diagnostic analyzer.
+            // We need to provide a *real* diagnostic engine to address this.
+            if (typeName != CodeActionHelper.RemoveUnnecessaryUsingsProviderName)
+            {
+                if (!codeFixProvider.FixableDiagnosticIds.Contains(diagnosticId))
+                {
+                    return false;
                 }
             }
+            else if (diagnosticId != "CS8019") // ErrorCode.HDN_UnusedUsingDirective
+            {
+                return false;
+            }
+
+            return true;
         }
 
-        private async Task CollectRefactoringActions(CodeRefactoringContext context)
+        private async Task CollectRefactoringActions(Document document, TextSpan span, List<CodeAction> codeActions)
         {
             foreach (var provider in this.Providers)
             {
-                foreach (var refactoring in provider.Refactorings)
+                foreach (var codeRefactoringProvider in provider.CodeRefactoringProviders)
                 {
-                    if (_helper.IsDisallowed(refactoring))
+                    if (_helper.IsDisallowed(codeRefactoringProvider))
                     {
                         continue;
                     }
 
+                    var context = new CodeRefactoringContext(document, span, a => codeActions.Add(a), CancellationToken.None);
+
                     try
                     {
-                        await refactoring.ComputeRefactoringsAsync(context);
+                        await codeRefactoringProvider.ComputeRefactoringsAsync(context);
                     }
                     catch (Exception ex)
                     {
-                        this.Logger.LogError(ex, $"Error computing refactorings for {refactoring.GetType().FullName}");
+                        this.Logger.LogError(ex, $"Error computing refactorings for {codeRefactoringProvider.GetType().FullName}");
                     }
                 }
             }

src/OmniSharp.Roslyn.CSharp/Services/RoslynFeaturesHostServicesProvider.cs

@@ -1,14 +1,13 @@
 using System.Collections.Immutable;
 using System.Composition;
 using System.Reflection;
-using OmniSharp.Options;
 using OmniSharp.Services;
 
 namespace OmniSharp.Roslyn.CSharp.Services
 {
+    [Shared]
     [Export(typeof(IHostServicesProvider))]
     [Export(typeof(RoslynFeaturesHostServicesProvider))]
-    [Shared]
     public class RoslynFeaturesHostServicesProvider : IHostServicesProvider
     {
         public ImmutableArray<Assembly> Assemblies { get; }
@@ -17,13 +16,7 @@ public class RoslynFeaturesHostServicesProvider : IHostServicesProvider
         public RoslynFeaturesHostServicesProvider(IAssemblyLoader loader)
         {
             var builder = ImmutableArray.CreateBuilder<Assembly>();
-
-            var Features = Configuration.GetRoslynAssemblyFullName("Microsoft.CodeAnalysis.Features");
-            var CSharpFeatures = Configuration.GetRoslynAssemblyFullName("Microsoft.CodeAnalysis.CSharp.Features");
-
-            builder.AddRange(loader.Load(Features, CSharpFeatures));
-
-
+            builder.AddRange(loader.Load(Configuration.RoslynFeatures, Configuration.RoslynCSharpFeatures));
             Assemblies = builder.ToImmutable();
         }
     }

src/OmniSharp.Roslyn.CSharp/Workers/Refactoring/FixUsingsWorker.cs

@@ -32,7 +32,7 @@ public FixUsingsWorker(IEnumerable<ICodeActionProvider> providers)
         {
             _providers = providers;
 
-            var codeFixProviders = providers.SelectMany(p => p.CodeFixes);
+            var codeFixProviders = providers.SelectMany(p => p.CodeFixProviders);
 
             _addImportProvider = FindCodeFixProviderByTypeFullName(codeFixProviders, CodeActionHelper.AddImportProviderName);
             _removeUnnecessaryUsingsProvider = FindCodeFixProviderByTypeFullName(codeFixProviders, CodeActionHelper.RemoveUnnecessaryUsingsProviderName);