Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ironcev opened this issue Dec 6, 2020 · 3 comments
Labels

Comments

@ironcev
Copy link
Member

ironcev commented Dec 6, 2020

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

Here is a short summary:

  • At compile-time we compile Sharpen Engine with a certain version of Roslyn. At the moment, this is a very old v2.4.0.0.
    This way we do not have access to the new Roslyn API like e.g. IOperation but we assume that most of the analyzers can be comfortably written using this old Roslyn version.
  • At runtime, VS will bind Sharpen Engine to the version of Roslyn coming with that particular instance of VS. This can be anything between v2.4.0.0 and the newest version of Roslyn.
  • Here we face two issues:
    • If there are breaking changes in the API between the v2.4.0.0 and the actual runtime version, Sharpen will crash.
    • If the newer version supports some language constructs required for the proper analysis, we cannot use them.
      E.g. , IsNullableContextEnabled(...) method can simply return false in VS2017 but must do some actual work in VS2019. This is the reason for the issue False positives in c# 8 nullable reference suggestions #38.

So, we want to achieve the following:

  • Have just a single VSIX that supports VS2017+ and different versions of Roslyn.
  • Be able to develop the analyzers by using the newer language feature when available at runtime, and using the appropriate fallbacks when they are not available. E.g. IsNullableContextEnabled(...) method has different implementations for Roslyn version older than v3.0.0.0 (C# 8.0) and newer or equal then v3.0.0.0.

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:

  • Create an abstract interface of the Sharpen.Engine. Put it in the Sharpen.Engine.Abstractions and compile against v2.4.0.0 of Roslyn. This interface will require only the very basic Roslyn types like SyntaxTree and Workspace.
  • Create a shared project called Sharpen.Engine. Put all the common code here. Implement the Sharpen.Engine.Abstractions here.
  • Create separate project for each supported version of Roslyn, reference the shared Sharpen Engine project, and compile it against the specific version of Roslyn. Add Roslyn version-specific code to those projects.
  • In the VS Extension, link to Engine DLLs produced by each of the projects and copy them to the VSIX as files.
  • In the VS Extension, load the closest of those DLLs at runtime based on the exact Roslyn version used at runtime.

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

Questions:

  • Is this the final way to go?
  • Are there any better alternatives?
@ironcev
Copy link
Member Author

ironcev commented Dec 6, 2020

@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
Copy link

cezarypiatek commented Dec 7, 2020

@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 dotnet/roslyn#42538

@ironcev
Copy link
Member Author

ironcev commented Dec 7, 2020

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.

@ironcev ironcev closed this as completed Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants