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

[.NET9 + WASM] LinkerConfig.xml no longer respected?!? #110181

Closed
DierkDroth opened this issue Nov 26, 2024 · 16 comments
Closed

[.NET9 + WASM] LinkerConfig.xml no longer respected?!? #110181

DierkDroth opened this issue Nov 26, 2024 · 16 comments
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers

Comments

@DierkDroth
Copy link

DierkDroth commented Nov 26, 2024

Description

Originally I posted this issue on the Platform UNO GitHub. However, I learnt from @jeromelaban (CTO, Platform UNO) that this is a .NET runtime issue which is why I'm posting this here. Please relocate this issue as your see need and fit ... thanks.

After upgrading .NET8 -> .NET9 I get tons of linker warnings on my code although all my assemblies are (still) excluded in LinkerConfig.xml.

Please see repo below:

  • there is a single code line which causes a warning
  • however, the custom assembly is excluded in LinkerConfig.xml
    Note: there are a couple of warnings resulting from the UNO assemblies. Please ignore those for now, but focus on the single warning resulting from my custom repo code.
    Minimal.9.zip

Here is my question:
Why am getting the warning as my custom assembly is excluded in LinkerConfig.xml? How could I now be sure that the exclusion works as expected? This makes no sense to me. If I wanted to know what warnings my custom assembly had, then I would just remove the exclusion, no? The current situation just causes confusion to the user and should be rectified.

< rant on >
Also: why do I get these warnings at all? Why can't the WASM linker "just shut up and do its job" similar to the linker technology with the native C# stack? As a user I don't want to be bothered with that stuff. If it's safe to trim off code then do so, if not then leave it. There still could be an option for the experienced/interested user like 'ok mister super-duper-user, here you could trim your binary even more if you did this' ... but as an ordinary user I just "don't give a sh...". I strongly recommend providing the same level of convenience with the WASM linker as with the linker in the native C# stack.
< rant off >

What am I missing here?

Reproduction Steps

please see above

Expected behavior

please see above

Actual behavior

please see above

Regression?

I think so

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 26, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 26, 2024
@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Nov 26, 2024

After upgrading .NET8 -> .NET9 I get tons of linker warnings on my code although all my assemblies are (still) excluded in LinkerConfig.xml.

I'm not aware of any changes around warning suppressions between 8 and 9. Blazor WASM defaults to setting the documented SuppressTrimAnalysisWarnings property to disable all warnings even if the app could be broken. I don't know what UNO does, I know they have their own build integration.

Why am getting the warning as my custom assembly is excluded in LinkerConfig.xml

The warnings that are generated have no relationship with the LinkerConfig.xml file. Consider:

class Program
{
    static int Main()
    {
        return Activator.CreateInstance(Type.GetType(Console.ReadLine())).GetHashCode();
    }
}

Above is going to generate a warning because analysis doesn't know what type this is going to ask about, since we literally Console.ReadLine it from user. The app will likely be broken at runtime. That's all the warning is saying. Maybe you only type System.Object, then the app would work.

We cannot "If it's safe to trim off code then do so, if not then leave it.". You as a user requested trimming. The tool says it will probably produce something broken because we don't know what type name the user types. Maybe you specified all the types the user can type in your LinkConfig.xml. Maybe you didn't. The tool has no clue, there's no way to connect that XML to here. We could in lieu of generating the warning just not trim anything at all, but then people would file bugs saying they requested trimming and nothing happened (the tool literally doesn't know what to keep so it just must keep everything).

If you don't want trimming you can just leave it off. If you don't mind the app is probably broken, you can suppress the warnings with the documented switch.

@MichalStrehovsky MichalStrehovsky added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 26, 2024
Copy link
Contributor

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

@DierkDroth
Copy link
Author

@MichalStrehovsky thanks for getting back to me.

Let me try to sort the feedback you provided:

  • I do know that Platform UNO changed the logic of building the app when switching from .NET8 -> .NET9. I learnt that with .NET9 now the MS WASM toolchain is used. I don't know how exactly it worked before with .NET8 and Platform UNO. May be @jeromelaban could clarify?!?
  • still I don't understand why it makes sense to provide warnings for assemblies which I explicitly excluded. Why would that make any sense? It rather confuses me since I don't know if the exclusion really is working or not. How would I know if the warning is legitimate and the exclusion was not effective for some reason = I needed to do 'something about it' ... or if this warning could be ignored safely, since the exclusion worked as expected? What am I missing here?
  • still my claim stands: why would I need to deal with trimming warnings in the first place? if it's safe to trim, then the linker should trim, if there are warnings, then the 'suspicious' code should not be trimmed - but anything else which does not have warnings still could be trimmed.
    I'm not a compiler/linker expert, but I would think that the native C# stack has a similar logic. Why not just follow along?
    Also: worst case there could be different trimming levels: one which trims the code which is safe to trim and a more 'aggressive' level which even trims off the code with warnings.

@MichalStrehovsky
Copy link
Member

warnings for assemblies which I explicitly excluded

Can you be more specific about what you mean by this?

if it's safe to trim, then the linker should trim, if there are warnings, then the 'suspicious' code should not be trimmed

I'm not sure i understand. Let's take the example I posted above. The suspicious code is Type.GetType(Console.ReadLine()). The suspicious code is not getting trimmed at all. We keep the entire method intact, nothing is touched. What does get trimmed is everything else. And that's the problem the warning is telling you. If someone types System.Net.Http.WebClient to the ReadLine call, we probably trimmed it and the type won't be found, returning null.

The only way to implement your proposal would be instead of emitting a warning, to keep everything. But I don't see much point in that - this is something you as a user can do too - you see a warning, decide you don't want to address it and just turn off trimming and problem solved.

@DierkDroth
Copy link
Author

DierkDroth commented Nov 26, 2024

@MichalStrehovsky thanks again for your feedback.

We keep the entire method intact, nothing is touched.

Thanks for clarification. This was not clear to me so far. I now understand that code which throws warnings in fact is not trimmed at all.

Can you be more specific about what you mean by this?

Sure, if you unzipped the repo you will find these lines in the file Minimal.9/Minimal.Wasm/LinkerConfig.xml

<linker>
  <assembly fullname="Minimal.Wasm" />

... which means I'm excluding the assembly Minimal.Wasm from the linker pass, correct?

However, when you run Minimal.9/Minimal.Wasm/Make.bat you will find this warning:
C:\tmp\Minimal.9\Minimal.9\Minimal.Shared\MainPage.xaml.cs(35,4): Trim analysis warning IL2072: Minimal.MainPage.MainPage(): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Activator.CreateInstance(Type)'. The return value of method 'Minimal.MainPage.TestType.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

Why do I get that warning although the Minimal.Wasm assembly is excluded as per above?

@DierkDroth
Copy link
Author

We keep the entire method intact, nothing is touched.

@MichalStrehovsky to clarify: so I wouldn't even need LinkerConfig.xml at all. I just could remove it altogether and live with the gazillions of warnings I would get from my code (or suppress them) - however, the linker would do its thing and not touch the code which throws the warnings, but trim anything else. In any case the trimming is safe and would not corrupt the resulting binary.

Is my understanding correct?

@jeromelaban
Copy link
Contributor

jeromelaban commented Nov 26, 2024

I don't know what UNO does, I know they have their own build integration.

With net9, we're using the official tool chain now, and the original behavior applies, which includes linker validations which were enabled in net8 for blazor.

We have linker warnings that are showing up inside Uno assemblies which we are fixing for an upcoming release and @DierkDroth you can disable those warnings to get to the Uno net8 behavior. You only need to wait for the next uno update to be released.

As I mentioned earlier, I don't think what you're reporting a bug that explicitly excluded assemblies still provide warnings, mainly because linker warnings are transitive based on what methods are using.

@jtschuster
Copy link
Member

I now understand that code which throws warnings in fact is not trimmed at all.

To clarify here, the method where a warning originates will not be trimmed, but the warning is saying "there is a dependency here that I can't understand, and I might remove what the code needs". In this example, the trimmer sees a code path that calls Minimal.MainPage.MainPage(), which calls something like Activator.CreateInstance(Minimal.MainPage.TestType). The System.Type object in the property Minimal.MainPage.TestType could represent a type from any assembly. This type may have had its constructor removed, in which case Activator.CreateInstance would fail. Rather than keep every constructor on every type, it warns. Keeping everything that it might need defeats the purpose of trimming.

To reiterate, trimmer warnings aren't saying "You could trim more if you do this", it's saying "I can't analyze this statically, but I'm still going to remove stuff and your app might break".

Sure, if you unzipped the repo you will find these lines in the file Minimal.9/Minimal.Wasm/LinkerConfig.xml

<linker>
  <assembly fullname="Minimal.Wasm" />

... which means I'm excluding the assembly Minimal.Wasm from the linker pass, correct?

This means that the linker will not trim anything inside the assembly Minimal.Wasm. The trimmer will still analyze the entire assembly and warn if it sees any code that it can't guarantee will work.

I wouldn't even need LinkerConfig.xml at all. I just could remove it altogether and live with the gazillions of warnings I would get from my code (or suppress them).

You still need LinkerConfig.xml. Without it, the linker would aggressively trim everything that it doesn't see static dependencies for, and the app would be much more likely to fail. Trimming with a LinkerConfig.xml and suppressing warnings is telling the linker, "I've put everything in the LinkerConfig.xml that I'll need. Keep those assemblies/types/methods, and the app will work, even though you can't guarantee it."

Regarding the rant section, the WASM trimmer is the exact same tool as the native C# trimmer. The problem is that UI and Web frameworks in C# rely heavily on reflection which cannot be statically analyzed. The framework authors do their best to provide LinkerConfig.xml files to keep everything that should be needed, and suppress warnings when they are confident that they have manually kept the metadata needed, but it can be fragile.

@DierkDroth
Copy link
Author

DierkDroth commented Nov 26, 2024

@jtschuster thanks for providing your feedback.

The trimmer will still analyze the entire assembly and warn if it sees any code that it can't guarantee will work.

This is exactly what I'm 'complaining' about: there then just is no way to tell if the exclusion actually worked or not - since there always will be the warnings, correct?
IMHO there is no point in providing warnings if the assembly is excluded. It's only confusing to the user. If I wanted to see the warnings, then I would not add the assembly to the exclusions.

... it sees any code that it can't guarantee will work

Actually, wait a minute: are you saying there could be legit C# code (= works natively no problem) but might fail in WASM even without any trimming involved at all ... and that's what these warnings are for?!?

Without it, the linker would aggressively trim everything that it doesn't see static dependencies for, and the app would be much more likely to fail.

Got it. Thanks, I understand that reasoning...

@DierkDroth
Copy link
Author

@jtschuster
Also: I just looked up the IL2072 warning which is - as you pointed out - a linker warning which is available in the native C# setup as well. This enforces my challenge: why do I get the warning in the WASM environment, but not with the 'regular' C# linker?

@jtschuster
Copy link
Member

there then just is no way to tell if the exclusion actually worked or not - since there always will be the warnings, correct?
IMHO there is no point in providing warnings if the assembly is excluded. It's only confusing to the user. If I wanted to see the warnings, then I would not add the assembly to the exclusions.

The warnings can be fixed (see here) or suppressed by the author of the assembly with the UnconditionalSuppressMessageAttribute (see here), or you can set the PublishTrimmed property to false in your Wasm csproj: <PublishTrimmed>false</PublishTrimmed>.

Exclusion isn't the right way to think about this; "fully preserved" is more accurate. The assembly in LinkerConfig.xml preserved, but other assemblies in the app will be trimmed. The code causing the warning can reflect over arbitrary types in the app from any assembly, so the preserving the current assembly doesn't guarantee that the metadata it requires will not be trimmed.

are you saying there could be legit C# code (= works natively no problem) but might fail in WASM even without any trimming involved at all ... and that's what these warnings are for?!?

No, any warnings from the linker are only related to trimming. If you don't use the linker, you should have identical behaviors (or at least any warnings would come from the compiler instead).

the IL2072 warning which is - as you pointed out - a linker warning which is available in the native C# setup as well. This enforces my challenge: why do I get the warning in the WASM environment, but not with the 'regular' C# linker?

It looks like the default Windows build doesn't run the linker at all.

@DierkDroth
Copy link
Author

DierkDroth commented Nov 26, 2024

Thanks again @jtschuster. The confusion starts to clear up...

The code causing the warning can reflect over arbitrary types in the app from any assembly, so the preserving the current assembly doesn't guarantee that the metadata it requires will not be trimmed.

This actually concerns my again since - if I understand you correctly - preserving the assembly in LinkerConfig.xml is great and fine and does what it should do. However, the warned code still could fail since relevant metadata might have been deleting by trimming other, non-preserved assemblies? Is that what you're saying here?

Now, if that is true then the question is: what needed to be done to resolve the warnings? I admit I haven't looked into yet at all - but rather am digesting it from a conceptual aspect at this point... since this implies that 'ignoring' or 'masking away' these warnings bears the risk of unknown failure at runtime even when excluding/preserving the hosting assembly in LinkerConfig.xml, correct?

@jtschuster
Copy link
Member

if I understand you correctly - preserving the assembly in LinkerConfig.xml is great and fine and does what it should do. However, the warned code still could fail since relevant metadata might have been deleting by trimming other, non-preserved assemblies? Is that what you're saying here?

'ignoring' or 'masking away' these warnings bears the risk of unknown failure at runtime even when excluding/preserving the hosting assembly in LinkerConfig.xml, correct?

Yes, that's accurate.

what needed to be done to resolve the warnings?

For this particular warning, you need to add a DynamicallyAccessedMembersAttribute to the TestType property with the same (or more) DynamicallyAccessedMemberTypes as Activator.CreateInstance, which requires DynamicaalyAccessedMemberTypes.PublicParameterlessConstructor. It should look like this:

using System.Diagnostics.CodeAnalysis;
public sealed partial class MainPage : Page
{
	public MainPage()
	{
		Activator.CreateInstance(TestType);

		Content = new TextBlock { Text = "Hi there" };
	}

	[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
	public Type TestType
		=> typeof(string);
}

The DynamicallyAccessedMembersAttribute tells the trimmer that the members of any time assigned to this value must be preserved. The fixing warning documentation linked can provide more of an explanation of what's going on, but let me know if something there isn't clear.

@jtschuster
Copy link
Member

Closing the issue as expected behavior, but feel free to comment if you have anymore questions.

@jtschuster jtschuster closed this as not planned Won't fix, can't repro, duplicate, stale Nov 26, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Nov 26, 2024
@DierkDroth
Copy link
Author

Thanks @jtschuster for walking me through the issue. I will review my code for these (and other) warnings and will come back to in case...

@DierkDroth
Copy link
Author

DierkDroth commented Nov 27, 2024

@jtschuster some quick feedback after I invested a few hours ripping through the 100s of warnings (from preserved/excluded assemblies) in my code. This feels like the old days of FxCop where after spending days and weeks on working through warnings you wonder if you actually had fixed any true issue at all...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
Status: No status
Development

No branches or pull requests

4 participants