Properly Consider WarningsAsMessages In TaskLoggingHelper's `HasLoggedErrors` by benvillalobos · Pull Request #6308 · dotnet/msbuild

benvillalobos · 2021-03-29T01:04:24Z

Context

#6174 didn't properly account for warningsaserrors and warningsasmessages together. This PR makes each taskhost aware of both WarningsAsMessages and WarningsAsErrors when telling tasks whether or not an error should be treated as a warning.

Changes Made

LoggingService and TaskLoggingContext expose a GetWarningsAsMessages to each taskhost for them to store.
No changes to IBE8 API
Taskhosts first check if a warning would be treated as a message when telling tasks whether or not a warning should be treated as a warning.

Testing

Added test cases for WarningsAsMessages getting translated properly for the OOPTHN.
Added test cases for WarningsAsErrors and WarningsAsMessages for the same warning code.
Added test case for MSBuildTreatWarningsAsErrors and WarningsAsMessages on a logged warning (WAM takes priority)

Notes

Does MSBuildTreatWarningsAsMessages exist as a property a user can set?
If WarningsAsMessages is an empty set, does it follow the rules of WarningsAsErrors? (empty set = all warnings as errors) /cc: @jeffkl

… the logging service and taskloggingcontext

…en OOPTHN and TH

Forgind · 2021-03-29T19:40:27Z

+            if (WarningsAsErrors == null && (_warningsAsErrorsByProject == null || !_warningsAsErrorsByProject.ContainsKey(key)))
            {
                return null;
            }

+            HashSet<string> allWarningsAsErrors = new HashSet<string>();
+
+            if (WarningsAsErrors != null)
+            {
+                allWarningsAsErrors.UnionWith(WarningsAsErrors);
+            }
+
+            if (_warningsAsErrorsByProject != null)
+            {
+                if (_warningsAsErrorsByProject.TryGetValue(key, out ISet<string> warningsAsErrors))
+                {
+                    allWarningsAsErrors.UnionWith(warningsAsErrors);
+                }
+            }
+
+            return allWarningsAsErrors;


This could be simplified a good bit. Also, if they're both null, we should return null and not an empty HashSet, right? Otherwise, we'd be indicating that all warnings should be counted as errors. I'd recommend something similar below. Also, since they're essentially the same method, it seems a lot easier to make them one method and call it with slightly different parameters. (You don't have to change the public API for that, since you can make the heart of the implementation private.)

Suggested change

if (WarningsAsErrors == null && (_warningsAsErrorsByProject == null || !_warningsAsErrorsByProject.ContainsKey(key)))

{

return null;

}

HashSet<string> allWarningsAsErrors = new HashSet<string>();

if (WarningsAsErrors != null)

{

allWarningsAsErrors.UnionWith(WarningsAsErrors);

}

if (_warningsAsErrorsByProject != null)

{

if (_warningsAsErrorsByProject.TryGetValue(key, out ISet<string> warningsAsErrors))

{

allWarningsAsErrors.UnionWith(warningsAsErrors);

}

}

return allWarningsAsErrors;

ISet<string> warningsAsErrors = null;

if (_warningsAsErrorsByProject is not null && _warningsAsErrorsByProject.TryGetValue(key, out warningsAsErrors)

{

if (WarningsAsErrors is not null)

{

warningsAsErrors.UnionWith(WarningsAsErrors);

}

return warningsAsErrors;

}

else

{

return WarningsAsErrors;

}

If they're both null they'll return through the first if statement. I do like your suggested change though, less to sift through.

Forgind · 2021-03-29T19:52:48Z

+        [Fact]
+        public void TaskReturnsHasLoggedErrorAndLogsWarningAsError_WarningIsAlsoMessage_BuildShouldContinue()
+        {
+            using (TestEnvironment env = TestEnvironment.Create(_output))


I don't normally look critically at tests, but please make sure your tests are substantially different from each other. I diffed your first and third tests with the second one, and they differed by one and two lines respectively. That could trivially become a theory and cut out almost a hundred lines of code.

Fair point. Streamlined the tests, also realized the two batched builds test were covering the same scenario. Removed one of them.

ladipro

I have added a few very minor comments, totally optional.

ladipro · 2021-03-30T12:39:41Z

+
+            Assert.NotNull(deserializedConfig.WarningsAsMessages);
+            config.WarningsAsMessages.SequenceEqual(deserializedConfig.WarningsAsMessages, StringComparer.Ordinal).ShouldBeTrue();
+


super-nit: Extra empty line.

ladipro · 2021-03-30T12:46:38Z

        {
-            if (WarningsAsErrors == null)
+            // Warnings as messages overrides warnings as errors.
+            if (WarningsAsErrors == null || (WarningsAsMessages != null && WarningsAsMessages.Contains(warningCode)))


super-nit:

Suggested change

if (WarningsAsErrors == null || (WarningsAsMessages != null && WarningsAsMessages.Contains(warningCode)))

if (WarningsAsErrors == null || WarningsAsMessages?.Contains(warningCode) == true)

Also, does empty WarningsAsMessages mean that all warnings should be treated as messages or does only WarningsAsErrors have this behavior?

Great question, I had the same when doing this PR. It doesn't look like it. ShouldTreatWarningAsError has this check:

if (WarningsAsErrors != null) { // Global warnings as errors apply to all projects. If the list is empty or contains the code, the warning should be treated as an error // if (WarningsAsErrors.Count == 0 || WarningsAsErrors.Contains(warningEvent.Code)) { return true; } }

whereas ShouldTreatWarningAsMessage checks like so:

// This only applies if the user specified /nowarn at the command-line or added the warning code through the object model // if (WarningsAsMessages?.Contains(warningEvent.Code) == true) { return true; }

There's no check for the count being zero, so I don't think that rule applies.

ladipro · 2021-03-30T12:49:33Z

        {
-            return WarningsAsErrors != null && (WarningsAsErrors.Count == 0 || WarningsAsErrors.Contains(warningCode));
+            // Warnings as messages overrides warnings as errors.
+            if (WarningsAsErrors == null || (WarningsAsMessages != null && WarningsAsMessages.Contains(warningCode)))


Suggested change

if (WarningsAsErrors == null || (WarningsAsMessages != null && WarningsAsMessages.Contains(warningCode)))

if (WarningsAsErrors == null || WarningsAsMessages?.Contains(warningCode) == true)

ladipro · 2021-03-30T12:53:46Z

+            translator.Translate(collection: ref _warningsAsMessages,
+                                 objectTranslator: (ITranslator t, ref string s) => t.Translate(ref s),
+#if CLR2COMPATIBILITY
+                                 collectionFactory: count => new HashSet<string>());


I see that this is the same as _warningsAsErrors a few lines above but I'm wondering if we shouldn't be constructing with StringComparer.OrdinalIgnoreCase on CLR2 as well. The constructor taking both capacity and comparer was introduced later but the one taking IEqualityComparer<T> should be available.

Forgind

LGTM! Thanks for refactoring.

Forgind · 2021-03-30T15:16:20Z

-        /// Null if no warnings should be treated as errors.
        /// </returns>
-        public ICollection<string> GetWarningsToBeLoggedAsErrorsByProject(BuildEventContext context)
+        public ICollection<string> GetWarningsAsErrors(BuildEventContext context)


I'd still prefer for this method and the next to just call HelperMethodName(context, _warningsAsErrorsByProject, WarningsAsErrors) or the equivalent and have HelperMethodName include the real logic, but that's a little nit.

benvillalobos · 2021-03-31T16:07:27Z

Not sure what this test case is and why it's failing: OutOfProcProjectInstanceBasedBuildDoesNotReloadFromDisk. Rerunning.

benvillalobos added 4 commits March 28, 2021 17:31

Pass warningsasmessages to the taskhost. Add GetWarningsAsMessages to…

1b8e748

… the logging service and taskloggingcontext

Pass warningsasmessages to OOPTHN

f2de8f7

Add test for warningsasmessages serialization. Code consistenty betwe…

949f239

…en OOPTHN and TH

Add test cases for warnaserr and warnasmessage

810a85b

benvillalobos requested a review from jeffkl March 29, 2021 01:04

benvillalobos changed the title ~~Consider WarningsAsMessages In TaskLoggingHelper's HasLoggedErrors~~ Properly Consider WarningsAsMessages In TaskLoggingHelper's HasLoggedErrors Mar 29, 2021

jeffkl approved these changes Mar 29, 2021

View reviewed changes

Forgind suggested changes Mar 29, 2021

View reviewed changes

benvillalobos added 2 commits March 29, 2021 16:09

PR Feedback: Simplify GetWarningsAsMessages and Errors

0d44724

Streamline tests, remove unnecessary test

0559707

ladipro approved these changes Mar 30, 2021

View reviewed changes

Forgind approved these changes Mar 30, 2021

View reviewed changes

benvillalobos added 2 commits March 30, 2021 11:45

Fix taskreturnstrue tests

8b6356c

PR Feedback: Code cleanup. Pass IEqualityComparer under CLR2

9e36efd

Forgind mentioned this pull request Mar 31, 2021

Verify that a key is in the warningsAsErrors dictionary before using it #6269

Merged

benvillalobos 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 Mar 31, 2021

Forgind merged commit 80f316e into dotnet:main Mar 31, 2021

benvillalobos self-assigned this Apr 1, 2021

benvillalobos mentioned this pull request May 21, 2021

Add option for "warning as errors except ..." #3062

Closed


		Assert.NotNull(deserializedConfig.WarningsAsMessages);
		config.WarningsAsMessages.SequenceEqual(deserializedConfig.WarningsAsMessages, StringComparer.Ordinal).ShouldBeTrue();

	if (WarningsAsErrors == null \|\| (WarningsAsMessages != null && WarningsAsMessages.Contains(warningCode)))
	if (WarningsAsErrors == null \|\| WarningsAsMessages?.Contains(warningCode) == true)

Conversation

benvillalobos commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ladipro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benvillalobos commented Mar 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benvillalobos commented Mar 29, 2021 •

edited

Loading