Design and implement user experience around linker analysis warnings

[vitek-karas]

This mostly concerned with the warnings produced by the data flow analysis and correctness checks, but in theory it applies to all linker warnings.

Things to consider

• Define the set of trim analysis warnings - Define the trim analysis warning category #1367
• Analysis warnings - Off by default - easy to turn on - Introduce option to enable trim analysis warnings sdk#12388
• How do warning messages play together if there's more than one. This is also related to Improve IL2006 warning #1157.
• At first we expect apps/frameworks will get lot of linker analysis warnings, maybe we should sort them to get a predictable order.

[mateoatr]

Analysis warnings - Off by default - easy to turn on

For this we could make sure that all analysis warnings declare a subcategory and then check in LogMessage if the MessageContainer that is received contains any of the subcategories that we know to be specific to analysis warning.
If printing this analysis-specific warnings off by default, how should the user activate it? Through a command-line option?

At first we expect apps/frameworks will get lot of linker analysis warnings, maybe we should sort them to get a predictable order.

For this we'd need to cache all warnings and print them when the linker exits. I don't think this adds much overhead to the overall process, should we do it?

[vitek-karas]

For this we could make sure that all analysis warnings declare a subcategory and then check in LogMessage if the MessageContainer that is received contains any of the subcategories that we know to be specific to analysis warning.

We've discussed this to some degree already - I think the conclusion was to disable a set of warning codes on the MSBuild side. (I think it's the NoWarn property).

If printing this analysis-specific warnings off by default, how should the user activate it? Through a command-line option?

The off-by-default mentioned above via NoWarn would be conditional on some new property... LinkerAnalysisWarnings (needs better name). If that property is "on" then we would not disable the warnings.

So the end result is that linker always produces the warnings, but sometimes (most of the time) they're filtered out by MSBuild. This needs to be tried though - I'm not sure it works well end-to-end.

[sbomer]

I am a little wary of doing this in MSBuild. It seems unusual to introduce warnings that aren't ready to be on by default, but still emit them, then filter them out by default, and have an option on top of that to un-filter them. It would be more straightforward not to emit experimental warnings by default, and pass a flag to the linker that turns them on. Then there would be no MSBuild magic, and NoWarn (or MSBuildWarningsAsMessages until dotnet/msbuild#4421 is done) could be used as usual to selectively disable them.

Just wanted to express this concern - in this particular case adding it at the MSBuild layer might be the most expedient thing to do, just to avoid introducing a new command-line and task input. It just means there won't be a simple way to disable them when running illink from the command line or using the task directly. My hope is that we can avoid this pattern, and get rid of the MSBuild logic once we are confident that the warnings are "ready".

[marek-safar]

I'm all for clearly separating analyzer warnings by introducing a new category or even using a special number range for quick overview check.

If we go with category approach we could have something like -warn-category:all|default|analyzer|experimental|etc. as linker command line arguments. I agree with @sbomer that whatever filtering we do should be done inside linker.

I don't think we have to sort the warnings at the end as we would need to come up with sorting rules which will be imperfect anyway.

Issuing multiple warnings is something that will need to be tweaked manually to reduce cascading or overlapping warnings (csc has that problem as well) and it's not a generally serious concern.

[mateoatr]

I opened a PR with a proposal: #1303.

[agocke]

What would we say the status of this issue is?

[vitek-karas]

I think we're getting close - things we should do:

• review warning messages (not codes, those should remain as they are)
• validate E2E what does it look like to enable analysis warnings on an app
• ideally (but we probably won't be able too much about it) - think about limiting warnings per assembly or some set of assemblies - this is related to Make it easy to run trimming for diagnostic purposes on libraries sdk#12907

[vitek-karas]

I think this is basically done. Notable improvements from the last comment here:

• We have all warnings documented on public docs
• Warnings are on by default for console apps, off by default for other verticals like Blazor
• Warnings are unrelated to each other, and this is intentional.
• We have analyzer which produces the same warnings as linker (or very close)
• We have E2E validation in the SDK repo
• We implemented single-warn and IL2104 which reduces warnings from assemblies which are not part of the user's code base to just one warning per assembly