Newtonsoft.Json is installed as an analyzer

[yaakov-h]

When installing StyleCop.Analyzers, Newtonsoft.Json is installed as an Analyzer.

Is this intentional? Is it possible to ship an Analyzer dependency without installing the dependency itself as an analyzer?

[sharwell]

It's not intentional, but as far as I can tell it's also not avoidable.

[yaakov-h]

What requires it there?

I experimented by commenting out that Analyzer in the csproj, i.e.

  <ItemGroup>
    <!--<Analyzer Include="..\packages\StyleCop.Analyzers.1.0.0-beta013\analyzers\dotnet\cs\Newtonsoft.Json.dll" />-->
    <Analyzer Include="..\packages\StyleCop.Analyzers.1.0.0-beta013\analyzers\dotnet\cs\StyleCop.Analyzers.dll" />
  </ItemGroup>

and StyleCop seems to be running as expected, even after restarting Visual Studio 2015.

[sharwell]

NuGet automatically adds that line because the assembly is located in the StyleCop.Analyzers NuGet package. We don't add it on purpose, but we need to distribute Newtonsoft.Json with the analyzers so the stylecop.json settings work properly.

[yaakov-h]

It's not NuGet itself, but StyleCop.Analyzer's install Powershell script: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers/tools/install.ps1#L41

But yeah, I do now see the problem you mentioned.

[sharwell]

I completely forgot about that custom install script. 😫

[jessehouwing]

It would be better if you were to use the same version of NewtonSoft JSon that ships with Visual Studio... Plus, it now forces all analyzers that depend on this assembly to use at least your version. Wouldn't statically registering an AppDomain.AssemblyResolve event allow you to fetch the assembly either from the same directory the main assembly is stored, or a tools assembly folder?!

http://blogs.msdn.com/b/microsoft_press/archive/2010/02/03/jeffrey-richter-excerpt-2-from-clr-via-c-third-edition.aspx

[sharwell]

It would be better if you were to use the same version of NewtonSoft JSon that ships with Visual Studio...

I haven't noticed any negative interaction between our assembly reference and Visual Studio. What problem(s) would you anticipate solving by changing the version we use?

Plus, it now forces all analyzers that depend on this assembly to use at least your version.

Everything involved uses strong names. What would keep another analyzer assembly from including its own version of Newtonsoft.Json (e.g. 6.x) and referencing that? I would expect both references to load side-by-side.

Wouldn't statically registering an AppDomain.AssemblyResolve event allow you to fetch the assembly either from the same directory the main assembly is stored, or a tools assembly folder?

It's possible, but I'd want a better understanding of the problems with the current approach as well as input from the analyzers team (e.g. @mavasani, @tmeschter) before going down this path.

[jessehouwing]

I tried adding a second reference (from my own analyzer pack) to version 6.05, but only one version sticks. So the analyzer that sets the reference last will probably win, causing funny issues. I don't see a way to set binding redirects for Analyzers, so looking for a fellow-analyzer to load dependent assemblies feels like a bad design decision.

While strong names would work just fine, every AppDomain can only load one version of each strong named assembly at a time, I'm not entirely sure whether analyzers are running in their own AppDomain, which would allow you to load your own version, otherwise the first assembly to load its dependencies wins. This is already causing massive issues with VSIX extensions loading different versions of the same dependent assembly in different orders. I can't say for sure whether one analyzer with a dependency could cause these issues, but multiple will, as it looks like. The issues usually surface as component model cache corruptions: http://stackoverflow.com/questions/20301697/visual-studio-2013-is-unable-to-open-the-test-window

Right now, and for quite some time now, Visual Studio ships with version 6.05 of Newtonsoft.Json and stores in a common location which can easily be resolved.

Alternate options would be to link these dependencies statically or to compile them as internal classes to the analyzer from source. or to load them from a well known location off of the analyzers own assembly location.

I see no need to add your dependencies to the project causing your implementation details to seep into the projects that use the analyzer.

[tmeschter]

@sharwell @jessehouwing If an analyzer assembly A.dll depends on another assembly B.dll, then both must be listed as analyzers. This is to ensure that the host application knows where to find dependencies and can load the correct one.

Exceptions: You do not need to and should not include the core system assemblies (e.g., mscorlib.dll, System.dll) as you generally have no control over those, or the core Roslyn assemblies (e.g., Microsoft.CodeAnalysis.dll) or their dependencies (e.g., System.Collections.Immutable.dll) as those will be provided by the host application and you can assume they are already loaded.

The point of this requirement is to ensure consistent analyzer behavior, especially from one build to the next. Consider what would happen if your analyzer ended up with one copy of Newtonsoft.Json.dll in VS, and a different copy when building in the command line. You could conceivably get different results from your analyzer. You could run into even worse problems when compilation is handled by vbcsserver.exe. Since that is a long-running process that will handle multiple builds, exactly which dependencies are loaded could depend on the order in which the builds occur. Which means that the set of diagnostics reported could also be dependent on the build order, or even whether or not the build succeeds.

We avoid these issues by requiring that dependencies be specified. That way we can try to load exactly the assembly that is expected, and if we can't, we can check to see if that is OK or might pose a problem.

I don't have time for a fuller explanation (I'll get back to that later), so here's some brief guidance:

1. Specify dependencies as analyzers.
2. Use strong name signing if at all possible.
3. Do not use AppDomain.AssemblyResolve in your analyzers. You will end up fighting the assembly loading logic in the host application (and losing).

[sharwell]

I tried adding a second reference (from my own analyzer pack) to version 6.05, but only one version sticks.

I'm having trouble reproducing this behavior. I added Newtonsoft.Json 6.0.4 as an additional analyzer reference, and everything seems to still be working. This worked whether I placed it as the first analyzer or the last analyzer in the list. It even showed up twice in the list of analyzers:

❓ Is the other analyzer pack open source and available so I can test with it as well?

I don't see a way to set binding redirects for Analyzers...

Our deployment is relying on side-by-side loading of assemblies with different versions. Binding redirects override this behavior and cause different assemblies to be treated as equivalent. In other words, I would expect things to work correctly now, but binding redirects would actually break them.

While strong names would work just fine, every AppDomain can only load one version of each strong named assembly at a time...

This is not (normally) correct. When using strong names, two assemblies which differ only by their assembly version are treated as two completely different assemblies and can be loaded side-by-side in the same AppDomain. There may be ways to override this behavior, but if things weren't already confusing enough with the default behavior for strong-named assemblies...

... This is already causing massive issues with VSIX extensions loading different versions of the same dependent assembly in different orders. I can't say for sure whether one analyzer with a dependency could cause these issues, but multiple will, as it looks like. The issues usually surface as component model cache corruptions: ...

I believe the problems you refer to here are actually related to the way MEF locates parts. There is a good chance that one or more MEF-exported components failed to follow one or more items in Assembly Versioning in Extensible Applications (search for 'MEF' in that page).

[sharwell]

@jessehouwing Is the other analyzer you're working with SonarSource/sonarlint-vs? If so, it appears from the project file (this is the only project I saw that references Newtonsoft.Json) that the analyzer assembly for that project does not have a strong name. You will need to add a strong name to your assembly before side-by-side assembly loading will work.

[sharwell]

@tmeschter I'm going to take a guess that the loading logic behaves as follows. Suppose AnalyzerA does have a strong name and references X version 2. Also suppose AnalyzerB does not have a strong name and references X version 1.

If both AnalyzerA and AnalyzerB are added to a project, then one of the following sequences will occur:

1. AnalyzerA assembly loads
2. X version 2 loads (required since AnalyzerA has a strong name)
3. AnalyzerB assembly loads
4. X version 2 is already loaded and reused (since AnalyzerB does not have a strong name, and 2 ≥ 1)

Alternately:

1. AnalyzerB assembly loads
2. X version 1 loads (not required, but suppose it was located first during probing) → AnalyzerB is now using X version 1.
3. AnalyzerA assembly loads
4. X version 2 loads, and is used by AnalyzerA (version 1 could not be reused because AnalyzerA has a strong name)

[sharwell]

Alright, I set up a complicated case to experiment with this. In sharwell/StyleCopAnalyzers@ca42b3e, I made our analyzer assembly depend on both Newtonsoft.Json 6.0.0.0 and Newtonsoft.Json 7.0.0.0. If either assembly fails to load with that specific version, half the analyzers which use stylecop.json will fail.

When I installed the updated package (sharwell/StyleCopAnalyzers@da335ec), you can see that the installation script failed to add both references to Newtonsoft.Json, even though they were both present in the .nupkg file. As expected, a large number of warnings were reported in the IDE regarding failure to load analyzers. However, when I manually added the second reference (sharwell/StyleCopAnalyzers@557cac8), everything worked.

It looks like this is a bug in either the installation script or in the project system.

[sharwell]

@jessehouwing I filed dotnet/roslyn#6322 regarding the failure to include both analyzer references when packages are installed through NuGet. For now I'm going to close this as external for the following reasons:

1. We're already following the recommendations listed by @tmeschter above

2. I have reproduced the following behavior which you described, and filed an external bug regarding it

   I tried adding a second reference (from my own analyzer pack) to version 6.05, but only one version sticks.

3. I posted a branch which conclusively demonstrates that two different analyzers can use two different versions of Newtonsoft.Json without negative consequences (aside from the previously mentioned bug)