ref(gen): replace runtime-evaluated BuildProperty-Generator with compile-time-evaluated AppTrimming/NativeAOT-Generator

[Flash0ver]

Description

Summary

Replace the current BuildPropertySourceGenerator, which emits a Lookup of a subset of MSBuild-Properties to the consuming compilation that are set via a [ModuleInitializer] from the consuming application which we then evaluate during runtime at Initialization of the SentrySdk,
with a Source-Generator that evaluates the Sentry.Internal.AotHelper-related CompilerVisibleProperty at compile-time and sets the effective boolean to use, depending on the TFM of the published application.

Remarks

A thought invoked by @aritchie via #4305 (comment).

We currently emit, via the BuildPropertySourceGenerator, the values of a subset of MSBuild-Properties used during build. But these MSBuild-Properties are without context, and we then need the Sentry.Internal.AotHelper to evaluate these Build-Properties at runtime to detect either "Publishing & Self-Contained & Trimmed" or "Publishing & NativeAOT".

Originally, the generated code was constrained to #if NET8_0_OR_GREATER.
In #4333 we changed this to #if NET5_0_OR_GREATER, because [ModuleInitializer] is available since net5.0, and to also broaden the support of #4330.

However, although App-Trimming was introduced in .NET 5, Native AOT was introduced in .NET 7 for Console Applications and in .NET 8 for ASP.NET Core.
This may now lead to a problem - admittedly a corner case, but a potential problem all the same - where when an (out-of-support) .NET 5 Application is build with <PublishAot>true</PublishAot>. Well, Native-AOT applications do require App-Trimming as well, so perhaps this isn't a problem in reality ... but it still lead me to this this thought:

• _IsPublishing && PublishSelfContained && PublishTrimmed should be evaluated for NET5_0_OR_GREATER (of the consuming application, not the Sentry library)
• _IsPublishing && PublishAot should be evaluated for NET7_0_OR_GREATER (of the consuming app, not the Sentry lib)

And other Source-Generator-depending features like #3212 could/should emit their own code to the consuming assembly, to keep these features independent of each other (single-responsibility style) to have more control over each feature, and also to be able to enable/disable Source-Generator-Features individually ("SentryDisableSourceGenerator" assumes that there is only the BuildPropertySourceGenerator, and/or is a flag to opt-out of every future Source-Generator as well).

So we may end up conflating concerns by emitting context-less values of build-time evaluated properties, and evaluating them at run-time for different features of the Sentry .NET SDK.

Example

// Sentry library
namespace Sentry.CompilerServices;
public static class BuildHelpers
{
  internal static bool? IsTrimmed { get; }
  internal static bool? IsAot { get; }
  public static void SetTrimmed(bool isTrimmed) => IsTrimmed ??= isTrimmed;
  public static void SetAot(bool isAot) => IsAot ??= isAot;
}

// added to consuming application via Sentry.SourceGenerators
namespace Sentry.Generated;
internal static class BuildInitializer
{
  [global::System.Runtime.CompilerServices.ModuleInitializerAttribute]
  internal static void Initialize()
  {
#if NET5_0_OR_GREATER
    // _IsPublishing = ..
    // PublishSelfContained = ..
    // PublishTrimmed = ..
    global::Sentry.CompilerServices.BuildHelpers.SetTrimmed(..);
#endif
#if NET7_0_OR_GREATER
    // _IsPublishing = ..
    // PublishAot = ..
    global::Sentry.CompilerServices.BuildHelpers.SetAot(..);
#endif
  }
}

Performance

As a side-effect, if we evaluate the Build-Properties at compile-time,
rather than allocating and statically rooting a new Dictionary, that is looked up at run-time and during Initialization only,
we could reduce / eliminate the heap-allocation of the [ModuleInitializer] and speed-up the evaluation slightly.

Alternative

Change the existing Source-Generator to no longer emit CompilerVisibleProperty unconditionally,
but emit CompilerVisibleProperty explicitly for the TFM version that they got introduced in,
in order to avoid emitting newer Build-Properties to Applications with older Build/Compiler/Runtime-Version where the newer Build-Property is not yet supported.

// [Generator]
..
if (property.Equals("build_property.PublishTrimmed", global::System.StringComparison.OrdinalIgnoreCase))
{
  source.AppendLine("#if NET5_0_OR_GREATER")
  source.AppendLine(/*add value of "PublishTrimmed" to BuildProperty-Lookup*/)
  source.AppendLine("#endif")
}
if (property.Equals("build_property.PublishAot", global::System.StringComparison.OrdinalIgnoreCase))
{
  source.AppendLine("#if NET7_0_OR_GREATER")
  source.AppendLine(/*add value of "PublishAot" to BuildProperty-Lookup*/)
  source.AppendLine("#endif")
}
..

Consideration

To be fair, as long as there is only a single Feature that is depending on / enhanced by the Source-Generator, this may be a neglectable issue or perhaps even a non-issue with minimal runtime-performance impact. But should we have more Features in the future that depend on that Source-Generator, we could end-up with some problems caused by the "contextless-ness" of the Build-Properties generated.

[jamescrosswell]

Given the frameworks that sentry targets:

sentry-dotnet/src/Sentry/Sentry.csproj

Lines 10 to 15 in b5d9cc2

	<PropertyGroup>
	<TargetFrameworks>net9.0;net8.0;netstandard2.1;netstandard2.0;net462</TargetFrameworks>
	<TargetFrameworks Condition="'$(NO_ANDROID)' == ''">$(TargetFrameworks);net9.0-android35.0;net8.0-android34.0</TargetFrameworks>
	<TargetFrameworks Condition="'$(NO_IOS)' == '' And $([MSBuild]::IsOSPlatform('OSX'))">$(TargetFrameworks);net9.0-ios18.0;net8.0-ios17.0</TargetFrameworks>
	<TargetFrameworks Condition="'$(NO_MACCATALYST)' == '' And $([MSBuild]::IsOSPlatform('OSX'))">$(TargetFrameworks);net9.0-maccatalyst18.0;net8.0-maccatalyst17.0</TargetFrameworks>
	</PropertyGroup>

... if someone's application does target net5.0, it would presumably be using the Sentry NuGet compiled for netstandard2.1 right? So the current guard we have (if net5.0 or greater) should be fine. The only scenario where that resolves to true is when someone is targeting net8 or greater at the moment.

None of our AOT stuff works for the legacy targets (netstandard and netfx)... so it should definitely be disabled in those targets in our NuGet.

[Flash0ver]

The Source Generator runs depending on the .NET SDK, and independent of the TargetFramework of the consuming project.

E.g.
When an application targets net5.0, then NuGet indeed select the compatible /lib/netstandard2.1/Sentry.dll.
However, the Analyzers from /analyzers/dotnet/cs/ run when the consuming project is build with .NET SDK 6.0.400 (or greater) since the Source-Generator depends on Microsoft.CodeAnalysis.CSharp v4.3.0.

Well, this may be quite a corner case, or perhaps even a non-issue, and may "just" be a (startup) performance improvement for the consuming applications.

[Flash0ver]

Discussion

• it's not a problem at runtime, as we additionally #if guard Native-AOT related code in Sentry
• it's only has a negligible startup performance impact
• we only would do this alongside significantly refactoring the Source-Generator
  • e.g. when changing from ISourceGenerator to IIncrementalGenerator

[jamescrosswell]

I'd argue not worth our time. Edge case for legacy products with no functional impact and a negligible impact on startup (not on hot paths)… we've got way more important stuff to do.

@Flash0ver shall we close this?