Add support for --warn <version> #1381

sbomer · 2020-07-22T22:43:40Z

Adds support for warning versions, following the design described in dotnet/roslyn#46148. Fixes #1359.

This passes in a warning version at each callsite, like what we are doing with the warning strings. I plan to update the callsites after we agree on the API.
The version is optional. Non-versioned warnings always show up (but I think we will make all of the warnings versioned anyway). Custom steps can optionally use versions for their warnings - they will be controlled by the same global version setting as the built-in warnings.
Internally it uses an enum to represent the versions as @agocke suggested
Our command-line default is 9999 - the "neutral" default that will produce all warnings we support. There is separate SDK work to enable this option with different defaults per TFM (0 for netcoreapp3.0, 5 for net5.0).

filed dotnet/sdk#12601 for the SDK work.

src/ILLink.Tasks/LinkTask.cs

src/linker/Linker/LoggingReflectionPatternRecorder.cs

vitek-karas · 2020-07-22T23:00:34Z

src/linker/Linker/LinkContext.cs

+		// TODO
+		public void LogWarning (string text, int code, IMemberDefinition origin, int? ilOffset = null, string subcategory = MessageSubCategory.None, WarnVersion? version = null)


I don't have a problem with the API as is - I would just like better "defaults".

For warnings from the linker itself:

we should imply ILLink5

in the future we should imply warning version based on the code (new warnings will get higher numbers, so there will be a clear line - if above this number -> higher version)

For warnings from custom steps - it should basically mean "report always" (ignore warn version) by default (unless they pass a value in this API). Would be interesting how Roslyn plans to handle this for analyzers... but not a big deal I think.

As I understand the plan from reading through github issues and PRs, Roslyn allows analyzers to ship with some MSBuild files that are imported to the build, which are expected to map the SDK AnalysisLevel to an editorconfig file also shipped with the analyzer. The editorconfig files control warning severity, so this lets analyzers pick warning behavior with a different file per wave. They are passed to Roslyn on the command-line and combined with user editorconfigs. I think analyzers are also be able to read the warning version passed directly to Roslyn and make decisions based on that - but the public warning API doesn't mention versions, so it would be up to each analyzer to filter its own warnings based on the version.

At least that's what I pieced together... I could be completely mistaken. :) @agocke would be able to answer better.

If we put a default version in the API, how would we change it for future versions? We need some place that remembers the version when a warning was introduced - currently it would be in the callsite that creates a warning. The current API also has the "report always" behavior when no version is given.

I was not asking for changing the default in the API - as in defaulting the value to some constant. I was asking for: If the caller doesn't specify the value, then based on the code, calculate the version.

Reasoning:

code -> version mapping must be fixed

every callsite using a given code must use the same version - hard to guarantee

code -> version mapping must not change in the future (easier to guarantee)

would be easier to review/make sure it's correct if in a single location

Basically - the callsite should really only worry about which warning to use (code). In the future I want us to remove the message from the callsite as well (resources), so the callsite would only specify the code and parameters for the message.

I see - that's what I was referring to in the first bullet in the PR. I would like to simplify the API to just take a code and map it to a message and version in one place, but given how contentious that idea was last time, I thought it better to be consistent with the existing API until we decided to factor it.

Maybe now is a good opportunity to do that factoring? I'm curious what @marek-safar thinks.

I think we need sort of a double treatment here:

For the warnings from the linker itself - I strongly feel that having the API only take code is the right approach

For custom steps we need the current API which allows everything to be passed in - since we can't build the code->message/version map into the linker

I'm working on a separate PR which will make sure that we only use one message per code (still no API refactoring though). So once that is done we can do the refactoring and create the map.

I put the mapping from code -> version into LinkContext.LogWarning, assuming that will be the API that we eventually factor for warnings from the linker itself. PTAL.

src/linker/Linker/LinkContext.cs

mateoatr · 2020-07-22T23:04:12Z

src/linker/Linker/MessageContainer.cs

 		/// <returns>New MessageContainer of 'Warning' category</returns>
-		public static MessageContainer CreateWarningMessage (LinkContext context, string text, int code, MessageOrigin origin, string subcategory = MessageSubCategory.None)
+		public static MessageContainer CreateWarningMessage (LinkContext context, string text, int code, MessageOrigin origin, string subcategory = MessageSubCategory.None, WarnVersion? version = null)


Why do we default to null and not to Latest, here and also in LinkContext?

I don't want any warnings to explicitly have version Latest - they should either be unversioned (always on - which is what the null does), or versioned with a specific number. ~~I personally find that more clear than giving the unversioned warnings a version Latest, since that suggests they were recently introduced.~~

edit: forget the last part - it doesn't make sense. I would rather have the warnings be unversioned by default, than give them a Latest version - which would make them perpetually Latest, so they wouldn't show up by default in any release unless the developer explicitly requested it. I don't think we want that behavior for anything, and if we do it seems better to be explicit about it.

src/linker/Linker/LinkContext.cs

vitek-karas · 2020-07-23T21:02:22Z

src/linker/Linker/LinkContext.cs

+		public static WarnVersion GetWarningVersion (int code)
+		{
+			// TODO: Once we add warnings beyond .NET5, give them a new WarnVersion
+			return WarnVersion.ILLink5;


If the code is in custom steps range we should return the null version - basically always show the warning (steps should specify warning version via the API)

What is the custom step range? As far as I can tell we only expose CreateWarningMessage to custom steps, and we don't check for conflicts with built-in warnings - I thought this is what you meant to do in the PR you mentioned you were working on.

https://github.com/mono/linker/blob/master/docs/error-codes.md#illink-errors-and-warnings specifically:

The known codes are in the range 1000 to 6000. Custom steps should avoid using this range not to collide with existing or future error and warning codes.

I see, then we should change the check in CreateWarningMessage. I'm hesitant to do that here since I don't know whether there are already steps relying on the current behavior - I'll file an issue about it.

src/linker/Linker/LinkContext.cs

- Fix typo - Clarify behavior for null version

marek-safar · 2020-07-29T13:13:13Z

src/linker/Linker/Driver.cs

@@ -1059,6 +1084,8 @@ static void Usage ()
 			Console.WriteLine ("  -out PATH                     Specify the output directory. Defaults to 'output'");
 			Console.WriteLine ("  --about                       About the {0}", _linker);
 			Console.WriteLine ("  --verbose                     Log messages indicating progress and warnings");
+			Console.WriteLine ("  --warn VERSION                Only print out warnings with version <= VERSION. Defaults to '9999'");


This description does not make much sense. We should be explicit about what is supported and that's right now the only version 5

marek-safar · 2020-07-29T13:14:28Z

docs/error-codes.md

+For versioned warnings, the warning version is indicated in parentheses following
+the error code. For example:
+
+#### `ILXXXX` (version): Message


What is the added value in duplicating the version in every message? We could just group the warnings per warning with one line

marek-safar · 2020-07-29T13:29:25Z

src/linker/ref/Linker/MessageContainer.cs

@@ -6,8 +6,8 @@ namespace Mono.Linker
 {
 	public readonly struct MessageContainer
 	{
-		public static MessageContainer CreateErrorMessage (string text, int code, string subcategory = "", MessageOrigin? origin = null, bool isWarnAsError = false) { throw null; }
-		public static MessageContainer CreateWarningMessage (LinkContext context, string text, int code, MessageOrigin origin, string subcategory = "") { throw null; }
+		public static MessageContainer CreateErrorMessage (string text, int code, string subcategory = "", MessageOrigin? origin = null, bool isWarnAsError = false, WarnVersion? version = null) { throw null; }


This does not make sense to me bool isWarnAsError = false, WarnVersion? version = null is an implementation detail and should not be exposed to public API.

* Add support for --warn <version> * Add documentation * Fix formatting * PR feedback - Fix typo - Clarify behavior for null version * Add doc comments * Infer version from code for internal warnings * Remove TODO * Test interaction with warnaserror * Fix test Commit migrated from dotnet/linker@8224c72

sbomer requested a review from marek-safar as a code owner July 22, 2020 22:43

sbomer requested review from mateoatr and vitek-karas July 22, 2020 22:43

vitek-karas reviewed Jul 22, 2020

View reviewed changes

mateoatr reviewed Jul 22, 2020

View reviewed changes

sbomer mentioned this pull request Jul 23, 2020

Pass warning options to ILLink dotnet/sdk#12601

Closed

vitek-karas reviewed Jul 23, 2020

View reviewed changes

sbomer requested review from vitek-karas and mateoatr July 23, 2020 21:02

vitek-karas approved these changes Jul 23, 2020

View reviewed changes

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

sbomer mentioned this pull request Jul 23, 2020

Clarify warning API for custom steps #1384

Closed

sbomer added 7 commits July 24, 2020 16:58

Add support for --warn <version>

c0ac5a3

Add documentation

9a21f0f

Fix formatting

a1bc7db

PR feedback

962e7fe

- Fix typo - Clarify behavior for null version

Add doc comments

2712a03

Infer version from code for internal warnings

c86bef1

Remove TODO

b4404e6

sbomer force-pushed the warnversion branch from fecb3db to b4404e6 Compare July 24, 2020 17:42

sbomer added 2 commits July 24, 2020 18:39

Test interaction with warnaserror

9a4d13d

Fix test

ce0dd99

sbomer merged commit 8224c72 into dotnet:master Jul 24, 2020

marek-safar reviewed Jul 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for --warn <version> #1381

Add support for --warn <version> #1381

sbomer commented Jul 22, 2020 •

edited

Loading

vitek-karas Jul 22, 2020

sbomer Jul 22, 2020 •

edited

Loading

vitek-karas Jul 23, 2020

sbomer Jul 23, 2020

vitek-karas Jul 23, 2020

sbomer Jul 23, 2020

mateoatr Jul 22, 2020

sbomer Jul 22, 2020 •

edited

Loading

vitek-karas Jul 23, 2020

sbomer Jul 23, 2020

vitek-karas Jul 23, 2020

sbomer Jul 23, 2020

sbomer Jul 23, 2020

marek-safar Jul 29, 2020

marek-safar Jul 29, 2020

marek-safar Jul 29, 2020

		// TODO
		public void LogWarning (string text, int code, IMemberDefinition origin, int? ilOffset = null, string subcategory = MessageSubCategory.None, WarnVersion? version = null)

Add support for --warn <version> #1381

Add support for --warn <version> #1381

Conversation

sbomer commented Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

sbomer Jul 22, 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

sbomer Jul 22, 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

sbomer commented Jul 22, 2020 •

edited

Loading

sbomer Jul 22, 2020 •

edited

Loading

sbomer Jul 22, 2020 •

edited

Loading