sharpenrocks / Sharpen

Visual Studio extension that intelligently introduces new C# features into your existing codebase
https://sharpen.rocks
MIT License
418 stars 32 forks source link

Decide on how to support different versions of Roslyn within a single VS extension #40

Closed ironcev closed 3 years ago

ironcev commented 3 years ago

The issue is explained in detail in: https://github.com/sharpenrocks/Sharpen/tree/master/lab/RoslynDependenciesAtBuildAndRuntime

Here is a short summary:

So, we want to achieve the following:

Proposed solution and its alterantives are described in detail here: https://github.com/sharpenrocks/Sharpen/tree/master/lab/RoslynDependenciesAtBuildAndRuntime

Here is the summary of the proposed solution:

The proof-of-concept of the approach can be seen here: https://github.com/sharpenrocks/Sharpen/tree/master/lab/RoslynDependenciesAtBuildAndRuntime/solution-04

Questions:

ironcev commented 3 years ago

@cezarypiatek Did you ever face a similar problem? Any better ideas? Before I do anything stupid I would appreciate if you could take a look :-)

cezarypiatek commented 3 years ago

@ironcev, Yes, I had the same problem when I wanted to add support for nullable references to my extension while referencing the old roslyn version (VS2017). I ended up with a simpler version of your solution https://github.com/cezarypiatek/MappingGenerator/blob/master/MappingGenerator/MappingGenerator/MappingGenerator/Mappings/SourceFinders/NullableExtensions.cs

I needed only nullable-references related features so I created an abstraction over that and provided two implementations: one for the new versions of roslyn where desired methods are called by reflection and the fallback one for old roslyn versions. Everything is in the single project, but when you need to have access to broader ranger of new roslyn feature then this split into the separated projects seems to be a more maintainable solution.

There's also an option to split your analyzers into a separated projects that reference different version of roslyn and put them in a single VSIX package. VS should load only those that have roslyn version supported by a given VS instance, but there's still on open issue related to that https://github.com/dotnet/roslyn/issues/42538

ironcev commented 3 years ago

Thanks a lot for your detailed and valuable comment @cezarypiatek ! What you are presenting is the fifth possible solution I didn't think of before :-)

The big difference and advantage of your shim-based solution is that you do not need to load any assemblies at runtime. You rely on what is already loaded by VS and just put the right shim in place if the support for a particular method is there. Being someone who has seen thousands of issues coming from loading assembly dependencies in complex real-life constelations I am always for avoiding dynamic loading of assemblies if possible. From that perspective, I like your simplified approach very much. But as you've mentioned, in my case, I'll most likely need access to broader API for certain cases, and programming via shims could end up less maintainable compared to shared projects and direct usage of API in code.

Weighing between those two options - shims without risks of potential problems at assembly loading or direct API access with assembly loading - at this moment, I also see the second one as better fitting. Still, it is good to know if I ever run into any issues with assemblies, I can always switch to the "Option 5", shims.

dotnet/roslyn#42538 is definitely the right way to go for the regular analyzers, and I am looking forward to seeing that feature implemented. It will, unfortunately, be of no direct use to Sharpen, because Sharpen does not use regular Analyzers infrastructure. VS does not recognize Sharpen as a Roslyn analyzer.

Thanks a lot once again! I'll then go with the multiple projects and DLLs loaded at runtime approach.