NativeAOT implement warning as errors as a compiler feature

[vitek-karas]

Both roslyn and trimmer implement "warn as error" as a compiler feature - specifically the TreatWarningsAsErrors property only affects tools which explicitly react to it. There's a related MSBuild command line option which will affect all warnings, but that's not the one VS sets from the UI and what most customers use. See the comments in #94178.

This implements the same behavior as the trimmer. It adds 3 new command line options to ilc and enable "warn as error" globally and to enable/disable specific warning codes. Note that illink implements a more complex command line parsing where order of parameters matters and sometimes the later ones can override the earlier ones. This was trying to emulate csc behavior. For ilc I don't see a reason to emulate that because running ilc on its own is not supported, only going through the SDK, which will never pass the new parameters multiple times.

For testing this enables the existing trimmer tests for this functionality. This uncovered a small issue in substitution parser, which is also fixed here. For simplicity the test infra emulates the illink command line behavior over ilc (which is easy as they're very similar).

Open question:
Trimmer introduces ILLinkTreatWarningsAsErrors property which defaults to TreatWarningsAsErrors but can be overriden and it specifically applies on to illink. I didn't introduce a new one for ilc as I don't see much value in doing so, but I may be missing something. @sbomer - do you think we should add IlcTreatWarningsAsErrors?

Update:
We agreed to add IlcTreatWarningsAsErrors.

[ghost]

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

Issue Details

---

Both roslyn and trimmer implement "warn as error" as a compiler feature - specifically the TreatWarningsAsErrors property only affects tools which explicitly react to it. There's a related MSBuild command line option which will affect all warnings, but that's not the one VS sets from the UI and what most customers use. See the comments in #94178.

This implements the same behavior as the trimmer. It adds 3 new command line options to ilc and enable "warn as error" globally and to enable/disable specific warning codes. Note that illink implements a more complex command line parsing where order of parameters matters and sometimes the later ones can override the earlier ones. This was trying to emulate csc behavior. For ilc I don't see a reason to emulate that because running ilc on its own is not supported, only going through the SDK, which will never pass the new parameters multiple times.

For testing this enables the existing trimmer tests for this functionality. This uncovered a small issue in substitution parser, which is also fixed here. For simplicity the test infra emulates the illink command line behavior over ilc (which is easy as they're very similar).

Open question:
Trimmer introduces ILLinkTreatWarningsAsErrors property which defaults to TreatWarningsAsErrors but can be overriden and it specifically applies on to illink. I didn't introduce a new one for ilc as I don't see much value in doing so, but I may be missing something. @sbomer - do you think we should add IlcTreatWarningsAsErrors?

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

Looks like tests will need some warning disabling since the warnings now trigger a build failure.

[sbomer]

@sbomer - do you think we should add IlcTreatWarningsAsErrors

No strong opinion. I would probably add it in case it comes in handy for someone, but it's probably not going to be widely useful. We can also always add it later if needed.

[MichalStrehovsky]

Thank you for doing the "nice" suppressions in the tests (instead of just blanket disabling everything).

Looks like there's more failures outside nativeaot/SmokeTests.

We should also run the pri0 suite since I assume there will be more of these in the test tree. If you don't want to run it locally, I suggest disabling WarnAsError in the PR temporarily so that the build doesn't just stop after a couple of these (or look at warnings in some older PRs where we did azp run runtime-nativeaot-outerloop).

[vitek-karas]

I added the IlcTreatWarningsAsErrors.
And then I used it in tests to disable it - to avoid breaking tests if some code doesn't correctly suppress all the right warnings. Basically as before the change.
I guess we could try fixing the tests, but this felt simpler.

[vitek-karas]

/azp run runtime-nativeaot-outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vitek-karas]

All the failures seem to be known (The S.T.Json failure is the crash without a dump problem reported elsewhere already).
@MichalStrehovsky could you please take a look at the recent test changes if you like it?

[MichalStrehovsky]

Thank you! Sorry for the delay, I thought I signed off already.