dotnet
Fix RunClassConstructor trim warning for Nullable<T>
@sbomer would it be possible to port this fix to .NET 9 as well? The XAML compiler relies on the analyzer and ILC not emitting a warning here to make sure WinUI 3 apps will not produce trim warnings. Right now this will break everyone using WarnAsErrors and trying to build a WinUI 3 app with NativeAOT, and the workaround of suppressing the warning is not viable either as it will also hide real warnings from user code 😅
Pardon my ignorance -- does this cover ILC as well? I don't see anything specifically in the ILC sources changing.
Yes, the fix is in the shared intrinsics handling code used by ILC, ILLink, and the roslyn analyzer. The testcase is shared as well.
@Sergio0694 before considering backport I think we need to know two things:
- Why was this discovered now vs. sooner?
- Are we confident this is the only thing that will be blocking?
I'm confident that we will not get a blanket approval for servicing changes of this kind into .NET 9, so we need to make sure we have everything we need.
"Why was this discovered now vs. sooner?"
We updated the XAML compielr with a fix for this just recently, it went out with WASDK 1.6-preview1 a couple weeks ago, so we didn't have too much time to test things on a broad range of projects. On top of this we don't currently have warnings as errors in the various internal pipelines with test projects and other repos using preview WASDK builds, so those new warnings probably just slipped by. I proposed to special case all trim/AOT warnings to actually fail builds, in the future (in WASDK repos), if just enabling all warnings as errors wasn't possible in the short term. On a related note: is there an easy way to achieve this, other than manually copying all 40+ ILxxxx warnings from the docs and special casing them somewhere in a .props/.targets?
"Are we confident this is the only thing that will be blocking?"
I wouldn't say we're 100% confident. Those were the only trim warnings that showed up in my local testing the other day, but I can't definitely conclude nothing else could potentially come up elsewhere. If possible, could we perhaps hold off on that PR backporting the fix to .NET 9, until let's say a few weeks after the public WASDK 1.6 release, so we'd have time to also get more feedback from users and spot any other related issues? Then at that point we could either complete the PR into .NET 9 GA, or if it's too late we could also just do that into the first servicing update for .NET 9, since this is technically not a blocker 🙂
Would that work?
I proposed to special case all trim/AOT warnings to actually fail builds, in the future (in WASDK repos), if just enabling all warnings as errors wasn't possible in the short term. On a related note: is there an easy way to achieve this, other than manually copying all 40+ ILxxxx warnings from the docs and special casing them somewhere in a .props/.targets?
I think ILLinkTreatWarningsAsErrors and IlcTreatWarningsAsErrors will do what you are looking for.
I wouldn't say we're 100% confident. Those were the only trim warnings that showed up in my local testing the other day, but I can't definitely conclude nothing else could potentially come up elsewhere. If possible, could we perhaps hold off on that PR backporting the fix to .NET 9, until let's say a few weeks after the public WASDK 1.6 release, so we'd have time to also get more feedback from users and spot any other related issues? Then at that point we could either complete the PR into .NET 9 GA, or if it's too late we could also just do that into the first servicing update for .NET 9, since this is technically not a blocker
The bar goes up even higher for GA, but that doesn't mean we can't take anything if the problem is severe enough.
How about this: you spend a little time finishing up local testing and validation and see if you find anything else? If you don't see anything else, we take this bug next week, and we can take anything else we find later.
@sbomer would it be possible to port this fix to .NET 9 as well? The XAML compiler relies on the analyzer and ILC not emitting a warning here to make sure WinUI 3 apps will not produce trim warnings. Right now this will break everyone using
WarnAsErrorsand trying to build a WinUI 3 app with NativeAOT, and the workaround of suppressing the warning is not viable either as it will also hide real warnings from user code 😅
Would it be possible to fix the XAML compiler not to emit RunClassConstructor call on Nullable types? I assume this comes from the RunClassConstructor that is supposed to register dependency properties. Since nullable will never have dependency properties (or even a class constructor), calling RunClassConstructor is just an expensive nop. This is also a reason why the analysis for this wasn't implemented - I certainly didn't expect people would want expensive nops to be trim safe.
"you spend a little time finishing up local testing and validation and see if you find anything else?"
@agocke sounds good! We looked into this some more and also tested things in a bigger app (the WinUI 3 Gallery App), and we can confirm that those two nullable warnings are the only one that show up. I think this fix alone should be fine then. We can look into filling in the .NET tactics template for servicing to .NET 8 (Michal mentioned we'll likely need that as well).
"Would it be possible to fix the XAML compiler not to emit
RunClassConstructorcall on Nullable types?"
Mentioned on Teams but also sharing here for context as well. We can do that and I do plan on investigating it, but the issue is it might take a while (ie. months) to roll out that fix, and we wanted to say that WinAppSDK 1.6 is officially supported with NativeAOT on release, and we can't technically say that if publishing produces trim warnings (even if harmless here). So that's why we wanted to ideally get this fixed so folks upgrading to 1.6 and .NET 8 could get a "clean" experience with AOT.
We can look into filling in the .NET tactics template for servicing to .NET 8 (Michal mentioned we'll likely need that as well).
(I hope you meant .NET 9. Backport to 8 would be a very different conversation since this PR has prerequisites.)
I think I might've been a tiny bit too optimistic there 😅
.NET 9 would also be nice to have at least. Do we need .NET tactics for that too, or is that easier?
.NET 9 would also be nice to have at least. Do we need .NET tactics for that too, or is that easier?
Looks like that backport just merged in #107039
Yup, .NET 9 change was approved. I think we should still get an answer to this:
Would it be possible to fix the XAML compiler not to emit RunClassConstructor call on Nullable types?
Ah, perfect, thank you! As for that fix, yeah absolutely! Like I mentioned, I do want to look into this for a future release. My current plan is to make it at least exclude all types from corelib + all types with no static constructor. We might come up with better ways to further refine the logic there, but that would seem a good way to do a first trim down pass, and it'd solve this issue.
@sbomer mmh I've set both properties but it seems they're all still just warnings 🤔
Ah, those properties will only affect the warnings from ILC an ILLink, not the analyzer. @agocke do you know if there is a way to treat warnings as errors in an analyzer, based on the warning category? If not, maybe we should add an option specifically for the trim warnings.
You can do this via editorconfig: dotnet_analyzer_diagnostic.category-<category>.severity.