Version mismatch between Roslyn in MSBuild and CodeStyle analyzers in the SDK

[KirillOsenkov]

Not sure if it's a bug, or whether it should go here or in the SDK.

There may be a situation where MSBuild.exe is started from C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\MSBuild.exe, which ships Microsoft.CodeAnalysis.dll, say, 4.2.0.0.

Which MSBuild.exe gets loaded depends on your PATH and which Developer Command Prompt you have open.

At the same time global.json determines which .NET SDK is resolved during the build. So you may end up in a situation where a CodeStyle analyzer is loaded from a location such as:

C:\Program Files\dotnet\sdk\6.0.400-preview.22330.6\Sdks\Microsoft.NET.Sdk\codestyle\cs\Microsoft.CodeAnalysis.CodeStyle.dll

and that version may be built against Roslyn 4.3.0.0. This will result in warnings such as:

CSC warning CS8032: An instance of analyzer Microsoft.CodeAnalysis.MakeFieldReadonly.MakeFieldReadonlyDiagnosticAnalyzer cannot be created from C:\Program Files\dotnet\sdk\6.0.400-preview.22330.6\Sdks\Microsoft.NET.Sdk\codestyle\cs\Microsoft.CodeAnalysis.CodeStyle.dll : Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.3.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified..

It feels like there's a fundamental design flaw that can often lead to a mismatch like this, since Roslyn .dlls ship with MSBuild and the CodeStyle analyzers ship with the SDK. I don't know how to fix this, but perhaps we should consider either shipping the analyzers with MSBuild, or shipping the Roslyn compilers and binaries with the SDK. I'm guessing the former will be drastically simpler than the latter.

[francoistanguay]

Could this explain why we're no seeing this issue with msbuild/our source generators after 17.3Preview 3 update?

unoplatform/uno#9297

[tig]

This broke DocFx too:

dotnet/docfx#8097

[Nirmal4G]

...we should consider either shipping the analyzers with MSBuild, or shipping the Roslyn compilers and binaries with the SDK. I'm guessing the former will be drastically simpler than the latter.

We could ship MSBuild common targets and Roslyn, FSharp targets as SDKs and link them altogether via tasks/targets. I know, it's not simple but we already started on this path. Maybe it's time to see where it leads. See #1686.

[rainersigwald]

@marcpopMSFT don't we have a policy that the analyzers should reference an older Roslyn, congruent with the minimumMSBuildVersion?

[baronfel]

Yes, this policy is documented here, but I believe aspects of this, especially how it relates to source generators, are still contentious.

[marcpopMSFT]

Correct, we ask all analyzers to target the baseline version of Roslyn for a given major release so that if customers end up mixing and matching within that major release, they aren't impacted (so 6.0.1xx-6.0.4xx with 17.0-17.3 analyzers). CC @jaredpar who has opinions here. The main contention is how to make updates that require new Roslyn features more than once a year which we don't have a good answer for solving as long as global.json exists.

[tig]

This issue now exists in the NON-Preview, VS2022 17.3.1 release that just came out.

[craigktreasure]

It was already affecting 17.3.0.

[marcpopMSFT]

@ericstj @chsienki do ya'll know who owns this particular analyzer and are they working on changing the targeted roslyn version (or is there other guidance here)?

[chsienki]

@marcpopMSFT Believe this is one of the built in Roslyn analyzers. They're actually compiled against Roslyn source, not NuGet packages, so seems like they're always going to risk being out of sync.

This feels like a very difficult problem to solve if we're going to keep supporting a newer SDK running on an older MSBuild, as we essentially have an atomic product being split between two delivery vehicles, so not surprising it breaks when those are out of sync :(

@sharwell @mavasani Any thoughts on how we can work around this without pulling them out of the Roslyn repo altogether?

[marcpopMSFT]

Is it possible for the analyzer to turn itself off if run with a version of roslyn it doesn't support? I think we're getting further down the path that it may be hard to build against the right version every time but if we could create a light-up experience, that would provide a better error and migration experience.

[KirillOsenkov]

What do we think about my proposal about shipping the analyzers next to the compilers in MSBuild toolset, so that they’re always matching by definition? Not sure why they are in the SDK to begin with.

[jaredpar]

What do we think about my proposal about shipping the analyzers next to the compilers in MSBuild toolset, so that they’re always matching by definition? Not sure why they are in the SDK to begin with.

That only fixes part of the problem. Essentially it fixes the problem for these operators but doesn't solve the problem for analyzers in general. That is becoming an increasing problem.

The root problem is that analyzers / generators in the .NET SDK have to be hardened to run against many different versions of the Roslyn API surface. That is a very tall ask for most analyzers. I think we really need to find a way to fix that problem. Essentially a given version of the .NET SDK should always use a compatible version of the Roslyn API.

It's a tough problem to fix though but it is something we're actively looking at. Actually have some meetings tomorrow to begin looking at one particular path forward.

[KirillOsenkov]

I think even if it fixes just the CodeStyle analyzers, it will be a huge win. In my view we need to decouple this from the general "fix all analyzers" problem as that seems much more monumental.

I just cloned a repo that targets 17.3 on a machine with 17.2 and getting these:

  CSC : error CS8032: An instance of analyzer Microsoft.CodeAnalysis.RemoveUnnecessaryNullableDirective.CSharpRe
       moveUnnecessaryNullableDirectiveDiagnosticAnalyzer cannot be created from C:\Program Files\dotnet\sdk\6.0.400\Sd
       ks\Microsoft.NET.Sdk\codestyle\cs\Microsoft.CodeAnalysis.CSharp.CodeStyle.dll : Could not load file or assembly
       'Microsoft.CodeAnalysis, Version=4.3.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its depend
       encies. The system cannot find the file specified.

I honestly think it's a bad experience for our ecosystem and we need to move the analyzers from SDK into the MSBuild toolset next to the Roslyn compiler binaries.

[Nirmal4G]

we need to move the analyzers from SDK into the MSBuild toolset next to the Roslyn compiler binaries.

Analyzers, atleast the built-in ones always depend on Roslyn, right? Then, it should've been from the Roslyn folder itself.

A better solution would be expanding the idea of an SDK to include hard/soft dependencies similar to NuGet packages and SDK workloads. That way, we can have SDK packages depending on particular version of a Toolkit that always works, no matter what!

I propose Roslyn and other extensions (like FSharp) present in the MSBuild folder move to SDK pattern. So, they can be provided as both out of band and installed variants.

Something like Microsoft.NET.Analyzers.Sdk depending on Microsoft.NET.Compilers.Sdk which gets used by Microsoft.NET.Sdk.

[jaredpar]

I honestly think it's a bad experience for our ecosystem and we need to move the analyzers from SDK into the MSBuild toolset next to the Roslyn compiler binaries.

It's unfortunately not that easy. It requires a significant amount of reworking of the targets in addition to that.

I think even if it fixes just the CodeStyle analyzers, it will be a huge win

I agree it's a win but it's also work that doesn't actually fix the problem. It's a "fix Microsoft but leave the community behind" type of fix.

[sharwell]

The code style analyzers are version-locked to the version of Roslyn they are developed against. Other analyzers target earlier versions of Roslyn, but these cannot. The only available option is to downgrade global.json to an earlier SDK, or update Visual Studio to a newer release that matches the SDK.

The code style analyzers are frequently updated to use new syntax APIs as the compiler creates new features, and there is no straightforward way to lock the code style analyzer package to an earlier version of Roslyn that does not provide those APIs. We originally tried to avoid this, and it resulted in multiple prolonged cases where the code style analyzers in the SDK failed to recognize new language features with many user-visible bugs as a result.

[jaredpar]

The rough idea I have in mind for solving this problem is:

1. Include the .NET Framework version of the Roslyn compilers in the .NET SDK for Windows only
2. Provide a mechanism for msbuild to use that version of Roslyn compilers vs. what it has inbox: maybe as simple as a property

That would mean when customers mix MSBuild and .NET SDK versions they could choose to use the compilers that came with .NET SDK. That would mean analyzers / generators end up running on the compiler they expect to run on. There would no longer be the version mismatch that comes with always using the MSBuild version of the tools.

This is still speculative, particularly how it the mechanism would engage. But it seems very promising.

[Nirmal4G]

Include the .NET Framework version of the Roslyn compilers in the .NET SDK for Windows only

This is going to create more problems than it solves. I do agree that framework tools and binaries to be included as a part of .NET SDK since it's no longer called Core SDK. You could extend the current workload feature to ship framework tools and binaries and remove those from VS MSBuild.

I think there was some conversation about moving desktop MSBuild into .NET SDK or back into programs folder as standalone tool. But that requires a lot of refactoring and breaking changes.

Though, IMO expanding the idea of MSBuild Extensions and SDKs is not a bad solution and requires very little effort from MSBuild's side. It'll also benefit Roslyn and other tools versioning with just repackaging since NuGet can take care of that.

[rainersigwald]

This is going to create more problems than it solves.

What problems does it create?

remove those from VS MSBuild.

This isn't possible, because it would break projects that don't use the .NET SDK.

Though, IMO expanding the idea of MSBuild Extensions and SDKs is not a bad solution and requires very little effort from MSBuild's side. It'll also benefit Roslyn and other tools versioning with just repackaging since NuGet can take care of that.

Can you give a bit more detail on your idea here? I don't see a low-effort solution to this problem.

[jaredpar]

This is going to create more problems than it solves

I don't see how this creates really any problems. It means that the .NET SDK would contain both the .NET Framework and .NET Core versions of the compiler. That means that msbuild can effectively choose to use the compiler it ships with or the one from the .NET SDK. There would be no change to the dotnet build based scenarios (they work just fine today).

The root problem we have here is that tools shipped in the .NET SDK expect to use the compiler that shipped with the SDK. They don't account for the fact that they can be run on many different versions of the compiler (older and newer). When that actually happens it results in errors like the above where APIs depended on are missing and the analyzers / generators cannot run.

Shipping the .NET Framework compiler int the .NET SDK means we can always use the compiler the .NTE SDK tools were expecting to use. The solution just loads the compiler from .NET SDK instead of MSBuild. At that point mixing .NET SDK and MSBuild versions doesn't become the pitfall that it is today (for analyzer / generator scenarios at least). It just works like developers expect it to.

Note: for the most part you could just use the .NET Core compiler that comes with the .NET SDK and get the same benefits. It's not a perfect solution though. A number of components assume that if they're running on .NET Core MSBuild they get a .NET Core compiler and vice versa. So while it'd solve many scenarios it would break others which is why we've been leaning towards the solution I outlined

This isn't possible, because it would break projects that don't use the .NET SDK.

+100.

[marcpopMSFT]

@jaredpar I thought in our meeting last week we were going to try using the .net core version of Roslyn from the SDK rather than including the framework versions in the SDK. Have issues come up that would change that plan? This would all be tied to a setting customers would be able to opt into which limits the risk to some degree.

CC @Forgind

[jaredpar]

Have issues come up that would change that plan?

One issue I brought up with @rainersigwald is that this subverts existing expectations. Customers today understand that $(MSBuildRuntimeType)' != 'Core' means a .NET Framework compiler and tasks are going to be used and vice versa for when a 'Core' runtime is used. For example customers use this today to swap in analyzer / generators which target .NET Core when running on a .NET Core compiler. This is an advanced scenario but customers do employ it. Swapping in a .NET Core compiler in .NET Framework MSBuild would break this.

There are subtle other issues to deal with like finding the right dotnet to invoke the compiler with. Today in the .NET Core tasks we can essentially say "use the dotnet which was used to invoke the build". That's an extremely safe bet to make. When the build is on .NET Framework though it gets more muddy. Different builds could cause us to pick different runtimes which we may or may not have tested against. It's swapping one scenario we don't test for another we don't test.

All this means that it's also hard for us to commit to doing this swap by default in the long term. That means the end result is customers have to mix MSBuidl and .NET SDK versions, get bit by a compilation error, google the error, read a post, then set a variable before their builds start to work.

Where as if we ship the .NET Framework compiler in the Windows .NET SDK all of these questions and uncertainties go away. It is the literal compiler that we test with so swapping it in is a known quantity. There are no weird corner cases to deal with. It is just going to work like the customer expected it to.

[marcpopMSFT]

In the situation I outlined, wouldn't MSBuild still be the framework version from within VS and it's just be Roslyn running in a .NET core task host? Do we know how many cusdtomers use MSBuildRuntimeType and do we know for sure they'd be impacted here (as I assume that'd still say framework).

On top of that, dotnet build currently uses the .net core tooling already so I'm not sure a customer would want to have significantly different behavior running in core tooling versus framework tooling (or else you couldn't use dotnet build).

I think this comes back to how we are each thinking of this. I'm thinking of the switch as the customer saying "I want the SDK tooling" and in my mind, that means .net core. I think you're seeing it as "I want the tooling to match the SDK version but still be the VS tooling". This would already been fairly limited in scope (hopefully).

I should add that I think running the .net core task is something we better understand (though the work is not that simple) compared with the setup authoring of pulling all the framework components into the SDK install.

[jaredpar]

In the situation I outlined, wouldn't MSBuild still be the framework version from within VS and it's just be Roslyn running in a .NET core task host?

Correct. MSBuild would be .NET Framework and it'd be running a .NET Core compiler. That is the core issue though, customers expect those to be related. Breaking that relationship would break the code they wrote to depend on it.

Do we know how many cusdtomers use MSBuildRuntimeType and do we know for sure they'd be impacted here (as I assume that'd still say framework).

Can get a sense of the scale of usage with a grep query

As for "do we know for sure they'de be impacted" the answer for some is "yes". Several customers multi-target their analyzer / generators between netstandard2.0 and .NET Core. Typically because it was the only way for them to get dependency management correct in .NET Core. Those customers then drop a props file in their NuPkg and use MSBuildRuntimType to decide which generator will load. If we break this relationship we break that scenario.

I think you're seeing it as "I want the tooling to match the SDK version but still be the VS tooling". This would already been fairly limited in scope (hopefully).

The way I'm thinking of it is a bit different. I want the solution which fixes broadest set of problems while introducing the fewest new ones. I originally was a big fan of just use the .NET Core compiler in .NET SDK. But as I went through past discussions with customers and the known set of problems I drifted away from it. Yes this would fix a significant set of our problems. But it also concretely introduces new ones. Using the .NET Framework compiler means we're effectively using what we originally tested so it's easy to have a lot more confidence in it.

Given how painful this has been so far, rather than fixing most of it I'd rather fix all of it (assuming the cost isn't that much different).

I should add that I think running the .net core task is something we better understand (though the work is not that simple)

I don't fully agree with this. It introduces a new scenarios which is using the machine .NET Core install to drive the compiler. The compiler is tuned to the runtime and every release of the runtime introduces changes the compiler has to account for. As the runtime on the machine moves forward, but the compiler stays constant, these bugs will begin to manifest in compilation. Also as I mentioned earlier there are specific customer scenarios that will break when we do this.

compared with the setup authoring of pulling all the framework components into the SDK install.

I don't think that is going to be too expensive. Most of the cost is on my team to restructure our NuPkg. The SDK consumption is mostly just xcopying our contents. I think we'd need to condition part of the copy to be windows only and not much else.

[jaredpar]

Forgot to publish our meeting notes on how we're going to move forward with this. In short:

1. Roslyn is going to produce a package containing the desktop compilers: Microsoft.Net.Compilers.Framework.
2. .NET SDK is going to support a new property inside of Directory.Build.props called <UseRoslynSdk>. When this is truthy and building from .NET Framework the desktop compilers will be installed and used vs. grabbing the compilers from the MSBuild install

This will fix the analyzer / generator inconsistency issues because it means the desktop compiler will match the expected API for analyzers and generators.

[vers-one]

So, what is the solution? Does the latest .NET SDK support the <UseRoslynSdk> property? When I search for UseRoslynSdk on GitHub, I get only one result (this issue). In fact, even Google returns only one result which is this GitHub issue.

[Forgind]

We changed to using BuildWithNetFrameworkHostedCompiler instead of UseRoslynSdk.

PR is here: dotnet/sdk#29787

So yes! We now support that. I also have a PR out to backport it to 7.0.3xx; we'll see if we need it in an earlier branch or not later.

[marcpopMSFT]

the 7.0.3xx port was approved for release in July. There won't be earlier ports as 7.0.2xx is out of support and the Roslyn side changes aren't in 17.4 (7.0.1xx).