'ILLinkTreatWarningsAsErrors' doesn't seem to work when publishing as MSIX

[Sergio0694]

Description

Perhaps related to #109154. I noticed that when creating an MSIX package, which uses MSBuild -t:build and runs the publish targets internally, setting ILLinkTreatWarningsAsErrors doesn't seem to work. I get a bunch of ILC warnings, yet the build completes successfully just fine.

Reproduction Steps

Same as #109270. Can't share repro steps here (since the code is not open source).
I can however share:

• binlog (MSFT only): link
• repo link and instructions to build locally (ping me on Teams)

Expected behavior

Build should fail with all ILXXXX warnings as errors.

Actual behavior

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[MichalStrehovsky]

This is currently by design. Per the docs: "Some of the options mention ILLink, which is the name of the underlying tool that implements trimming." This is one place where the underlying tool that performs IL level trimming seems to have leaked out. There is no IL level trimming during an AOT compilation and ILLink never executes in that scenario. This warning is generated by ILC, which is not ILLink and doesn't respect ILLink specific settings.

The question is whether we need ILC to respect this. I don't know what prompted this ILLink specific property to be documented. It's not the kind of property we'd generally have in .NET (e.g. there's no CSCTreatWarningsAsErrors to only treat C# compiler warnings as errors). If we do teach ILC to respect this, should this also apply to AOT and single file warnings generated by ILC? If not, do we need new properties to have parity? How about the analyzers? Should single file analyzer also respect this? Etc.

In .NET we do have a mechanism that works universally and that's NoWarn, WarningsAsErrors, WarningsNotAsErrors, and TreatWarningsAsErrors, as spelled out in the trimming docs next to ILLinkTreatWarningsAsErrors. If it was up to me, I'd deprecate ILLinkTreatWarningsAsErrors and keep it for backcompat only (and drop it from docs).

@dotnet/illink for opinions.

[Sergio0694]

I suppose my concern is: especially in large codebases with lots of history, it's very hard or impractical to enforce all warnings as errors (I'd love to enable that, but it'll take us some time to get there). However, for trim/AOT warnings, I definitely would like to make them all errors. But maintaining a list of all the 50+ warnings manually seems like not a great solution. I'd like to have just a single setting I could turn on, that'd make all ILXXXX warnings be a build error, regardless of where they're coming from (analyzers, ILC, doesn't matter).

Edit: I think we've had a similar conversation with @sbomer in the past and I think I remember him suggesting using this property for this purpose (or it might be a different one and I'm mixing them up now). But I can't find that thread again at the moment 😅

[MichalStrehovsky]

I suppose my concern is: especially in large codebases with lots of history, it's very hard or impractical to enforce all warnings as errors (I'd love to enable that, but it'll take us some time to get there)

The way I've seen it done in large codebases is that one enables warnings as errors and adds NoWarn/WarningsNotAsErrors for everything that is already triggering. This draws a line that says: "any new kinds of warnings need to be looked at". Any new additions to the NoWarn will be reviewed from then on. This guarantees the codebase doesn't get any worse, unless code review says it's fine. Then one can chip off warning by warning at a leisure time, fixing them and dropping them from NoWarn.

Is that not feasible?

[sbomer]

dotnet/sdk#12601 has some context on the original decision making. I think part of what motivated it is the distinction between TreatWarningsAsErrors (which is documented as specific to CSC, even though that's not true) and MSBuildTreatWarningsAsErrors.

The question is whether we need ILC to respect this.

ILC should not respect this - it's specifically for ILLink by design. The equivalent option for ILC is ILCTreatWarningsAsErrors.

I suppose my concern is: especially in large codebases with lots of history, it's very hard or impractical to enforce all warnings as errors (I'd love to enable that, but it'll take us some time to get there). However, for trim/AOT warnings, I definitely would like to make them all errors.

If I remember correctly, at the time we were also interested in the opposite scenario - treating compiler warnings as errors, but leaving ILLink warnings as warnings - since folks were trying out trimming with the less aggressive defaults and hoping for the best (but might have set TreatWarningsAsErrors in the project). But now that we have Roslyn analyzers too, it's less helpful to bifurcate the warnings settings by tool.

But maintaining a list of all the 50+ warnings manually seems like not a great solution. I'd like to have just a single setting I could turn on, that'd make all ILXXXX warnings be a build error, regardless of where they're coming from (analyzers, ILC, doesn't matter).

You might be looking for SuppressTrimAnalysisWarnings and SuppressAotAnalysisWarnings, which do pretty much that (they don't suppress all ILXXXX warnings, just those in the trim or AOT categories, respectively).

If it was up to me, I'd deprecate ILLinkTreatWarningsAsErrors and keep it for backcompat only (and drop it from docs).

I have no objection to this - its only purpose today is an escape hatch in case somebody wants different settings for the compiler and ILLink, but there are better ways to accomplish what users likely want (TreatWarningsAsErrors, NoWarn, or SuppressTrimAnalysisWarnings).

[Sergio0694]

Thank you both for the additional context! 🙂

"The way I've seen it done in large codebases is that one enables warnings as errors and adds NoWarn/WarningsNotAsErrors for everything that is already triggering. This draws a line that says: "any new kinds of warnings need to be looked at". Any new additions to the NoWarn will be reviewed from then on. This guarantees the codebase doesn't get any worse, unless code review says it's fine. Then one can chip off warning by warning at a leisure time, fixing them and dropping them from NoWarn."

Yeah that seems reasonable, and like you said that'll also make all ILXXXX warnings be errors, which is what I'm looking for.
I can just do that and explicitly opt out the few warnings we currently have, and we can resolve them later.

Thank you again!