fix: Stop warnings from showing in Blazor WASM projects

[alexsohn1126]

fixes: #3369

Problem

Warning appears when publishing Blazor WASM apps with AOT enabled

I thought I could pass in additional properties when referencing Sentry from Blazor WASM sdk like this:

    <ProjectReference Include="..\Sentry\Sentry.csproj">
      <AdditionalProperties>UsingSentryBlazorWebAssemblySdk=true</AdditionalProperties>
    </ProjectReference>

Then we can set a compile time processor directive referencing enabled when UsingSentryBlazorWebAssemblySdk is present, but I kept getting the warnings regardless of the things I've tried.

So I just suppressed the warning like mentioned in #3369.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.33%. Comparing base (4581417) to head (78ea186).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Sentry/Platforms/Native/CFunctions.cs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4519      +/-   ##
==========================================
- Coverage   73.36%   73.33%   -0.04%     
==========================================
  Files         479      479              
  Lines       17505    17509       +4     
  Branches     3445     3445              
==========================================
- Hits        12843    12840       -3     
- Misses       3782     3789       +7     
  Partials      880      880              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jamescrosswell]

I'm a little bit nervous about disabling this globally. In our case (if I've understood) the code in question never gets called at runtime, so we can safely ignore it... but there are going to be scenarios where that warning would alert SDK users to genuine concerns.

I wonder if we could be a bit more specific, so that we only disable the warning for our package/code?

I'm not sure if these would work but we might be able to do:

[UnsupportedOSPlatform("browser")]
internal static void InitNativeStuff()
{
    if (!OperatingSystem.IsBrowser())
    {
        // native/PInvoke etc.
    }
}

Or alternatively:

#pragma warning disable WASM0001
internal static void InitNativeStuff() { /* ... */ }
#pragma warning restore WASM0001

Providing we can identify the specific code that is triggering the warnings...

[Flash0ver]

I have a similar concern: masking the issue too broadly.

I'm wondering if this is a more general concern, and not just Blazor WASM, when it comes to NativeAOT and DllImportAttribute.
Perhaps this could be fixed by using the net7.0 Source-Generators via LibraryImport? But then I'm wondering why we haven't hit the problem with other app-models yet 🤔 ... also this would be a bigger issue with way more testing involved.

[alexsohn1126]

I think the code that is causing the warnings are all in here:

sentry-dotnet/src/Sentry/Platforms/Native/CFunctions.cs

Lines 11 to 20 in 7061fb9

	internal static class C
	{
	internal static void SetValueIfNotNull(sentry_value_t obj, string key, string? value)
	{
	if (value is not null)
	{
	_ = sentry_value_set_by_key(obj, key, sentry_value_new_string(value));
	}
	}

I can try both of them and see what works. I don't think UnsupportedOSPlatform("browser") works if they don't have the TFM set to net9.0-browser according to this docs. Blazor WASM applications don't have to use the browser TFM.

So I'm leaning more towards the second option. I'll try that and report back.

[Flash0ver]

now I'm not sure if I'm jumping the gun with the source-generated P/Invoke ... since the scope of this is the entire SDK, I suppose this would be another issue

on second though, I'm now leaning towards a non-invasive quick fix for this issue, that we then follow-up in an overarching fashion with source-generated P/Invoke

[alexsohn1126]

Weird observation I made between .NET 8 vs. 9.

If .NET 8 is used to make the Blazor WebAssembly project, it shows these warnings:

Sentry.Samples.AspNetCore.Blazor.Wasm succeeded with 3 warning(s) (56.6s)
    /usr/local/share/dotnet/packs/Microsoft.NET.Runtime.WebAssembly.Sdk/8.0.19/Sdk/WasmApp.Native.targets(338,5): warning WASM0001: Could not get pinvoke, or callbacks for method 'Sentry.Native.C::sentry_transport_new' because 'Parsing function pointer types in signatures is not supported.'

[same error in different functions that use `delegate *` in their parameters in Cfunctions.cs]

However, if the Blazor WebAssembly project is in .NET 9:

Sentry.Samples.AspNetCore.Blazor.Wasm succeeded with 16 warning(s) (60.3s)
    /usr/local/share/dotnet/packs/Microsoft.NET.Runtime.WebAssembly.Sdk/9.0.8/Sdk/WasmApp.Common.targets(793,5): warning WASM0001: 
      Could not get pinvoke, or callbacks for method 'Sentry.Native.C::sentry_value_new_object' because 'System.NotSupportedException: Unsupported parameter
       type in method 'Sentry.Native.C.sentry_value_new_object'
         at PInvokeCollector.<CollectPInvokes>g__CollectPInvokesForMethod|6_0(MethodInfo method, <>c__DisplayClass6_0&)
         at PInvokeCollector.CollectPInvokes(List`1 pinvokes, List`1 callbacks, HashSet`1 signatures, Type type)'

[same errors for many of the functions that use sentry_value_t type in Cfunctions.cs]

First, it's weird that the warnings disappeared for function pointer type warnings in .NET 9, and new warnings appeared out of nowhere.

Why is this the case? I'll dig in deeper, perhaps we can figure out a better solution to this.

[alexsohn1126]

Had a chat with @Flash0ver about this issue, seems like there is more caveat to this than it first seems.

We have tried targeting net8.0-browser, and using BROWSER constant to block out some code from being compiled, but that did not work as long as the Blazor WebAssembly project's TargetFramework was net8.0. Even if the project's TargetFramework was net8.0-browser, it would cause build errors which we didn't have time to resolve. Also, it would be undesirable to tell the user "Hey, change the TargetFramework!", as they could be using their own target framework for whatever reason.

We also brainstormed the idea of possibly adding Roslyn analyzers to suppress these warnings conditionally. We haven't tried it, but it could be an option.

Using #pragma warning disable was not effective because the warnings in this issue occurs in Microsoft WASM sdk.

If there are any ideas, it would be great!

[jpnurmi]

What if the native bindings were moved into a separate Sentry.Bindings.Native package? Would it help if it was only referenced on relevant platforms?

[alexsohn1126]

What if the native bindings were moved into a separate Sentry.Bindings.Native package? Would it help if it was only referenced on relevant platforms?

Ooh, this sounds promising. I'll look into this more!

[jamescrosswell]

Using #pragma warning disable was not effective because the warnings in this issue occurs in analyzers.

Another option might be the SuppressMessageAttribute.

If worst comes to worst we can simply document this in the troubleshooting section of our docs and recommend users include the Sentry package like so, for WebAssembly apps:

<ItemGroup>
  <PackageReference Include="Sentry Version="1.0.0" NoWarn="WASM0001" />
</ItemGroup>

That way the warning would be suppressed for Sentry but not for any other packages.

[Flash0ver]

I do like the separation into a new Sentry.Bindings.Native, but we would also need dedicated net8.0-browser/net9.0-browser targets, so that we can conditionally include/exclude the dependency.

I also like the SuppressMessageAttribute idea ... that would be the simplest solution ... but SuppressMessageAttribute will not be part of the output compilation / assembly, because it has a ConditionalAttribute.
However, the UnconditionalSuppressMessageAttribute is preserved in the output compilation / assembly.

Cool ... I knew you can give Aliases to a PackageReference, but I didn't know you can also do an isolated NoWarn 💡

[alexsohn1126]

tldr; UnconditionalSuppressMessageAttribute and SuppressMessageAttribute do not work to suppress these WASM0001 warnings.

After trying out both UnconditionalSuppressMessageAttribute and SuppressMessageAttribute, I found out these warnings cannot be suppressed by neither of them.

WASM0001 warnings are triggered by the task ManagedToNativeGenerator:

Looking at the source code for ManagedToNativeGenerator (link), we can see that it scans .dll files.

Referring back to the .NET9.0 warnings, the warning is from CollectPInvokes function. In v8.0.16 of the file that contains the function, we can see where those .NET8.0 warnings originated from:

• https://github.com/dotnet/runtime/blob/v8.0.16/src/tasks/WasmAppBuilder/PInvokeCollector.cs#L212

All previous .NET8.0 versions do not have this warning, and all .NET9.0 versions do not have this warning, so that explains why we were seeing the different warnings between .NET8 and .NET9.

And the new errors we encounter are probably from these lines of code that was added:

• dotnet/runtime@ed9f475#diff-429d68fa67442212800d3b0c7ec9a048510188e463a890d2865fa04833e7a9c5R60-R66

All that is to say:

WASM0001 is from build tasks from scanning assemblies (yes, i was wrong on saying it was from analyzers 😮‍💨), thus we cannot use UnconditionalSuppressMessageAttribute nor SuppressMessageAttribute to suppress these warnings. These suppression attributes only work with AOT/trimmer warnings, not WASM sdk triggered by MSBuild task like in this case...

We can either ask the users to mute the warnings just for Sentry like @jamescrosswell mentioned, or we can go with the refactoring - making Sentry.Bindings.Native.

[alexsohn1126]

@Flash0ver

I do like the separation into a new Sentry.Bindings.Native, but we would also need dedicated net8.0-browser/net9.0-browser targets, so that we can conditionally include/exclude the dependency.

Can't we use the UsingMicrosoftNETSdkBlazorWebassembly property?

[kg]

First, it's weird that the warnings disappeared for function pointer type warnings in .NET 9, and new warnings appeared out of nowhere.

Why is this the case? I'll dig in deeper, perhaps we can figure out a better solution to this.

It's been a while so my memory isn't perfect, but essentially the NET8 version of this analyzer and the associated code generator had a handful of bugs. We tried to fix all the bugs we could find while also making the analyzer more robust and consistent. That's why there are more warnings now, most likely - that stuff might have been silently broken before. One of the things we did is try to conform more closely to the wasm C ABI.

From looking at your code one thing you could try is changing sentry_value_t so it's not a union - that may make the interop generator stuff be able to understand it correctly. Instead of using FieldOffset(0) you could use Unsafe.As, I think (preferably by wrapping it in helper property accessors).

[jamescrosswell]

From looking at your code one thing you could try is changing sentry_value_t so it's not a union - that may make the interop generator stuff be able to understand it correctly. Instead of using FieldOffset(0) you could use Unsafe.As, I think (preferably by wrapping it in helper property accessors).

This definitely sounds like a better solution, if we can make that work for us...

or we can go with the refactoring - making Sentry.Bindings.Native

I can't quite wrap my head around how that would work... where/when would the decision to include Sentry.Bindings.Native get made? We have to delay this until the SDK user is building their solution and we know whether or not the target is WASM right?

[Flash0ver]

Can't we use the UsingMicrosoftNETSdkBlazorWebassembly property?

Not sure how to solve this with an MSBuild property. In a NuGet package, we can have /lib/{TFM}/*.dll to target several versions, where each version can have varying References/Versions to other Assemblies/Packages. This is resolved at build/pack time. We can't dynamically change a Dependency of an already built Assembly in consuming projects ... unless we load Assemblies dynamically (via Reflection / AssemblyLoadContext) which I would like to avoid since this may be another can of worms regarding Trimming and AOT.

[alexsohn1126]

From looking at your code one thing you could try is changing sentry_value_t so it's not a union - that may make the interop generator stuff be able to understand it correctly. Instead of using FieldOffset(0) you could use Unsafe.As, I think (preferably by wrapping it in helper property accessors).

Thank you so much for the detailed insight! I will try changing sentry_value_t, and see if we can come up with a fix.

where/when would the decision to include Sentry.Bindings.Native get made? We have to delay this until the SDK user is building their solution and we know whether or not the target is WASM right?

I thought we could use the property that comes with the Microsoft Blazor WASM sdk.. Apparently not.

We can't dynamically change a Dependency of an already built Assembly in consuming projects ... unless we load Assemblies dynamically (via Reflection / AssemblyLoadContext) which I would like to avoid since this may be another can of worms regarding Trimming and AOT.

Yea, I suppose that means we can't use that property to stop this warning using that blazor wasm sdk property.. I thought maybe we could do something like this:

<ItemGroup Condition="'$(UsingMicrosoftBlazorSdk)' != 'true''">
  <ProjectReference Include="..\Sentry.Bindings.Native.csproj" />
</ItemGroup>

but if that's resolved at build time, then this would be useless.

[Flash0ver]

Yes, the ProjectReferences are resolved at build-time (also during restore and pack).

The Sentry package would have a dependency on Sentry.Bindings.Native, to not break existing scenarios.
So when Sentry.AspNetCore.Blazor.WebAssembly has a dependency on Sentry, it also transitively depends on Sentry.Bindings.Native.

Unless we can use the net8.0-browser and net9.0-browser TFMs, which - for Sentry - would not include the dependency to Sentry.Bindings.Native, and Sentry.AspNetCore.Blazor.WebAssembly would target these instead of net9.0;net8.0. But if I recall correctly, we gave that a brief try and it did not work as expected ... unless we have missed something in that quick test.

Another consideration could be a breaking change, so that we split into Sentry and Sentry.Bindings.Native, but Sentry does not depend on Sentry.Bindings.Native, so users would need to explicitly opt-in by also referencing the package Sentry.Bindings.Native. But I am a bit uncertain about the tradeoff regarding the impact.

Also, I am quite surprised that the [UnconditionalSuppressMessage] Attribute does not work for this case 🤔