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

System.Text.Json does not work with Roslyn analyzers #41785

Open
kolkinn opened this issue Feb 19, 2020 · 13 comments
Open

System.Text.Json does not work with Roslyn analyzers #41785

kolkinn opened this issue Feb 19, 2020 · 13 comments
Assignees
Labels
Area-Analyzers Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality
Milestone

Comments

@kolkinn
Copy link

kolkinn commented Feb 19, 2020

Version Used:
Microsoft (R) Visual C# Compiler version 3.4.1-beta4-19614-01 (1650460)

Steps to Reproduce:

  1. Install System.Text.Json as a dependency in your analyzer package
  2. Use System.Text.Json in your analyzer
  3. Run your analyzer

I have made a simple reproduction branch, using the MakeConst sample in from dotnet/samples:
https://github.com/mikaelkolkinn/samples/tree/roslyn-json-repro
Do the following to reproduce the issue using the provided repository:

  1. Open the MakeConst solution
  2. Start the MakeConst.Vsix project
  3. Open a solution with the instance of Visual Studio that was spawned
  4. Create a variable somewhere in your code, like this var couldBeConst = "hello world";
  5. Observe that there is no diagnostics saying the variable could be made a constant

Expected Behavior:
System.Text.Json works and lets you deserialize JSON.

Actual Behavior:
Analyzer does not run.
Logs show this error:

Unable to load one or more of the requested types. Retrieve the LoaderExceptions property for more information. Could not load file or assembly 'System.Runtime, Version=4.2.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified. Could not load file or assembly 'System.Text.Json, Version=4.0.1.1, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified.

@jinujoseph jinujoseph added Area-Analyzers Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality labels Feb 19, 2020
@jinujoseph jinujoseph added this to the 16.6 milestone Feb 19, 2020
@sharwell
Copy link
Member

sharwell commented Feb 19, 2020

📝 It's currently very difficult for analyzers to have any dependencies. All of the analyzer projects I've worked on that had dependencies eventually removed them (either by embedding or replacement with equivalent standard functionality) due to inconsistent behaviors observed across the many environments where analyzers run.

StyleCop Analyzers needed JSON functionality and ended up embedding a modified LightJson implementation:
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/tree/master/StyleCop.Analyzers/StyleCop.Analyzers/LightJson

@jinujoseph jinujoseph modified the milestones: 16.6, Backlog Feb 19, 2020
@kolkinn
Copy link
Author

kolkinn commented Feb 19, 2020

📝 It's currently very difficult for analyzers to have any dependencies. All of the analyzer projects I've worked on that had dependencies eventually removed them (either by embedding or replacement with equivalent standard functionality) due to inconsistent behaviors observed across the many environments where analyzers run.

StyleCop Analyzers needed JSON functionality and ended up embedding a modified LightJson implementation:
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/tree/master/StyleCop.Analyzers/StyleCop.Analyzers/LightJson

Very interesting @sharwell, I will take a look at LightJson.
I was hoping to use System.Text.Json to decrease the need for memory allocations, but LightJson might cut it.

@jmarolf
Copy link
Contributor

jmarolf commented Feb 20, 2020

I would move this to the Roslyn-sdk repo. we should at least have a system that allows dependencies to be packed correctly into your analyzer nuget package.

@mikaelkolkinn I agree with @sharwell though. Even if we made dependency deployment work, json is a problem. Almost every host that is going to load you is going to have some version of a json serializer (likely the exact same package but a different version) and reasoning about how to resolve those conflicts across both .NET Framework analyzer hosts and .NET Core Analyzer hosts is very hard.

💭: should we just provide a utility json serializer to analyzer authors since 90% of the time the reason they are going to add dependencies to their analyzer project it is going to be for json serialization?

@tmat
Copy link
Member

tmat commented Feb 20, 2020

We should focus on switching VBCSCompiler and Roslyn ServiceHub hosts, which are the main analyzer hosts, to run on .NET Core. Once we do that analyzer dependencies are not gonna be a problem since we can isolate each analyzer in a separate AssemblyLoadContext.

@mavasani
Copy link
Contributor

@genlu @tmat I believe Gen's changes to move OOP to core host should have taken care of #41785 (comment). Is there anything else to be done here?

@mavasani mavasani assigned genlu and unassigned mavasani Jan 13, 2022
@genlu
Copy link
Member

genlu commented Jan 13, 2022

Enabling .Net 6 host would address this in IDE, but I don't think the compiler shipped in VS today run on .Net 6 yet.

@benrobot
Copy link

I'm working on an analyzer and started having issues when I began depending on System.Text.Json. It sounds like I should forget about depending on any external libraries. Is that correct?

@genlu
Copy link
Member

genlu commented Jun 10, 2022

@benrobot That depends, your analyzer with external dependencies would work fine when building with SDK (e.g. dotnet build) as long as the dependencies are packaged properly in your analyzer nuget package. But in VS, it will take us a little longer to completely move both the IDE analyzer host and vbcscompiler to the ALC based version.

Measurity added a commit to Measurity/Nitrox that referenced this issue Dec 3, 2022
Visual Studio cannot find external libraries compiled with analyzers. See issue:
dotnet/roslyn#41785 (comment)
For now we regex parse the English localization JSON file.
Measurity added a commit to SubnauticaNitrox/Nitrox that referenced this issue Dec 4, 2022
* Added analyzer project for analysis and code gen

* Added comment why netstandard2.0 is used for analyzers

Analyzers MUST target netstandard2.0, do not update unless Microsoft (MSDN) says it's possible.

* Added JetBrains.Annotation package and applied it to Validate.NotNull API

Co-authored-by: Jannify <[email protected]>

* Changed where Validate.IsTrue was used for null checking

* Added codefix for null propagation on UnityEngine.Object

* Added "is null" analyzer for unity object lifetime

* Added code fixer for "?." on UnityEngine.Object

* Fixed JetBrains namespace collision with UnityEngine.Core

* Fixed invalid null propagation on UnityEngine.Object

* Set "Nitrox" as default style and fixed expression bodied members getting reverted

* Added GetFixAllProvider override to UnitySkippedObjectLifetimeFixProvider

CodeFixProvider types should override it by convention.

* Added ?? analysis to UnitySkippedObjectLifetimeAnalyzer

* Disabled resharper analysis of ?? for UnityEngine.Object

* Set MSBuild property NitroxProject to false by default for all projects

* Added LocalizationAnalyzer.cs

This analyzer detects if localization key exists in the en.json (English localization). If it does not, then a warning is emitted.

* Disabled localization analyzer if en.json is missing

* Changed name of field invalidLocalizationKey to invalidLocalizationKeyRule

* Added analyzer to prefer interpolated strings over string concat

* Fixed typo in comment of StringUsageAnalyzer

* Refactored unity object lifetime analyzer

Added simple ITypeSymbol.IsType() that will properly resolve inheritance tree.

* Used string interpolation in Nitrox.Analyzer project where possible

* Added Increment source generator support to Nitrox.Analyzers

* Improved accuracy of type symbol compare in UnitySkippedObjectLifetimeAnalyzer.cs

Changed analyzer to get the type symbol of UnityEngine.Object from the compilation context at startup.

* Changed debug logger to apply on any type

* Added dynamic resolve for System.Text.Json based on runtime used

* Removed dependency on System.Text.Json

Visual Studio cannot find external libraries compiled with analyzers. See issue:
dotnet/roslyn#41785 (comment)
For now we regex parse the English localization JSON file.

Co-authored-by: Jannify <[email protected]>
@benrobot
Copy link

@benrobot That depends, your analyzer with external dependencies would work fine when building with SDK (e.g. dotnet build) as long as the dependencies are packaged properly in your analyzer nuget package. But in VS, it will take us a little longer to completely move both the IDE analyzer host and vbcscompiler to the ALC based version.

@genlu About 14 months ago you said it will take a little longer for VS to catch up. Is there any update on this?

@genlu
Copy link
Member

genlu commented Aug 28, 2023

But in VS, it will take us a little longer to completely move both the IDE analyzer host and vbcscompiler to the ALC based version.

Roslyn language service is now default to run on .NET 8 in VS. I'm not sure about vbcscompiler though. Tag @jaredpar for the compiler server part.

@jaredpar
Copy link
Member

The VBCSCompiler process, and by extension msbuild style invocations, don't have a timeframe for moving to AssemblyLoadContext. That would necessitate that the compiler be able to run on .NET Core and that isn't an option in the msbuild install today.

@benrobot
Copy link

Thank you for the update @genlu and @jaredpar . I'm going to try to summarize the current state, in my own terminology. Please correct me if I said something incorrect.

Summary

If I write an analyzer that depends on NuGet packages then it will fail within Visual Studio. My options are to either:

  1. Include source code of my dependencies within my analyzer project
  2. Only target users that don't use Visual Studio or any other msbuild based IDE

There's no timeframe for resolving this issue at this time.

@jaredpar
Copy link
Member

If I write an analyzer that depends on NuGet packages then it will fail within Visual Studio

The core scenarios for analyzers are Visual Studio, VS Code, msbuild and dotnet build. The vast majority of dependencies used via NuPkg work just fine in these scenarios. There are plenty of customers who are leveraging significant dependency chains in these scenarios successfully.

Every host listed above has some control over how dependencies are loaded. For example Visual Studio requires that their version of dependencies be loaded. That means if your analyzer and Visual Studio share a dependency, like System.Text.Json, then the version Visual Studio ships will win at execution time. That is a policy decision of the host and analyzers need to be aware of that when building their solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality
Projects
None yet
Development

No branches or pull requests

9 participants