Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 25, 2025

Fixes #12808

Context

When a project sets both MSBuildWarningsAsMessages (e.g., NU1701) and NoWarn (e.g., NETSDK1233), NoWarn is ignored because the targets only set MSBuildWarningsAsMessages from $(NoWarn) when MSBuildWarningsAsMessages is empty.

Changes Made

  • Modified Microsoft.Common.CurrentVersion.targets to always append $(NoWarn) to $(MSBuildWarningsAsMessages):
<MSBuildWarningsAsMessages>$(MSBuildWarningsAsMessages);$(NoWarn)</MSBuildWarningsAsMessages>
  • Added three end-to-end tests that import Common targets to verify the behavior:
    • NoWarnHonoredWhenMSBuildWarningsAsMessagesIsSetViaTargets: Verifies NoWarn is honored when MSBuildWarningsAsMessages is already set
    • NoWarnWorksWhenMSBuildWarningsAsMessagesIsNotSetViaTargets: Ensures original behavior is preserved when MSBuildWarningsAsMessages is not set
    • DuplicateDiagnosticCodesInNoWarnAndMSBuildWarningsAsMessagesAreHandled: Verifies that duplicate diagnostic codes (when the same code appears in both NoWarn and MSBuildWarningsAsMessages) are handled correctly

Testing

  • All 28 existing WarningsAsMessagesAndErrors tests pass
  • New tests verify: NoWarn honored when MSBuildWarningsAsMessages is set, original behavior preserved when it's not, and duplicate diagnostic codes are handled correctly

Notes

The leading semicolon when MSBuildWarningsAsMessages is initially empty (producing ;NAT013) is correctly handled by SemiColonTokenizer, which discards empty segments. This pattern is consistent with other property appending in the targets (e.g., AssemblySearchPaths).


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@dotnet-policy-service
Copy link
Contributor

Hello @@copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo.

Copilot AI changed the title [WIP] Fix NoWarn being ignored when MSBuildWarningsAsMessages is set Fix NoWarn ignored when MSBuildWarningsAsMessages is set Nov 25, 2025
Copilot AI requested a review from JanProvaznik November 25, 2025 14:25
@JanProvaznik JanProvaznik marked this pull request as ready for review November 25, 2025 14:55
Copilot AI review requested due to automatic review settings November 25, 2025 14:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes issue #12808 where the NoWarn property was being ignored when MSBuildWarningsAsMessages was already set. The fix modifies Microsoft.Common.CurrentVersion.targets to unconditionally append $(NoWarn) to $(MSBuildWarningsAsMessages) instead of only setting it when MSBuildWarningsAsMessages is empty.

Key Changes:

  • Changed the property assignment from conditional to unconditional append in Microsoft.Common.CurrentVersion.targets
  • Added two end-to-end tests that import Common targets to verify both the bug fix and backward compatibility

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Tasks/Microsoft.Common.CurrentVersion.targets Modified line 669 to always append $(NoWarn) to $(MSBuildWarningsAsMessages) using semicolon concatenation, removing the conditional check that prevented this when MSBuildWarningsAsMessages was already set
src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs Added two test methods: one verifying that NoWarn is honored when MSBuildWarningsAsMessages is already set (the bug fix), and another ensuring the original behavior is preserved when MSBuildWarningsAsMessages is not set

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@baronfel
Copy link
Member

@copilot ensure that there are tests that cover duplicate diagnostic code handling - now that NoWarn is being mirrored into MSbuildWarningsAsMessages it's possible that users will have a code set in both properties, so we need to be able to handle that.

Copy link
Contributor Author

Copilot AI commented Nov 25, 2025

@copilot ensure that there are tests that cover duplicate diagnostic code handling - now that NoWarn is being mirrored into MSbuildWarningsAsMessages it's possible that users will have a code set ...

Added DuplicateDiagnosticCodesInNoWarnAndMSBuildWarningsAsMessagesAreHandled test in 27f3f04. This test verifies that when NAT012 appears in both NoWarn and MSBuildWarningsAsMessages, all warnings (including the duplicate) are correctly treated as messages.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • rcxvsblobprodcus328.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/msbuild/msbuild/artifacts/bin/bootstrap/core/dotnet dotnet test src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj --filter FullyQualifiedName~NoWarn --logger console;verbosity=normal (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@JanProvaznik JanProvaznik merged commit ec0251e into main Nov 26, 2025
10 checks passed
@JanProvaznik JanProvaznik deleted the copilot/fix-nowarn-ignored-msbwam branch November 26, 2025 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NoWarn ignored if MSBWAM is set

3 participants