dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.56k stars 4.54k forks source link

Consider deprecating older source generators and their associated infrastructure #83383

Open ViktorHofer opened 1 year ago

ViktorHofer commented 1 year ago

See https://github.com/dotnet/runtime/pull/82179#issuecomment-1466613610 for the initial conversation.

Packages which include source generators are growing unboundedly because they not just contain assemblies targeting the current roslyn but also carry along all the previous targeted source generators, i.e. roslyn3.11, roslyn4.0 and roslyn4.4. Those never get deleted because the package could be loaded in an environment (SDK / Visual Studio) that doesn't support the newer version of the compiler.

That effectively means that we need to support our roslyn3.11 source generators until the minimum version of Visual Studio that supports the generators is EOL. Based on the official lifecycle document, that is April 2029.

Consequences based on that:

Those pain points could be mitigated by applying a required toolset version to our packages. While the toolset version refers to the minimum NuGet client version, the compiler and NuGet both ship in the SDK and Visual Studio and by picking a specific NuGet version, a compiler version is enforced as well. Alternatively we could discuss if we want to add another metadata to the nuspec file that indicates the required compiler version. Downside of that is that we would need to backport that support into those very old versions of the SDK and Visual Studio.

By applying a minimum required toolset version, we could remove those old source generators, their underlying infrastructure and with that make our packages smaller and less complex. The downside in doing that is that we would prevent customers on older environments to reference the very latest versions of our packages.

cc @ericstj @eerhardt @jaredpar @stephentoub @jkotas

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries See info in area-owners.md if you want to be subscribed.

Issue Details
See https://github.com/dotnet/runtime/pull/82179#issuecomment-1466613610 for the initial conversation. Packages which include source generators are growing unboundedly because they not just contain assemblies targeting the current roslyn but also carry along all the previous targeted source generators, i.e. roslyn3.11, roslyn4.0 and roslyn4.4. Those never get deleted because the package could be loaded in an environment (SDK / Visual Studio) that doesn't support the newer version of the compiler. That effectively means that we need to support our roslyn3.11 source generators until the minimum version of Visual Studio that supports the generators is EOL. Based on the [official lifecycle document](https://learn.microsoft.com/en-us/lifecycle/products/visual-studio-2019), that is April 2029. Consequences based on that: - Packages are growing unboundedly whenever their underlying source generators target a new version of roslyn. - Old / legacy source generator source code can't be removed. In the example of roslyn3.11 source generators, they aren't incremental and cause performance issues. - Custom infrastructure for polyfilling SDK features in older environments adds complexity to packages. That infrastructure is repository specific (their is no shared code today at all) and written in msbuild. A package consumer needs to pay for evaluating that custom infrastructure even on a current environment. - Whenever a new source generator is added, we need to add new projects and grow the build graph as msbuild doesn't allow multi-targeting based on the compiler version. Those pain points could be mitigated by applying a [required toolset version](https://learn.microsoft.com/en-us/nuget/reference/nuspec#minclientversion) to our packages. While the toolset version refers to the minimum NuGet client version, the compiler and NuGet both ship in the SDK and Visual Studio and by picking a specific NuGet version, a compiler version is enforced as well. Alternatively we could discuss if we want to add another metadata to the nuspec file that indicates the required compiler version. Downside of that is that we would need to backport that support into those very old versions of the SDK and Visual Studio. By applying a minimum required toolset version, we could remove those old source generators, their underlying infrastructure and with that make our packages smaller and less complex. The downside in doing that is that we would prevent customers on older environments to reference the very latest versions of our packages. cc @ericstj @eerhardt @jaredpar @stephentoub @jkotas
Author: ViktorHofer
Assignees: -
Labels: `area-Infrastructure-libraries`, `discussion`
Milestone: 8.0.0
sharwell commented 1 year ago

... In the example of roslyn3.11 source generators, they aren't incremental and cause performance issues. ...

This isn't really the case. In practice, one of two things is going to happen:

  1. The source generator is being used in a non-incremental environment (e.g. build0, where ISourceGenerator and IIncrementalGenerator exhibit very similar performance characteristics.
  2. The environment doesn't support something newer, in which case the generators are running as fast as is possible for the environment.
sharwell commented 1 year ago

Packages are growing unboundedly whenever their underlying source generators target a new version of roslyn.

Note that it's possible this has a natural upper bound. As the performance of the infrastructure is tuned to the source generators and stabilizes, we may find that newer releases of Roslyn require little to no changes to the source generators themselves. I'm concerned that by posing the question so soon after the introduction of significant features targeting runtime source generator scenarios (resulting in relatively large changes to the generator implementations), our outlook on the past year may be biasing our view of what the next 6 years look like for the same generators.

jkotas commented 1 year ago

our roslyn3.11 source generators until the minimum version of Visual Studio that supports the generators is EOL. Based on the official lifecycle document, that is April 2029.

.NET Core lifecycle forces you to upgrade Visual Studio much faster. Why do we need to worry about Visual Studio lifecycle?

ViktorHofer commented 1 year ago

While that lifecycle applies to modern .NET, packages like System.Text.Json support netstandard2.0 as well and include source generators. AFAIK, nothing hinders you from consuming an 8.0.x package on and old Visual Studio installation and target netstandard2.0.

jkotas commented 1 year ago

The linked PR has the tests ifdefed for netcoreapp - https://github.com/dotnet/runtime/pull/82179/files#diff-415bb466216b8b4fd07992787af62f30e8a2d6d83fff4b627b88176628db7731R14 . I assume that this source generator is not going to support netstandard2.0. Is that correct?

I expect that it is going to be common for new source generators to depend on new C# language features that will make it hard to target netstandard2.0. We do not generally support new C# language features with netstandard2.0. Can we limit source generators to netcoreapp targets only?

If we have shipped some netstandard2.0 targeting source generators already, can we tell how many people are actually using them with netstandard2.0? If the set is small, can we drop that support or require Visual Studio upgrade?

ViktorHofer commented 1 year ago

I assume that this source generator is not going to support netstandard2.0. Is that correct?

You mean the new one that @layomia is working on, right? He currently targets netstandard2.0: https://github.com/dotnet/runtime/pull/82179/files#diff-5cd2b9bb815efd906e5edd5f1c4bc5b1a3e5dc6e6d34195fd313e75b989c2ed5R3

When targeting modern .NET, how would Roslyn running on .NET Framework inside Visual Studio handle loading a .NETCoreApp only source generator?

stephentoub commented 1 year ago

I expect that it is going to be common for new source generators to depend on new C# language features that will make it hard to target netstandard2.0.

You mean the new one that @layomia is working on, right? He currently targets netstandard2.0:

There are two relevant versions with a source generator: what the source generator requires in order for itself to run, and what the code it generates requires. I believe (though he can correct me if I'm wrong) what Jan is asking about is the latter, e.g. that source generator is emitting code that uses nullable (https://github.com/dotnet/runtime/pull/82179/files#diff-eaa3c60ce61aebffabb88f108fdbb371de78fdd4b2d0526c26b24bc60e2047bbR2), which requires a C# language version of at least C# 8, but .NET Framework and .NET Standard 2.0 only officially support using language version 7.3.

jkotas commented 1 year ago

Yes, I am talking about the language version required by the source generator.

jaredpar commented 1 year ago

Why do we need to worry about Visual Studio lifecycle?

Even though the .NET lifecycle for runtimes moves faster, the lifecycle for TFMs does not in the .NET SDK. It is still legal and supported to target netcoreapp3.1 for libraries for build and design time and will be until we change the .NET SDK to put a finite lifetime on it (seems unlikely). That means by extension it's supported to do this in Visual Studio.

Once source generators enter the picture it means that the .NET SDK has design time assets that are a part of the editing experience. Those need to be setup for servicing for the lifetime of the Visual Studio they run in. How the servicing works is a bit more flexible but they do need to be setup for servicing.

jkotas commented 1 year ago

It is still legal and supported to target netcoreapp3.1

How can that be still legal and supported when the .NET Core 3.1 is out of support and we are not shipping servicing updates for it anymore? If we are not shipping servicing updates -> it is not supported.

eerhardt commented 1 year ago

I assume that this source generator is not going to support netstandard2.0.

For this specific source generator, we would like to use it ourselves in a netstandard2.0 library - Microsoft.Extensions.Logging.Console (see #82098 where we hand-wrote the code that we would like to be replaced with the source generator). I assume we have other potential consumers of it that are targeting netstandard2.0, but I haven't verified it.

jaredpar commented 1 year ago

How can that be still legal and supported when the .NET Core 3.1 is out of support and we are not shipping servicing updates for it anymore? If we are not shipping servicing updates -> it is not supported.

For the .NET SDK it remains a supported story to build apps, libs, etc ... targeting older TFMs. There is nothing stopping customers from doing this and we go to lots of lengths to continue making it work.

@marcpopMSFT

ViktorHofer commented 1 year ago

While you are talking about netcoreapp3.1, it isn't clear to me why this doesn't apply to netstandard2.0 and net462 as well. Am I missing something? When consuming our packages, the sdk automatically picks up the source generators which are all compiled against netstandard2.0, and roslyn then loads them.

I.e. a customers using VS 2019 can use our source generators when building a netstandard2.0 library and that version of VS only supports roslyn3.11. How does the .NET Core lifecycle policy come into play here?

EDIT: Note that @stephentoub just found out that our out-of-band source generators produce non-compileable code when targeting .NET Framework or netstandard2.0 and using official supported lang versions:

sharwell commented 1 year ago

Note that @stephentoub just found out that our out-of-band source generators produce non-compileable code when targeting .NET Framework or netstandard2.0 and using official supported lang versions:

The Microsoft.CodeAnalysis.Testing library can significantly help with this. It allows testing a source generator against a specific defined target framework, with options to either fully validate the output content produced by the generator, or just validate that the code produced by the generator builds without errors.

stephentoub commented 1 year ago

The Microsoft.CodeAnalysis.Testing library can significantly help with this.

We don't require any new components: the tests there simply aren't setting a language version. When I add the one argument to do so, the tests stop passing as expected.

jaredpar commented 1 year ago

While you are talking about netcoreapp3.1, it isn't clear to me why this doesn't apply to netstandard2.0 and net462 as well. Am I missing something?

It applies to all TFM that I'm aware of. We fairly regularly get support bugs where customers are targeting all manner of TFM.

I.e. a customers using VS 2019 can use our source generators when building a netstandard2.0 library and that version of VS only supports roslyn3.11. How does the .NET Core lifecycle policy come into play here?

I don't believe the .NET Core lifecycle applies there at all. It's fully supported to build those netstandard2.0 libraries irrespective of the .NET Core lifecycle. If we supported generators in that setup for VS2019 RTM, we're supporting them through the servicing lifetime of VS

jkotas commented 1 year ago

For the .NET SDK it remains a supported story to build apps, libs, etc ... targeting older TFMs. There is nothing stopping customers from doing this and we go to lots of lengths to continue making it work.

Targeting older TFMs requires ref packs. Ref packs include analyzers and source generators. What are we going to do when a customer reports a (security) bug in the source generator that is part of ref pack for out-of-support runtime (e.g. netcoreapp3.1)? I am pretty sure that we are going to say that it is unsupported scenario and tell the customer to upgrade.

There is a difference between .NET SDK not actively blocking targeting out-of-support TFMs, and the scenario being a supported story.

jaredpar commented 1 year ago

Targeting older TFMs requires ref packs. Ref packs include analyzers and source generators.

Understood. I brought this up with @ericstj and @ViktorHofer several times when we were initially working on the runtime generator story specifically to point out the ramifications this had for Visual Studio servicing.

What are we going to do when a customer reports a (security) bug in the source generator that is part of ref pack for out-of-support runtime (e.g. netcoreapp3.1)?

I'm more concerned about performance bugs as they're far more common and tend to be greater drivers of servicing requests for Visual Studio.

marcpopMSFT commented 1 year ago

Including @leecow in case he knows if this is documented anywhere officially. We allow SDKs to continue to target all prior runtimes and try to avoid any changes that would break those scenarios. We didn't want customers to have their builds broken just by updating their SDK.

However, as Jan points out, that's different than the scenario being supported. We would not fix even security bugs when targeting out of support runtimes and if a default scenario broke entirely (eg. building for netcoreapp2.0 completely broke for some reason), we might do a limited investigation to see if it were possible to unblock. If less common scenarios broke (ie anything outside of maybe build/restore), we likely would leave it broken without investigating.

leecow commented 1 year ago

... .NET SDK not actively blocking targeting out-of-support TFMs

This has been the best effort intent from the very beginning though I'm unable to find that we wrote it down in any support-related document.

cc @richlander and @DamianEdwards as they were also part of the original discussions and may remember.

richlander commented 1 year ago

Initially, we wanted to make it possible for people to target old TFMs, even if they were out of support. The ref packs for those TFMs only exposed shape so folks using them seemed fine and the ROI of moving seemed low. I agree with @jkotas that source generators significantly change the game here.

I propose that we update our guidance that we no longer support old TFMs (even for libraries) once a given .NET version goes out of support. The maintenance cost I'm reading about here seems much too high and we should find a way (as proposed) to reduce it.

I believe we already warn at build time when an old TFM is detected. It's likely overkill turning that to an error. We could also make it a warning to reference a package w/only unsupported TFM assets. That would be symmetric.

jaredpar commented 1 year ago

I believe we already warn at build time when an old TFM is detected. It's likely overkill turning that to an error.

Making that an error is not an option. It would be an explicit adoption blocker for many of our largest customers. For example you couldn't even deploy .NET SDK 8 to our own builds if you did this. There are many other 1P customers who reference TFM we consider out of date than we constantly push to grab new .NET SDK for other reasons.

marcpopMSFT commented 1 year ago

Agreed that we can't make it an error. Is there any alternative for source generators that isn't as high cost as a continuously growing package? Could we have separate packages for the out of support ones that we download independently so the in-support versions at least are a controlled size?

DamianEdwards commented 1 year ago

Just echoing what @jaredpar said and @marcpopMSFT agreed to: it's much too high of an adoption blocker to remove the ability to build for unsupported TFMs in new SDKs. New tools must continue to build the older TFMs past that TFM's support lifecycle. That said, I'm not sure it needs to support it indefinitely. We can be data-driven and consider dropping support for older TFMs once their usage drops to levels where we believe the trade-off is worthwhile and the number of impacted users is low.

Could we have separate packages for the out of support ones that we download independently so the in-support versions at least are a controlled size?

This line of thinking seems good. While understanding it likely introduces a new packaging boundary, being able to push out of support elements out of the SDK but allow them to be downloaded on demand seems worthy of discussion/investigation.

richlander commented 1 year ago

Here's a support statement to consider: https://github.com/dotnet/core/pull/8305

teo-tsirpanis commented 1 year ago

Packages are growing unboundedly whenever their underlying source generators target a new version of roslyn.

a continuously growing package

How about we distribute source generators only for Roslyn versions 3.11 (to support VS 2019), 4.0 and the latest it has an interesting feature, and remove the others? If a future 4.x version has an API we want to use, we could drop 4.4 and users on earlier versions of VS 2022 will fall back to the 4.0 edition. Performance will be worse (but not the worst; at least it will still be using incremental generators), but we will be always targeting three Roslyn versions and keep the package's size bounded.

sharwell commented 1 year ago

Source generators shouldn't need to constantly grow in size. To date, they only grew in size when we built a targeted API to address the high-level performance of one or more specific source generators. The opportunities for broad-scoped performance improvements like this is reducing over time, and for some of them (e.g. FAWMN) where the new API only uses types that can also be represented in the older versions, the package could use lightup/reflection to either polyfill or use the compiler feature as necessary from a single assembly.

ghost commented 5 months ago

Tagging subscribers to this area: @dotnet/area-meta See info in area-owners.md if you want to be subscribed.

Issue Details
See https://github.com/dotnet/runtime/pull/82179#issuecomment-1466613610 for the initial conversation. Packages which include source generators are growing unboundedly because they not just contain assemblies targeting the current roslyn but also carry along all the previous targeted source generators, i.e. roslyn3.11, roslyn4.0 and roslyn4.4. Those never get deleted because the package could be loaded in an environment (SDK / Visual Studio) that doesn't support the newer version of the compiler. That effectively means that we need to support our roslyn3.11 source generators until the minimum version of Visual Studio that supports the generators is EOL. Based on the [official lifecycle document](https://learn.microsoft.com/en-us/lifecycle/products/visual-studio-2019), that is April 2029. Consequences based on that: - Packages are growing unboundedly whenever their underlying source generators target a new version of roslyn. - Old / legacy source generator source code can't be removed. In the example of roslyn3.11 source generators, they aren't incremental and cause performance issues. - Custom infrastructure for polyfilling SDK features in older environments adds complexity to packages. That infrastructure is repository specific (their is no shared code today at all) and written in msbuild. A package consumer needs to pay for evaluating that custom infrastructure even on a current environment. - Whenever a new source generator is added, we need to add new projects and grow the build graph as msbuild doesn't allow multi-targeting based on the compiler version. Those pain points could be mitigated by applying a [required toolset version](https://learn.microsoft.com/en-us/nuget/reference/nuspec#minclientversion) to our packages. While the toolset version refers to the minimum NuGet client version, the compiler and NuGet both ship in the SDK and Visual Studio and by picking a specific NuGet version, a compiler version is enforced as well. Alternatively we could discuss if we want to add another metadata to the nuspec file that indicates the required compiler version. Downside of that is that we would need to backport that support into those very old versions of the SDK and Visual Studio. By applying a minimum required toolset version, we could remove those old source generators, their underlying infrastructure and with that make our packages smaller and less complex. The downside in doing that is that we would prevent customers on older environments to reference the very latest versions of our packages. cc @ericstj @eerhardt @jaredpar @stephentoub @jkotas
Author: ViktorHofer
Assignees: ViktorHofer
Labels: `area-Meta`, `discussion`
Milestone: 9.0.0
ericstj commented 5 months ago

Refreshing this issue for some discussion in 9.0. For some scoping -- this issue is to discuss the source generators we ship in NuGet packages. The targeting-pack generators are already tied to a specific minimum roslyn version and have a servicing lifecycle -- those are out of scope for this issue.

This issue is to discuss the removal of source generators from the build and nuget packages for very old versions of Roslyn - which are no longer supported in the latest Visual Studio version, but are supported in older Visual Studio versions (2019).

Today we have to keep those working in the latest releases, and make them support new features that are added to the OOB packages. This is acutely a problem for System.Text.Json, Microsoft.Extensions.Logging - which still target Roslyn 3.11. It's less of a problem for other packages (Options and Configuration) since those first introduced the generator to target a much more recent version of Roslyn and can track it's support lifecycle in the latest VS. Whatever we decide for Json and Logging can also be applied to the other generators.

One suggestion above was to split the generators out into another package -- while that could serve to remove the generator from the current package it doesn't really solve the maintenance challenges of keeping the generators targeting that old version building, testing, and getting new features.

Whatever we do here is going to be perceived as a breaking change by some set (could be small) of customers who are trying to use the source generators on VS 2019 when targeting .NETFramework or .NETStandard. I think we need to accept and coument that as a breaking, and provide a workaround. They could download and manually reference the old generator if they wish - or check-in it's output.

jaredpar commented 5 months ago

I think removing support for older Roslyn versions in NuGet is reasonable. The impact is updating to a new version on a fairly old VS is a breaking change. There are existing scenarios where that happens. Many of our build task NuGet packages take dependencies on newer versions of MSBuild and hence won't work in older versions of VS.

The one caveat is security issue which forces us to fix a generators. Could end up in a position where we have to service an old NuGet vs asking customers to upgrade to latest. Becuase this proposal is creating a scenario where updating to latest can break tooling and likely won't be acceptable in a security fix.

eiriktsarpalis commented 5 months ago

What is the precise nature of the breaking change when referencing a Roslyn >= 4.0 source generator in VS 2019? (apologies, I don't have it installed on my machine). Is it making it impossible to add the NuGet dependency altogether or is it just a matter of not being able to view the generated source files in the IDE and getting relevant intellisense hints?

ViktorHofer commented 5 months ago

The same as outside of Visual Studio when you reference a source generator that requires a newer version of Roslyn than the version of the compiler being used:

The analyzer assembly 'C:\Program Files\dotnet\sdk\9.0.100-alpha.1.24068.28\Sdks\Microsoft.NET.Sdk\codestyle\cs\Microsoft.CodeAnalysis.CSharp.CodeStyle.dll' references version '4.10.0.0' of the compiler, which is newer than the currently running version '4.9.0.0'.

For packages like System.Text.Json which specify the minimum required roslyn version, the analyzer assembly wouldn't even get passed to the compiler. I.e. if you remove roslyn3.11 from System.Text.Json, VS2019 wouldn't receive the roslyn4.0 or roslyn4.4 source generators.

sharwell commented 5 months ago

@ericstj Do we have reason to believe https://github.com/dotnet/runtime/issues/83383#issuecomment-1559310920 is not going to be correct? I understand that the current state caused (past tense) additional work, but it's not clear to me that the current state would be likely to lead to burdensome future work.

ericstj commented 5 months ago

The one caveat is security issue which forces us to fix a generators. Could end up in a position where we have to service an old NuGet vs asking customers to upgrade to latest. Becuase this proposal is creating a scenario where updating to latest can break tooling and likely won't be acceptable in a security fix.

Which is why we would want to proactively message to folks that it's no longer going to be supported in the future with enough time to get them to move, before the version (in 8.0 package, for example) goes out of support. Such a path requires us to follow some deprecation messaging.

@ericstj Do we have reason to believe https://github.com/dotnet/runtime/issues/83383#issuecomment-1559310920 is not going to be correct?

@sharwell I don't think we grow indefinitely. Probably just a finite set. We just want to determine what rules we follow for adding new targets, and removing the ones we have.