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

Multi-target compiler to netstandard2.0 and netcoreapp3.1 #40766

Closed
5 tasks
jaredpar opened this issue Jan 6, 2020 · 10 comments
Closed
5 tasks

Multi-target compiler to netstandard2.0 and netcoreapp3.1 #40766

jaredpar opened this issue Jan 6, 2020 · 10 comments

Comments

@jaredpar
Copy link
Member

jaredpar commented Jan 6, 2020

The compiler libraries should move to mult-target netstandard2.0 and netcoreapp3.1 where today they only target netstandard2.0. Adding a target for netcoreapp3.1 will provide some benefits to our code base:

  1. Increase the accuracy of our nullable annotations. Our nullable annotations are not as effective as they could be because the entire .NET SDK is effectively oblivious. Further this means we have to duplicate common methods like Debug.Assert and string.IsNullOrEmpty as the flow attributes aren't present in netstandard2.0. This is a significant burden for us as we annotate our source tree.
  2. Allows us to take advantage of .NET Core APIs that could help increase the performance of the compiler. For example our object pooling could be more effective on .NET Core.

This does have a few downsides:

  1. It will add extra overhead to our build in the form of three new libraries. Given we currently build several hundred libraries, particularly when you consider satellite assemblies, this overhead is fairly minimal.
  2. It will add some more rigor to our investigations. Today it's essentially safe to repro a customer bug on either .NET Desktop or Core (whatever is easier for the developer). As we begin to adopt .NET Core specific APIs though it's possible we will end up with runtime specific behavior. Unlikely this will manifest in more than a handful of bugs but it's an item we will need to be aware of.

This specifically includes the following libraries:

  • Microsoft.CodeAnalysis
  • Microsoft.CodeAnalysis.CSharp
  • Microsoft.CodeAnalysis.VisualBasic
  • Microsoft.Build.Tasks.CodeAnalysis

Check list:

  • The netcoreapp target framework in the repository is unified to netcoreapp3.1. Today it is a mix of versions do to timing around when we adopted runtime features like Default Interface Methods.
  • The nuspec file for the libraries is updated to have both the netstandard2.0 and netcoreapp3.1 targets.
  • The Microsoft.Net.Compilers.Toolset package is updated to ship the netcoreapp3.1 binaries
    • Notify @nguerrera when this happens as it will likely cause a bit of work when merging into the SDK
  • Understand the impact on the Shipped / Unshipped API files. The tool wasn't designed for multi-targeting and it's possible this will cause issues.
@jaredpar jaredpar added this to the 16.6 milestone Jan 6, 2020
@jaredpar jaredpar changed the title Multi-target compiler to netstandard and netcoreapp Multi-target compiler to netstandard2.0 and netcoreapp3.1 Jan 6, 2020
@tmat
Copy link
Member

tmat commented Jan 6, 2020

@jaredpar Shouldn't we start with switching the VBCSCompiler to run on .NET Core and keeping netstandard2.0 until that's done? Otherwise, who is gonna be actually using the netcoreapp3.1 assemblies? The build task does not load them.

@jaredpar
Copy link
Member Author

jaredpar commented Jan 6, 2020

@tmat VBCSCompiler is already mult-targeted to net472 and netcoreapp2.1 (as is csc and vbc).

@tmat
Copy link
Member

tmat commented Jan 6, 2020

But we are actually not running it on Windows using .NET Core, right?

@jaredpar
Copy link
Member Author

jaredpar commented Jan 6, 2020

@tmat

But we are actually not running it on Windows using .NET Core, right?

The compiler executables will use the same .NET runtime that the MSBuild process is using. It doesn't consider the underlying operating system. That means if you use dotnet build it will use the netcoreapp2.1 binaries irrespective of operating system.

@tmat
Copy link
Member

tmat commented Jan 6, 2020

I see. But when building from VS that never uses dotnet build, right? So this is only for command line build.

@jaredpar
Copy link
Member Author

jaredpar commented Jan 6, 2020

@tmat

But when building from VS that never uses dotnet build, right? So this is only for command line build.

Correct. That always goes through desktop MSBuild. Hence every tool, including the compiler, is the desktop version.

@tmat
Copy link
Member

tmat commented Jan 7, 2020

It's a bit worrisome that we would effectively have two different compilers once we start using .NET Core specific features and these compilers might potentially produce different results between dotnet build and build in VS (I guess this is [2] above). This would be mitigated if we run VBCSCompiler always on .NET Core.

There would still be difference between IDE analysis and build, but at least build would be consistent.

@jaredpar
Copy link
Member Author

jaredpar commented Jan 7, 2020

It's a bit worrisome that we would effectively have two different compilers once we start using .NET Core specific features and these compilers might potentially produce different results between dotnet build and build in VS (I guess this is [2] above).

Correct that is 2 above.

To be clear though there will be no language feature difference between .NET Desktop and .NET Core (same as today). The difference will be limited to implementation details, mostly around taking advantage of .NET Core APIs that provide better performance. Any language feature difference between .NET Desktop and .NET Core is a bug.

It's possible there will be observable compiler behavior differences between .NET Desktop and .NET Core but that is already true today. Concrete example is there are subtle differences in how analyzers load into the compiler between desktop and core. But that is true today so no change here.

This would be mitigated if we run VBCSCompiler always on .NET Core.

That is something we have looked into a few times but it's more involved than just flipping a switch. There are a couple of items that have to come along with that:

  1. It's a breaking change. Today analyzers are intended to target .NET Standard but there are surely analyzers that target .NET Desktop out there. Moving to always be on .NET Core will break those analyzers. Hence it seems certain that there will need to be at least a transition period where developers can force the compiler back down to Desktop to make progress.
  2. It's requires a lot of OptProf and RPS work. The Visual Studio infrastructure isn't currently setup to instrument and measure the performance of .NET Core applications. VBCSCompiler is presently a hot point in our measurements. Lots of prep work to allow .NET Core into that infrastructure has to happen before we can move the compiler over.
  3. It will further complicate our setup because now we have two .NET Core versions that we have to be aware of: the one in the .NET SDK that we insert into and the one in the Visual Studio that we insert into. In general they should be the same but once we start getting into servicing then it gets a bit more complicated.

@tmat
Copy link
Member

tmat commented Jan 7, 2020

Yes, I am aware that it won't be a simple switch. I'm just suggesting we should at least start working on it asap since it will need work from other teams as well. At some point we would also want to run IDE services on .NET Core, which will have the same problems. #39988 tracks [1]. I think we should enable this requirement by default in the IDE with a possible off switch that we'll remove later on.

jaredpar added a commit to jaredpar/roslyn that referenced this issue Jan 30, 2020
Had to clean up a few nullable annotations now that we are compiling
agaist `netcoreapp3.1` and hence get the full value of the framework
annotations.

This is also problematic though because there are now two places where
the compiler can see nullable attributes that are directly used by the
developer. For example `NotNullWhenAttribute`. This is both defined in
our assemblies for non-netcoreapp target frameworks and provided by the
SDK when targeting `netcoreapp3.1`.

This causes a problem for assemblies which have the following
characteristics:

1. Target `netcoreapp3.1`
1. Reference an assembly targeting `netstandard2.0` which uses our
nullable attributes definition
1. Has IVT into (2) above

These properties essentially define all of our unit test assemblies. In
that environment it's not possible to use nullable attributes in code
because the compiler can't disambiguate which definition of
`NotNullWhenAttribute` to use. This meant I had to temporarily remove a
few attributes until we can complete dotnet#40766.
@jaredpar
Copy link
Member Author

jaredpar commented Apr 2, 2020

This work is complete now.

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

No branches or pull requests

3 participants