Added an option to configure a code actions folder #848

filipw · 2017-05-05T12:15:52Z

Use case: I'd like to load my own or 3rd party code actions into Omnisharp.

In this PR I added a possibility to add the following CodeActions node to the omnisharp.json.

{
    "FormattingOptions": {
        "IndentationSize": 2
    },
    "CodeActions": {
        "LocationPaths": [
            "C:/codeactions"
        ]
    }
}

This will be picked up by RoslynFeaturesHostServicesProvider as a source location for loading assemblies containing refactorings and code fixes.
Because omnisharp.json is hierarchical (global/local) we can specify code actions on a global and local level if needed.

No I could download something like https://marketplace.visualstudio.com/items?itemName=josefpihrt.Roslynator2017 and extract its DLLs into C:/codeactions and profit. Relative and absolute paths are supported.

filipw · 2017-05-05T12:43:24Z

The reason why I thought it might be a good idea is it could give a chance for user to opt-in into features not yet available as internal part of Roslyn, but already built in the community or self-built. (i.e. dotnet/roslyn#8925 which has been requested already in Omnisharp repos and so on).

See: http://recordit.co/dEAKIBAvbl

filipw · 2017-05-05T12:45:28Z

src/OmniSharp/app.config

@@ -3,4 +3,20 @@
  <startup>
    <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.6" />
  </startup>
+  <runtime>


this was added so that a refactoring written against Roslyn 1.0.0 (which is often the case) can load too

This will need to be updated everytime we update Roslyn to a new version (or, at least, everytime the assembly version changes). Correct?

In the current format, yes, correct. While this is technically not necessary, most of the code actions out there (i.e. packaged as VSIX-es) reference Roslyn assemblies from the host (VS). So it has to be low version if you want to avoid having a dependency on something like let's say Update 3 of Visual Studio to be installed by the user.
I believe even the official Roslyn SDK templates default to Roslyn 1.0.0.

Otherwise, we'd only be able to load into the current process code actions built specifically against the version of Roslyn OmniSharp is built against.

I only added the three most common ones, maybe there are other DLLs that would be worthy of redirecting?

There are others that will come up eventually, but this is good for now.

david-driscoll

I think this is something we can support for the moment, with the knowledge that it is really just a power user feature, use at their own risk.

david-driscoll · 2017-05-05T15:08:57Z

src/OmniSharp.Abstractions/Options/CodeActionOptions.cs

+            var normalizePaths = new HashSet<string>();
+            foreach (var locationPath in LocationPaths)
+            {
+                if (Path.IsPathRooted(locationPath))


If the paths are relative shouldn't they be rooted in the directory where the configuration file lives? However this may be super painful to pull off (would require a custom options provider)

yeah so at the moment, relative always means relative to the folder in which OmniSharp is running.
I think this is reasonable for most cases, but you are right, when you are editing the global file like c:\users\filip\.omnisharp\omnisharp.json it might appear a bit counter intuitive.

Yeah, you're right that will be counter intuitive for the global config file case. Maybe a doc comment on LocationPaths to help describe the expected values?

DustinCampbell · 2017-05-05T15:54:46Z

Is this change all that interesting if we add a proper diagnostics engine that loads analyzers referenced by a project?

filipw · 2017-05-05T15:58:19Z

Is this change all that interesting if we add a proper diagnostics engine that loads analyzers referenced by a project?

however stand alone refactorings without analyzer (i.e. initalize a field from constructor, LINQ into foreach and so on) are installed as VSIX - they are not referenced by projects. So this PR attempts to cover only that use case.

Setting a code actions folder in a global omnisharp.json would be to me roughly equivalent to installing a VSIX into my Visual Studio.

DustinCampbell · 2017-05-05T16:00:46Z

The refactorings you mentioned will very likely be added to Roslyn itself.

filipw · 2017-05-05T16:11:08Z

yeah absolutely, then when they ship in the box, there is no need to opt into anything anymore 😃
But it's exactly the same case as with installing these types of add ons into Visual Studio - they can help fill some feature gap.

but until then, if you wish, you could tap into anything from here https://github.com/JosefPihrt/Roslynator/tree/master/source/Refactorings/Refactorings or here https://github.com/code-cracker/code-cracker/tree/master/src/CSharp/CodeCracker/Refactoring or few other places that people have been building.

This also has the reverse effect - if I'm an author of a refactoring, I'm loving the fact that I can build it, and it works in VS and I there is a way that I can also load it through OmniSharp into VS Code and other places. In fact, for example, at work, I have some domain specific refactorings for internal purposes only, that developers in our organization install into their VS - I think it would be great to be able to load them into OmniSharp.

I think ultimately the discussion should be what are the potential extensibility point in OmniSharp, and I think this is an excellent candidate one, especially as, in a way, it aligns with Visual Studio so experience-wise should be intuitive.

DustinCampbell · 2017-05-05T16:50:50Z

Note: I'm not against this change. I'm just pushing on whether it's the right change, so we don't have to support it later if we decide that we should have done something else.

david-driscoll · 2017-05-05T17:13:14Z

Would anyone complain if we called this a beta / alpha feature? Perhaps annotate the code (and docs when we get there) to indicate that.

Ideally I'd love some sort of nuget based process going forward, but this is a nice segway into that.

filipw · 2017-05-05T17:56:34Z

An alternative solution to the same problem could be something like this:

someone creates a hypothetical ThirdPartyCodeActionProvider implementation of OmniSharp's ICodeActionProvider which subclasses our RoslynCodeActionProvider and introduces the type of functionality contained in this current PR (ability to load code actions from some external paths)
package is published into Nuget
user installs this Nuget package into, say,`%USERPROFILE%.omnisharp\plugins
at startup we scan that plugins folder and pick up the plugin and swap the default RoslynCodeActionProvider with ThirdPartyCodeActionProvider and everything else remains unchanged.

The difference would be omnisharp.json cannot participate anymore, so the plugin would need to have its own config file, but I think that's fine.

This could also have a different format, and the plugin could just bundle the code actions straight up and offer them as direct dependency - the downside here is that you'd need such dedicated loader for everything, rather than a having a more generic solution.

DustinCampbell

Sorry for the delay in giving this a careful review.

DustinCampbell · 2017-05-07T14:13:54Z

src/OmniSharp.Abstractions/Options/OmniSharpOptions.cs

@@ -4,6 +4,8 @@ namespace OmniSharp.Options
 {
    public class OmniSharpOptions
    {
+        public CodeActionOptions CodeActions { get; set; } = new CodeActionOptions();


Why is this settable and instantiated here? Shouldn't it follow the same pattern as FormattingOptions below?

that's a good observation. Let's change it to public CodeActionOptions CodeActions { get; } = new CodeActionOptions(); I think this should still bind fine to the ASP.NET Configuration and be more elegant. The other property (FormattingOptions) should use the same approach, it would be cleaner. Currently it can be set through the constructor too, but that constructor is redundant - I don't believe we use it anywhere, only the default constructor is in use

DustinCampbell · 2017-05-07T14:16:23Z

src/OmniSharp.Abstractions/Services/IAssemblyLoader.cs

@@ -7,6 +7,8 @@ namespace OmniSharp.Services
    public interface IAssemblyLoader
    {
        Assembly Load(AssemblyName name);
+
+        IEnumerable<Assembly> LoadAll(string folderPath);


I'd prefer this be called LoadAllFrom to call out that it uses the LoadFrom context rather than the Load context.

DustinCampbell · 2017-05-07T14:18:46Z

src/OmniSharp.Host/Services/AssemblyLoader.cs

@@ -30,5 +33,43 @@ public Assembly Load(AssemblyName name)
            _logger.LogTrace($"Assembly loaded: {name}");
            return result;
        }
+
+        public IEnumerable<Assembly> LoadAll(string folderPath)


Seems like we could return a better interface than IEnumerable<Assembly> since the instance is a List<Assembly>. Maybe IReadOnlyList<Assembly> so that callers could index into it if they want.

DustinCampbell · 2017-05-07T14:19:33Z

src/OmniSharp.Host/Services/AssemblyLoader.cs

+
+        public IEnumerable<Assembly> LoadAll(string folderPath)
+        {
+            if (folderPath == null) return Enumerable.Empty<Assembly>();


string.IsNullOrWhitespace?

DustinCampbell · 2017-05-07T14:23:42Z

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

@@ -12,7 +14,7 @@ public class RoslynFeaturesHostServicesProvider : IHostServicesProvider
        public ImmutableArray<Assembly> Assemblies { get; }

        [ImportingConstructor]
-        public RoslynFeaturesHostServicesProvider(IAssemblyLoader loader)
+        public RoslynFeaturesHostServicesProvider(IAssemblyLoader loader, OmniSharpOptions options, IOmniSharpEnvironment env)


To separate concerns, this should probably be added to another IHostServicesProvider rather than tacked onto the RoslynFeaturesHostServicesProvider, since this is specifically about loading the Roslyn features assemblies. Note that OmniSharpWorkspace aggregates multiple IHostServicesProviders.

Also, I just realized that RoslynFeaturesHostServicesProvider is not marked as [Shared]. It probably doesn't matter in practice (since OmniSharpWorkspace is a singleton), but I just noticed.

I didn't know there can be many of them - makes sense, thanks!

DustinCampbell · 2017-05-07T14:30:16Z

src/OmniSharp.Abstractions/Options/CodeActionOptions.cs

+    {
+        public string[] LocationPaths { get; set; }
+
+        public IEnumerable<string> GetLocations(IOmniSharpEnvironment env)


Perhaps this should be called something like GetNormalizedLocationPaths(...) or something like that to indicate what it does differently over just accessing the LocationPaths property?

DustinCampbell · 2017-05-07T14:31:09Z

src/OmniSharp.Abstractions/Options/CodeActionOptions.cs

+        {
+            if (LocationPaths == null) return Enumerable.Empty<string>();
+
+            var normalizePaths = new HashSet<string>();


Do we want this to be case-sensitive? Or, should you add StringComparer.OrdinalIgnoreCase? (We do that in other places)

DustinCampbell · 2017-05-07T14:32:08Z

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

@@ -21,6 +23,15 @@ public RoslynFeaturesHostServicesProvider(IAssemblyLoader loader)

            builder.AddRange(loader.Load(Features, CSharpFeatures));

+            var codeActionLocations = options.CodeActions.GetLocations(env);
+            if (codeActionLocations != null && codeActionLocations.Any())


Optional: codeActionLocations?.Any() == true

DustinCampbell · 2017-05-07T14:33:52Z

src/OmniSharp.Abstractions/Options/CodeActionOptions.cs

+                normalizePaths.Add(Path.Combine(env.TargetDirectory, locationPath));
+            }
+
+            return normalizePaths;


Note that the paths will now be returned in a different order than they were specified, which could result in subtle bugs. (e.g. Imagine two assemblies that reference the same dependency but different versions. Their load order could be important.)

DustinCampbell · 2017-05-07T14:36:54Z

src/OmniSharp.Abstractions/Options/CodeActionOptions.cs

+            var normalizePaths = new HashSet<string>();
+            foreach (var locationPath in LocationPaths)
+            {
+                if (Path.IsPathRooted(locationPath))


Yeah, you're right that will be counter intuitive for the global config file case. Maybe a doc comment on LocationPaths to help describe the expected values?

DustinCampbell · 2017-05-15T14:38:27Z

I'm wondering if the folder should have a more general name. Maybe "RoslynExtensions" instead of "CodeActions". After all, there's nothing restricting this to just Code Actions. It would also load, say, CompletionProviders.

filipw · 2017-05-15T15:12:34Z

I'm wondering if the folder should have a more general name. Maybe "RoslynExtensions" instead of "CodeActions". After all, there's nothing restricting this to just Code Actions. It would also load, say, CompletionProviders.

yes makes sense to me. We still need relevant exports to be there (i.e. ICodeActionProvider) to understand which types should be discovered, but as far as loading assemblies, indeed, it can bring in all kinds of extensions/add-ons.

DustinCampbell

just nits

DustinCampbell · 2017-05-17T09:25:42Z

src/OmniSharp.Host/Services/AssemblyLoader.cs

+
+        public IReadOnlyList<Assembly> LoadAllFrom(string folderPath)
+        {
+            if (string.IsNullOrWhiteSpace(folderPath)) return new Assembly[0];


You could use Array.Empty<Assembly>() and avoid allocating here if you like.

DustinCampbell · 2017-05-17T09:26:07Z

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

 using OmniSharp.Services;
+using System.Linq;


Sort usings? It looks like System.Linq wound up on the bottom.

DustinCampbell · 2017-05-17T09:26:15Z

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

+using System.Reflection;
+using OmniSharp.Options;
+using OmniSharp.Services;
+using System.Linq;


Sort usings? It looks like System.Linq wound up on the bottom.

DustinCampbell · 2017-05-17T11:04:21Z

@filipw: CI passed. I'm good with this change (only found a couple of tiny nits). Are there more changes you wanted to make?

david-driscoll

Will wait for @filipw to come online after work (It's 5:30 now?)

Can fix the nits, or just let em pass and fix them later if we need to. 👍

filipw · 2017-05-17T15:55:31Z

Thanks guys, should be good to go.

DustinCampbell · 2017-05-17T16:22:52Z

I won the "merge first contest". 😄 Kicking this again.

filipw requested review from DustinCampbell and david-driscoll May 5, 2017 12:15

filipw commented May 5, 2017

View reviewed changes

david-driscoll approved these changes May 5, 2017

View reviewed changes

DustinCampbell requested changes May 7, 2017

View reviewed changes

filipw added 4 commits May 15, 2017 17:10

added code actions folder support

ecbd327

moved to multi folder support

71342f1

fixed null ref

1c7ec8c

addressed some CR

f51ad56

filipw force-pushed the features/code-actions-folder branch from cabd715 to da1bdb6 Compare May 15, 2017 15:10

filipw added 2 commits May 15, 2017 17:10

added ExternalCodeActionProvider

0df59c0

CodeActionOptions -> RoslynExtensionsOptions

da1bdb6

Merge branch 'dev' into features/code-actions-folder

7148e38

DustinCampbell approved these changes May 17, 2017

View reviewed changes

Merge branch 'dev' into features/code-actions-folder

9ff5c31

david-driscoll approved these changes May 17, 2017

View reviewed changes

minor cleanup

5d9b63a

Merge branch 'dev' into features/code-actions-folder

44dbde1

DustinCampbell merged commit 4e59671 into OmniSharp:dev May 17, 2017

filipw deleted the features/code-actions-folder branch May 18, 2017 16:32

filipw mentioned this pull request May 18, 2017

Error handling when loading assemblies from an external folder #866

Merged

filipw mentioned this pull request Jun 7, 2017

Added test for loading external code actions #887

Merged

Added an option to configure a code actions folder #848

Added an option to configure a code actions folder #848

Conversation

filipw commented May 5, 2017

filipw commented May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-driscoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented May 5, 2017

filipw commented May 5, 2017

DustinCampbell commented May 5, 2017

filipw commented May 5, 2017 • edited Loading

DustinCampbell commented May 5, 2017

david-driscoll commented May 5, 2017

filipw commented May 5, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented May 15, 2017

filipw commented May 15, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented May 17, 2017

david-driscoll left a comment

Choose a reason for hiding this comment

filipw commented May 17, 2017

DustinCampbell commented May 17, 2017

filipw commented May 5, 2017 •

edited

Loading

filipw commented May 5, 2017 •

edited

Loading