Add nowarn option #1324

mateoatr · 2020-07-08T07:37:24Z

This PR adds the new --nowarn option that behaves similarly to the Roslyn option of the same name. The user can turn off warning messages by specifying an ILLinker warning code. Some examples:

--nowarn IL2001
--nowarn IL2002;IL2003;IL2004
--nowarn IL2005,IL2006;IL2007
--nowarn IL2008,IL2009;IL2010
--nowarn "IL2011 IL2012,IL2013"

It also adds a new warning subcategory, TrimCorrectness, and a new ILLinkTask property, NoWarn.

This is the set of warnings that will be turned off whenever TrimmerAnalysisWarnings is set to true:

Code	Message	Subcategory
2006	-	TrimCorrectness
2026	Calling method annotated with RequiresUnreferencedCodeAttribute	TrimCorrectness
2043	Trying to propagate DynamicallyAccessedMemberAttribute from property 'property' to its getter 'method', but it already has such attribute.	TrimCorrectness

/cc @agocke @eerhardt

src/ILLink.Tasks/LinkTask.cs

vitek-karas · 2020-07-08T08:39:22Z

src/ILLink.Tasks/LinkTask.cs

+		/// Turns off trim correctness analysis warnings.
+		/// Maps to '--nowarn 2006;2026;2037;2043'.
+		/// </summary>
+		public bool TrimmerAnalysisWarnings { set => _trimmerAnalysisWarnings = value; }


Regardless of where this should live the name needs to change.
Currently we have "Trim correctness" analysis. In the future I anticipate "Single-file correctness", "AOT correctness" and potentially more. The name must reflect this.

I would probably call it EnableTrimCorrectnessAnalysis or TrimCorrectnessAnalysis (whichever fits the current style in SDK better). It would be "false" by default for now.

My understanding is that this should live in the SDK only for now, not in the task. For naming, I personally find correctness a little redundant. What do you think of TrimAnalysis?

Maybe it would make sense to preemptively open an SDK PR that introduces the option to discuss naming there, and avoid getting caught up in it here? I'm happy to do that if you think it makes sense @vitek-karas @mateoatr

Maybe it would make sense to preemptively open an SDK PR that introduces the option to discuss naming there, and avoid getting caught up in it here? I'm happy to do that if you think it makes sense @vitek-karas @mateoatr

This sounds good. I'm dropping this property from here then. I think EnableTrimAnalysis would be a good name, but we can follow up on this conversation in the SDK.

@sbomer Please prepare the SDK change (you can do that fastest)... EnableTrimAnalysis sounds good - please include Sam in the discussion on the SDK side.

dotnet/sdk#12388 to get the discussion started. I will add tests and such as we go.

src/ILLink.Tasks/LinkTask.cs

src/linker/Linker.Dataflow/FlowAnnotations.cs

src/linker/Linker/Driver.cs

src/linker/Linker/LoggingReflectionPatternRecorder.cs

PR feedback

src/linker/Linker/Driver.cs

marek-safar · 2020-07-08T17:35:48Z

src/linker/Linker/MessageSubcategory.cs

@@ -7,6 +7,7 @@ namespace Mono.Linker
 	public static class MessageSubCategory
 	{
 		public const string None = "";
+		public const string TrimCorrectness = "Trim correctness";


What is this for? I don't think this term is broadly understandable

I agree it might not be very informative to all users, but its main purpose is to group all warnings related to trimming analysis. Going forward, if we ever allow subcategory mapping in nowarn (things like --nowarn trimcorrectness = --nowarn IL2006;IL2026;IL2043) it might come handy. What do you think could be a more informative/useful subcategory name/message?

I think something like "trimalert" for the flag and "Trim alert" for subcategory printing is more readable.
--nowarn trimalert explains the purpose
Trim alert warning IL2006 : Mono.Linker.Tests....
No warn on correctness sounds like the program warns when is correct and we are disabling it. Also, since it's a broader term sounds like it should have a lot more warnings inside the subcategory.

Given the discussion on the public property - maybe "TrimAnalysis" ?

I think TrimAnalysis is a good option given that we'll probably have an EnableTrimAnlaysis property in the SDK. Correctness is even broader than TrimCorrectness.

src/linker/Linker/Driver.cs

src/linker/Linker/MessageSubcategory.cs

docs/illink-options.md

Co-authored-by: Marek Safar <marek.safar@gmail.com>

src/linker/Linker/Driver.cs

vitek-karas · 2020-07-08T21:06:57Z

src/linker/Linker/MessageSubcategory.cs

@@ -7,6 +7,7 @@ namespace Mono.Linker
 	public static class MessageSubCategory
 	{
 		public const string None = "";
+		public const string TrimCorrectness = "Trim correctness";


Given the discussion on the public property - maybe "TrimAnalysis" ?

src/ILLink.Tasks/LinkTask.cs

Fix NoWarn LinkTask property type

New versions of the linker will emit warnings for code that can be broken by trimming. dotnet/linker#1324 will add support for --nowarn, which we will use to leave them off by default in .NET5 to prevent noise. This change turns them off and adds a new property to let developers opt into the warnings.

src/ILLink.Tasks/LinkTask.cs

Co-authored-by: Sven Boemer <sbomer@gmail.com>

vitek-karas

LGTM
We should update the category name once we agree on the public facing property name for it (so that they match), but that can be done in a separate PR.

New versions of the linker will emit warnings for code that can be broken by trimming. dotnet/linker#1324 will add support for --nowarn, which we will use to leave them off by default in .NET5 to prevent noise. This change turns them off and adds a new property to let developers opt into the warnings.

* Introduce option to enable trim analysis warnings New versions of the linker will emit warnings for code that can be broken by trimming. dotnet/linker#1324 will add support for --nowarn, which we will use to leave them off by default in .NET5 to prevent noise. This change turns them off and adds a new property to let developers opt into the warnings. * Add tests * Also disable IL2046/IL2047 * Test no warnings for helloworld app * Print error based on exit code * Rename option to SuppressTrimAnalysisWarnings * Fix up tests * Require recent MSBuild in tests * Update warning suppressions * Check for more specific string in tests The full framework MSBuild verbosity was logging --nowarn arguments, causing some of the tests to fail. * Fix error message wording

* Add nowarn option * Allow multiple --nowarn * Add `--nowarn` to docs. * Add NoWarn test for ILLinkTask Commit migrated from dotnet/linker@73dd832

Mateo Torres Ruiz added 3 commits June 29, 2020 10:16

Add nowarn option

a2a12ac

Implement nowarn according to discussion

b28a11c

Merge with master

dc1223a

mateoatr added this to the .NET5.0 milestone Jul 8, 2020

mateoatr requested review from marek-safar, sbomer, vitek-karas, MichalStrehovsky and tlakollo July 8, 2020 07:37

mateoatr self-assigned this Jul 8, 2020

vitek-karas reviewed Jul 8, 2020

View reviewed changes

Mateo Torres Ruiz added 2 commits July 8, 2020 10:15

Allow multiple --nowarn

92585fd

PR feedback

Remove EnableTrimCorrectnessAnalysis

071ae28

marek-safar reviewed Jul 8, 2020

View reviewed changes

Add --nowarn to docs.

5a3e19a

marek-safar reviewed Jul 8, 2020

View reviewed changes

src/linker/Linker/Driver.cs Outdated Show resolved Hide resolved

marek-safar reviewed Jul 8, 2020

View reviewed changes

src/linker/Linker/MessageSubcategory.cs Outdated Show resolved Hide resolved

tlakollo reviewed Jul 8, 2020

View reviewed changes

docs/illink-options.md Show resolved Hide resolved

Remove redundant dot

2e3a163

Co-authored-by: Marek Safar <marek.safar@gmail.com>

vitek-karas reviewed Jul 8, 2020

View reviewed changes

sbomer reviewed Jul 8, 2020

View reviewed changes

src/ILLink.Tasks/LinkTask.cs Outdated Show resolved Hide resolved

Mateo Torres Ruiz added 3 commits July 8, 2020 14:19

Change TrimCorrectness to TrimAnalysis

e1055f4

Fix NoWarn LinkTask property type

Merge branch 'nowarn' of https://github.com/mateoatr/linker into nowarn

5389c1d

Use ordinal string comparison

c0ce863

sbomer mentioned this pull request Jul 8, 2020

Introduce option to enable trim analysis warnings dotnet/sdk#12388

Closed

sbomer reviewed Jul 8, 2020

View reviewed changes

src/ILLink.Tasks/LinkTask.cs Outdated Show resolved Hide resolved

mateoatr and others added 2 commits July 8, 2020 15:06

Use Quote

475f4c5

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Add NoWarn test for ILLinkTask

3d40f17

vitek-karas approved these changes Jul 9, 2020

View reviewed changes

mateoatr merged commit 73dd832 into dotnet:master Jul 9, 2020

sbomer mentioned this pull request Aug 6, 2020

Introduce option to enable trim analysis warnings dotnet/sdk#12815

Merged

tkapin pushed a commit to tkapin/runtime that referenced this pull request Jan 31, 2023

Add nowarn option (dotnet/linker#1324)

37fc2f3

* Add nowarn option * Allow multiple --nowarn * Add `--nowarn` to docs. * Add NoWarn test for ILLinkTask Commit migrated from dotnet/linker@73dd832

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nowarn option #1324

Add nowarn option #1324

mateoatr commented Jul 8, 2020 •

edited

Loading

vitek-karas Jul 8, 2020

sbomer Jul 8, 2020

mateoatr Jul 8, 2020

vitek-karas Jul 8, 2020

sbomer Jul 8, 2020

marek-safar Jul 8, 2020

mateoatr Jul 8, 2020

tlakollo Jul 8, 2020

vitek-karas Jul 8, 2020

mateoatr Jul 8, 2020

vitek-karas Jul 8, 2020

vitek-karas left a comment

Add nowarn option #1324

Add nowarn option #1324

Conversation

mateoatr commented Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

mateoatr commented Jul 8, 2020 •

edited

Loading