Implement warnnotaserror Fixes #3062 #7309

Forgind · 2022-01-21T01:21:39Z

Context

We previously had warnaserror (and a property for it) that, when you specified error codes, upgraded those error codes from warnings to errors. If you just left it empty, it upgraded all warnings to errors. (Null meant don't upgrade.)

This adds that you can ask for all error codes to be upgraded, then downgrade just a few of them (via codes) back to warnings.

Changes Made

Implement WarnNotAsError both as a command line switch and as a property.

Testing

Added a unit test. Tried it out from the command line.

Notes

Fixes dotnet#3062

Should now work for: TaskHosts If set via property (and then only for that project)

rainersigwald

Is there a way to override the command line switch?

I'd like to see tests for the properties and the interaction between properties and the CLI switches (specifically around msbuild -warnaserror + <MSBuildWarningsNotAsErrors>).

rainersigwald · 2022-01-21T15:42:28Z

src/MSBuild/Resources/Strings.resx

@@ -1258,6 +1274,14 @@ Copyright (C) Microsoft Corporation. All rights reserved.
      LOCALIZATION: The prefix "MSBUILD : error MSBxxxx:" should not be localized.
    </comment>
  </data>
+  <data name="MissingWarnNotAsErrorParameterError" UESanitized="true" Visibility="Public">
+    <value>MSBUILD : error MSB1060: Specify one or more warning codes to keep as warnings despite a global -warnaserror when using the -warnNotAsError switch.</value>


Suggested change

<value>MSBUILD : error MSB1060: Specify one or more warning codes to keep as warnings despite a global -warnaserror when using the -warnNotAsError switch.</value>

<value>MSBUILD : error MSB1060: Specify one or more warning codes when using the -warnNotAsError switch.</value>

rainersigwald · 2022-01-21T15:45:17Z

src/MSBuild/Resources/Strings.resx

+                     multiple warning codes. Has no effect if the -warnaserror
+                     switch is not set.


Why not error if -warnaserror is not on?

If warnaserror is not set at all, no warnings are promoted to errors anyway, so there's nothing to revert to a warning.

More generally, you can clear the warnaserror switch, so I thought of this as only important if the user asked all warnings to be promoted. That said, I'm not opposed to make it revert error codes even if there are only a few warnings promoted to errors. I'm curious as to whether it's more confusing, if someone is unaware of the WarningsNotAsErrors switch, to have a switch clearly set but not doing anything versus rely on all warnings to be promoted and have one just not be.

I'm proposing emitting a new warning at command-line-parsing time that says " you said warnnotaserror but didn't have any warnaserrors"

You could still have a project-level warnaserror that you wanted not promoted, right?

rainersigwald · 2022-01-21T15:48:31Z

src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs

@@ -748,7 +768,7 @@ public bool ShouldTreatWarningAsError(string warningCode)
            }

            // An empty set means all warnings are errors.
-            return WarningsAsErrors.Count == 0 || WarningsAsErrors.Contains(warningCode);
+            return (WarningsAsErrors.Count == 0 && (WarningsNotAsErrors == null || !WarningsNotAsErrors.Contains(warningCode))) || WarningsAsErrors.Contains(warningCode);


This is fairly confusing. Extract to a named local function and add comments?

I'll hold off on this for now until we've nailed down whether -warnaserror:FOR123 -warnnotaserror:FOR123 should have the warning promoted to an error or not. This should clean itself up if we go with it being just a warning.

rainersigwald · 2022-01-21T15:55:14Z

src/MSBuild/CommandLineSwitches.cs

@@ -263,6 +264,7 @@ bool emptyParametersAllowed
            new ParameterizedSwitchInfo(  new string[] { "preprocess", "pp" },                  ParameterizedSwitch.Preprocess,                 null,                           false,          null,                                  true,   false  ),
            new ParameterizedSwitchInfo(  new string[] { "targets", "ts" },                     ParameterizedSwitch.Targets,                    null,                           false,          null,                                  true,   false  ),
            new ParameterizedSwitchInfo(  new string[] { "warnaserror", "err" },                ParameterizedSwitch.WarningsAsErrors,           null,                           true,           null,                                  true,   true   ),
+            new ParameterizedSwitchInfo(  new string[] { "warnnotaserror", "noerr" },           ParameterizedSwitch.WarningsNotAsErrors,        null,                           true,           "MissingWarnNotAsErrorParameterError", true,   true   ),


Suggested change

new ParameterizedSwitchInfo( new string[] { "warnnotaserror", "noerr" }, ParameterizedSwitch.WarningsNotAsErrors, null, true, "MissingWarnNotAsErrorParameterError", true, true ),

new ParameterizedSwitchInfo( new string[] { "warnnotaserror", "noerr" }, ParameterizedSwitch.WarningsNotAsErrors, null, true, "MissingWarnNotAsErrorParameterError", true, false ),

rainersigwald · 2022-01-21T16:02:32Z

src/Build/BackEnd/Components/Logging/ILoggingService.cs

@@ -163,6 +163,15 @@ ISet<string> WarningsAsErrors
            set;
        }

+        /// <summary>
+        /// Set of warnings to not treat as errors. Only has any effect if WarningsAsErrors is non-null but empty.


Sounds like you're making a decision that it's invalid to build with -warnaserrors:AB1234,CD5678 and then in a project set <MSBuildWarningsNotAsErrors>AB1234</MSBuildWarningsNotAsErrors>. Do you think that's right? I'm not sure myself.

rainersigwald · 2022-01-21T16:07:55Z

src/MSBuild/XMake.cs

            // TODO: Parse an environment variable as well?



Suggested change

// TODO: Parse an environment variable as well?

rainersigwald · 2022-01-21T16:34:53Z

src/MSBuild/XMake.cs

                    // An empty /warnaserror is added as "null".  In this case, the list is cleared
                    // so that all warnings are treated errors
-                    warningsAsErrors.Clear();
+                    warningSwitches.Clear();


This behavior wasn't present on warnasmessage before. Should it be now?

Good catch, but also, I'd vote yes. It could theoretically break someone who has /warnasmessage:FOR123 /warnasmessage:, but if you were doing something bad, I'd say you deserve that. It's very logical to assume that converting warnings to errors/messages should work the same way, so we should do that.

rainersigwald · 2022-01-21T16:36:09Z

src/Build/PublicAPI/netstandard/PublicAPI.Unshipped.txt

Hm. Starting to wonder if the public API promotion should be in main rather than the release branch, since it will be an annoying merge here. But we also just checked in a revert that changed public API after forking so maybe not?

rainersigwald · 2022-01-21T16:45:30Z

src/Build/BackEnd/Components/Logging/LoggingService.cs

@@ -220,6 +220,11 @@ internal partial class LoggingService : ILoggingService, INodePacketHandler, IBu
        /// </summary>
        private IDictionary<int, ISet<string>> _warningsAsErrorsByProject;

+        /// <summary>
+        /// A list of warnings to treat as errors for an associated <see cref="BuildEventContext"/>.  If an empty set, all warnings are treated as errors.


rainersigwald · 2022-01-21T16:56:11Z

src/Build/BackEnd/Components/Logging/LoggingService.cs

+            return GetWarningsForProject(context, _warningsAsMessagesByProject, WarningsAsMessages);
+        }
+
+        private ICollection<string> GetWarningsForProject(BuildEventContext context, IDictionary<int, ISet<string>> warningsByProject, ISet<string> warnings)


Comments please

rainersigwald

LGTM with those added comments.

…into warn-not-as-error

I really don't know how this is even vaguely possible—but it looks like main is wrong?

Forgind added 4 commits January 20, 2022 13:59

Only log warnings with WarnNotAsError

977e445

Fixes dotnet#3062

Support WarnNotAsError elsewhere

d83e859

Should now work for: TaskHosts If set via property (and then only for that project)

Add test

b399154

Fix failing tests

97867fc

rainersigwald reviewed Jan 21, 2022

View reviewed changes

Forgind added 4 commits January 24, 2022 16:14

PR comments

576149a

Add more comprehensive tests

9808488

Little fix

95d2962

Merge branch 'main' into warn-not-as-error

21c54b2

rainersigwald approved these changes Jan 31, 2022

View reviewed changes

Forgind added 4 commits January 31, 2022 09:33

Add comments

06fa9fa

Merge branch 'warn-not-as-error' of https://github.com/Forgind/msbuild …

de263ad

…into warn-not-as-error

Bad merge?

8f0f08f

I really don't know how this is even vaguely possible—but it looks like main is wrong?

Move BuildThreadPriority to unshipped

a777eb3

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 7, 2022

Forgind merged commit 7d926d7 into dotnet:main Feb 16, 2022

Forgind deleted the warn-not-as-error branch February 16, 2022 01:45

baronfel added this to the VS 17.2 milestone Feb 23, 2022

baronfel mentioned this pull request Feb 28, 2022

Integrate MSBuildWarningsNotAsErrors with the SDK-level WarningsNotAsErrors property #7423

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement warnnotaserror Fixes #3062 #7309

Implement warnnotaserror Fixes #3062 #7309

Forgind commented Jan 21, 2022

rainersigwald left a comment

rainersigwald Jan 21, 2022

rainersigwald Jan 21, 2022

Forgind Jan 21, 2022

rainersigwald Jan 21, 2022

Forgind Jan 24, 2022

Forgind Jan 24, 2022

rainersigwald Jan 21, 2022

Forgind Jan 21, 2022

rainersigwald Jan 21, 2022

rainersigwald Jan 21, 2022

rainersigwald Jan 21, 2022

rainersigwald Jan 21, 2022

Forgind Jan 24, 2022

rainersigwald Jan 21, 2022

rainersigwald Jan 21, 2022

rainersigwald Jan 21, 2022

rainersigwald left a comment

	<value>MSBUILD : error MSB1060: Specify one or more warning codes to keep as warnings despite a global -warnaserror when using the -warnNotAsError switch.</value>
	<value>MSBUILD : error MSB1060: Specify one or more warning codes when using the -warnNotAsError switch.</value>

		multiple warning codes. Has no effect if the -warnaserror
		switch is not set.

	new ParameterizedSwitchInfo( new string[] { "warnnotaserror", "noerr" }, ParameterizedSwitch.WarningsNotAsErrors, null, true, "MissingWarnNotAsErrorParameterError", true, true ),
	new ParameterizedSwitchInfo( new string[] { "warnnotaserror", "noerr" }, ParameterizedSwitch.WarningsNotAsErrors, null, true, "MissingWarnNotAsErrorParameterError", true, false ),

Implement warnnotaserror Fixes #3062 #7309

Implement warnnotaserror Fixes #3062 #7309

Conversation

Forgind commented Jan 21, 2022

Context

Changes Made

Testing

Notes

rainersigwald left a 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

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

rainersigwald left a comment

Choose a reason for hiding this comment