Order code actions

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

@@ -23,6 +23,8 @@ public abstract class BaseCodeActionService<TRequest, TResponse> : IRequestHandl
     {
         protected readonly OmniSharpWorkspace Workspace;
         protected readonly IEnumerable<ICodeActionProvider> Providers;
+        protected List<CodeFixProvider> OrderedCodeFixProviders;
+        protected List<CodeRefactoringProvider> OrderedCodeRefactoringProviders;
         protected readonly ILogger Logger;
 
         private readonly CodeActionHelper _helper;
@@ -37,6 +39,8 @@ protected BaseCodeActionService(OmniSharpWorkspace workspace, CodeActionHelper h
             this.Logger = logger;
             this._helper = helper;
 
+            SetOrderedProviders();
+
             // Sadly, the CodeAction.NestedCodeActions property is still internal.
             var nestedCodeActionsProperty = typeof(CodeAction).GetProperty("NestedCodeActions", BindingFlags.NonPublic | BindingFlags.Instance);
             if (nestedCodeActionsProperty == null)
@@ -51,6 +55,41 @@ protected BaseCodeActionService(OmniSharpWorkspace workspace, CodeActionHelper h
             }
         }
 
+        private void SetOrderedProviders()
+        {
+            try
+            {
+                OrderedCodeFixProviders = GetSortedCodeFixProviders();
+            }
+            catch
+            {
+                OrderedCodeFixProviders = new List<CodeFixProvider>();
+                foreach (var provider in this.Providers)
+                {
+                    foreach (var codeFixProvider in provider.CodeFixProviders)
+                    {
+                        OrderedCodeFixProviders.Add(codeFixProvider);
+                    }
+                }
+            }
+
+            try
+            {
+                OrderedCodeRefactoringProviders = GetSortedCodeRefactoringProviders();
+            }
+            catch
+            {
+                OrderedCodeRefactoringProviders = new List<CodeRefactoringProvider>();
+                foreach (var provider in this.Providers)
+                {
+                    foreach (var codeRefactoringProvider in provider.CodeRefactoringProviders)
+                    {
+                        OrderedCodeRefactoringProviders.Add(codeRefactoringProvider);
+                    }
+                }
+            }
+        }
+
         public abstract Task<TResponse> Handle(TRequest request);
 
         protected async Task<IEnumerable<AvailableCodeAction>> GetAvailableCodeActions(ICodeActionRequest request)
@@ -69,9 +108,6 @@ protected async Task<IEnumerable<AvailableCodeAction>> GetAvailableCodeActions(I
             await CollectCodeFixesActions(document, span, codeActions);
             await CollectRefactoringActions(document, span, codeActions);
 
-            // TODO: Determine good way to order code actions.
-            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.
@@ -128,28 +164,57 @@ private async Task CollectCodeFixesActions(Document document, TextSpan span, Lis
 
         private async Task AppendFixesAsync(Document document, TextSpan span, IEnumerable<Diagnostic> diagnostics, List<CodeAction> codeActions)
         {
-            foreach (var provider in this.Providers)
+            foreach (var codeFixProvider in OrderedCodeFixProviders)
             {
-                foreach (var codeFixProvider in provider.CodeFixProviders)
+                var fixableDiagnostics = diagnostics.Where(d => HasFix(codeFixProvider, d.Id)).ToImmutableArray();
+                if (fixableDiagnostics.Length > 0)
                 {
-                    var fixableDiagnostics = diagnostics.Where(d => HasFix(codeFixProvider, d.Id)).ToImmutableArray();
-                    if (fixableDiagnostics.Length > 0)
+                    var context = new CodeFixContext(document, span, fixableDiagnostics, (a, _) => codeActions.Add(a), CancellationToken.None);
+
+                    try
+                    {
+                        await codeFixProvider.RegisterCodeFixesAsync(context);
+                    }
+                    catch (Exception ex)
                     {
-                        var context = new CodeFixContext(document, span, fixableDiagnostics, (a, _) => codeActions.Add(a), CancellationToken.None);
-
-                        try
-                        {
-                            await codeFixProvider.RegisterCodeFixesAsync(context);
-                        }
-                        catch (Exception ex)
-                        {
-                            this.Logger.LogError(ex, $"Error registering code fixes for {codeFixProvider.GetType().FullName}");
-                        }
+                        this.Logger.LogError(ex, $"Error registering code fixes for {codeFixProvider.GetType().FullName}");
                     }
                 }
             }
         }
 
+        private List<CodeFixProvider> GetSortedCodeFixProviders()
+        {
+            List<ProviderNode<CodeFixProvider>> nodesList = new List<ProviderNode<CodeFixProvider>>();
+            foreach (var provider in this.Providers)
+            {
+                foreach (var codeFixProvider in provider.CodeFixProviders)
+                {
+                    nodesList.Add(ProviderNode<CodeFixProvider>.From(codeFixProvider));
+                }
+            }
+
+            var graph = Graph<CodeFixProvider>.GetGraph(nodesList);
+            graph.CheckForCycles();
+            return graph.TopologicalSort();
+        }
+
+        private List<CodeRefactoringProvider> GetSortedCodeRefactoringProviders()
+        {
+            List<ProviderNode<CodeRefactoringProvider>> nodesList = new List<ProviderNode<CodeRefactoringProvider>>();
+            foreach (var provider in this.Providers)
+            {
+                foreach (var codeRefactoringProvider in provider.CodeRefactoringProviders)
+                {
+                    nodesList.Add(ProviderNode<CodeRefactoringProvider>.From(codeRefactoringProvider));
+                }
+            }
+
+            var graph = Graph<CodeRefactoringProvider>.GetGraph(nodesList);
+            graph.CheckForCycles();
+            return graph.TopologicalSort();
+        }
+
         private bool HasFix(CodeFixProvider codeFixProvider, string diagnosticId)
         {
             var typeName = codeFixProvider.GetType().FullName;
@@ -179,25 +244,22 @@ private bool HasFix(CodeFixProvider codeFixProvider, string diagnosticId)
 
         private async Task CollectRefactoringActions(Document document, TextSpan span, List<CodeAction> codeActions)
         {
-            foreach (var provider in this.Providers)
+            foreach (var codeRefactoringProvider in OrderedCodeRefactoringProviders)
             {
-                foreach (var codeRefactoringProvider in provider.CodeRefactoringProviders)
+                if (_helper.IsDisallowed(codeRefactoringProvider))
                 {
-                    if (_helper.IsDisallowed(codeRefactoringProvider))
-                    {
-                        continue;
-                    }
+                    continue;
+                }
 
-                    var context = new CodeRefactoringContext(document, span, a => codeActions.Add(a), CancellationToken.None);
+                var context = new CodeRefactoringContext(document, span, a => codeActions.Add(a), CancellationToken.None);
 
-                    try
-                    {
-                        await codeRefactoringProvider.ComputeRefactoringsAsync(context);
-                    }
-                    catch (Exception ex)
-                    {
-                        this.Logger.LogError(ex, $"Error computing refactorings for {codeRefactoringProvider.GetType().FullName}");
-                    }
+                try
+                {
+                    await codeRefactoringProvider.ComputeRefactoringsAsync(context);
+                }
+                catch (Exception ex)
+                {
+                    this.Logger.LogError(ex, $"Error computing refactorings for {codeRefactoringProvider.GetType().FullName}");
                 }
             }
         }

src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/CodeActionsOrder.Graph.cs

@@ -0,0 +1,83 @@
+// Adapted from ExtensionOrderer in Roslyn
+using System.Collections.Generic;
+
+namespace OmniSharp.Roslyn.CSharp.Services.Refactoring.V2
+{
+    internal class Graph<T>
+    {
+        //Dictionary to map between nodes and the names
+        private Dictionary<string, ProviderNode<T>> Nodes { get; }
+        private List<ProviderNode<T>> AllNodes { get; }
+        private Graph(List<ProviderNode<T>> nodesList)
+        {
+            Nodes = new Dictionary<string, ProviderNode<T>>();
+            AllNodes = nodesList;
+        }
+        internal static Graph<T> GetGraph(List<ProviderNode<T>> nodesList)
+        {
+            var graph = new Graph<T>(nodesList);
+
+            foreach (ProviderNode<T> node in graph.AllNodes)
+            {
+                graph.Nodes[node.ProviderName] = node;
+            }
+
+            foreach (ProviderNode<T> node in graph.AllNodes)
+            {
+                foreach (var before in node.Before)
+                {
+                    if (graph.Nodes.ContainsKey(before))
+                    {
+                        var beforeNode = graph.Nodes[before];
+                        beforeNode.NodesBeforeMeSet.Add(node);
+                    }
+                }
+
+                foreach (var after in node.After)
+                {
+                    if (graph.Nodes.ContainsKey(after))
+                    {
+                        var afterNode = graph.Nodes[after];
+                        node.NodesBeforeMeSet.Add(afterNode);
+                    }
+                }
+            }
+
+            return graph;
+        }
+
+        public void CheckForCycles()
+        {
+            foreach (var node in this.AllNodes)
+            {
+                node.CheckForCycles();
+            }
+        }
+
+        public List<T> TopologicalSort()
+        {
+            List<T> result = new List<T>();
+            var seenNodes = new HashSet<ProviderNode<T>>();
+
+            foreach (var node in AllNodes)
+            {
+                Visit(node, result, seenNodes);
+            }
+
+            return result;
+        }
+
+        private void Visit(ProviderNode<T> node, List<T> result, HashSet<ProviderNode<T>> seenNodes)
+        {
+            if (seenNodes.Add(node))
+            {
+                foreach (var before in node.NodesBeforeMeSet)
+                {
+                    Visit(before, result, seenNodes);
+                }
+
+                result.Add(node.Provider);
+            }
+        }
+    }
+}

src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/CodeActionsOrder.ProviderNode.cs

@@ -0,0 +1,71 @@
+// Adapted from ExtensionOrderer in Roslyn
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CodeFixes;
+
+namespace OmniSharp.Roslyn.CSharp.Services.Refactoring.V2
+{
+    internal class ProviderNode<T>
+    {
+        public string ProviderName { get; set; }
+        public List<string> Before { get; set; }
+        public List<string> After { get; set; }
+        public T Provider { get; set; }
+        public HashSet<ProviderNode<T>> NodesBeforeMeSet { get; set; }
+
+        public static ProviderNode<T> From(T provider)
+        {
+            var exportAttribute = provider.GetType().GetCustomAttribute(typeof(ExportCodeFixProviderAttribute));
+            string providerName = "";
+            if (exportAttribute is ExportCodeFixProviderAttribute && ((ExportCodeFixProviderAttribute)exportAttribute).Name != null)
+            {
+                providerName = ((ExportCodeFixProviderAttribute)exportAttribute).Name;
+            }
+
+            var orderAttributes = provider.GetType().GetCustomAttributes(typeof(ExtensionOrderAttribute), true).Select(attr => (ExtensionOrderAttribute)attr).ToList();
+            return new ProviderNode<T>(provider, providerName, orderAttributes);
+        }
+
+        private ProviderNode(T provider, string providerName, List<ExtensionOrderAttribute> orderAttributes)
+        {
+            Provider = provider;
+            ProviderName = providerName;
+            Before = new List<string>();
+            After = new List<string>();
+            NodesBeforeMeSet = new HashSet<ProviderNode<T>>();
+            orderAttributes.ForEach(attr => AddAttribute(attr));
+        }
+
+        private void AddAttribute(ExtensionOrderAttribute attribute)
+        {
+            if (attribute.Before != null)
+                Before.Add(attribute.Before);
+            if (attribute.After != null)
+                After.Add(attribute.After);
+        }
+
+        internal void CheckForCycles()
+        {
+            CheckForCycles(new HashSet<ProviderNode<T>>());
+        }
+
+        private void CheckForCycles(HashSet<ProviderNode<T>> seenNodes)
+        {
+            if (!seenNodes.Add(this))
+            {
+                //Cycle detected
+                throw new ArgumentException("Cycle detected");
+            }
+
+            foreach (var before in this.NodesBeforeMeSet)
+            {
+                before.CheckForCycles(seenNodes);
+            }
+
+            seenNodes.Remove(this);
+        }
+    }
+}

tests/OmniSharp.Roslyn.CSharp.Tests/CodeActionsV2Facts.cs

@@ -99,6 +99,35 @@ public void Whatever()
             Assert.Contains("Extract Method", refactorings);
         }
 
+        [Fact]
+        public async Task Returns_ordered_code_actions()
+        {
+            const string code =
+                @"public class Class1
+                {
+                    public void Whatever()
+                    {
+                        [|Console.Write(""should be using System;"");|]
+                    }
+                }";
+
+            var refactorings = await FindRefactoringNamesAsync(code);
+            List<string> expected = new List<string>
+            {
+                "using System;",
+                "System.Console",
+                "Generate variable 'Console' -> Generate property 'Class1.Console'",
+                "Generate variable 'Console' -> Generate field 'Class1.Console'",
+                "Generate variable 'Console' -> Generate read-only field 'Class1.Console'",
+                "Generate variable 'Console' -> Generate local 'Console'",
+                "Generate type 'Console' -> Generate class 'Console' in new file",
+                "Generate type 'Console' -> Generate class 'Console'",
+                "Generate type 'Console' -> Generate nested class 'Console'",
+                "Extract Method"
+            };
+            Assert.Equal(expected, refactorings);
+        }
+
         [Fact]
         public async Task Can_extract_method()
         {
@@ -190,7 +219,7 @@ private async Task<IEnumerable<OmniSharpCodeAction>> FindRefactoringsAsync(strin
         {
             var testFile = new TestFile(BufferPath, code);
 
-            using (var host = CreateOmniSharpHost(new [] { testFile }, configurationData))
+            using (var host = CreateOmniSharpHost(new[] { testFile }, configurationData))
             {
                 var requestHandler = host.GetRequestHandler<GetCodeActionsService>(OmniSharpEndpoints.V2.GetCodeActions);