Fix regression in CodeAnalysisTreatWarningsAsErrors when set to false #6450
+34
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes [AB#1726853](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1726853) dotnet#6427 re-implemented CodeAnalysisTreatWarningsAsErrors support using global config files instead of appending CA rule IDs to WarningsAsErrors and WarningsNotAsErrors properties. However, this new implementation only works as expected for `CodeAnalysisTreatWarningsAsErrors = true` case, but doesn't prevent escalating CA warnings to errors for `CodeAnalysisTreatWarningsAsErrors = false` and `TreatWarningsAsErrors = true` case. I am restoring the WarningsNotAsErrors logic deleted from dotnet#6427 to fix this case. I am also planning to file a separate tracking issue in Roslyn to see if we can implement this whole CodeAnalysisTreatWarningsAsErrors functionality in the compiler layer itself, instead of the current hybrid implementations in this repo, which seems to be quite a bit fragile.
Youssef1313
reviewed
Jan 20, 2023
Comment on lines
+1615
to
+1624
| <!-- | ||
| Design-time target to handle 'CodeAnalysisTreatWarningsAsErrors = false' for the CA rule ids implemented in this package. | ||
| Note that a similar 'WarningsNotAsErrors' property group is present in the generated props file to ensure this functionality on command line builds. | ||
| --> | ||
| <Target Name=""_CodeAnalysisTreatWarningsAsErrors"" BeforeTargets=""CoreCompile"" Condition=""'$(DesignTimeBuild)' == 'true' OR '$(BuildingProject)' != 'true'""> | ||
| <PropertyGroup> | ||
| <EffectiveCodeAnalysisTreatWarningsAsErrors Condition=""'$(EffectiveCodeAnalysisTreatWarningsAsErrors)' == ''"">$(CodeAnalysisTreatWarningsAsErrors)</EffectiveCodeAnalysisTreatWarningsAsErrors> | ||
| <WarningsNotAsErrors Condition=""'$(EffectiveCodeAnalysisTreatWarningsAsErrors)' == 'false' and '$(TreatWarningsAsErrors)' == 'true'"">$(WarningsNotAsErrors);$(CodeAnalysisRuleIds)</WarningsNotAsErrors> | ||
| </PropertyGroup> | ||
| </Target> |
There was a problem hiding this comment.
Isn't this the same logic as in props? I'm not quite sure what is the need to wrap it in a target that runs only for design-time builds.
There was a problem hiding this comment.
This was added earlier to account for a regression. I am just restoring the old functionality here for WarningsNotAsErrors. Either way, I personally don't like any of the current CodeAnalysisTreatWarningsAsErrors implementations. I am going to drive implementing this in the core analyzer driver in Roslyn and delete all the code we have in this repo related to CodeAnalysisTreatWarningsAsErrors.
Youssef1313
approved these changes
Jan 20, 2023
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6450 +/- ##
==========================================
- Coverage 96.15% 96.14% -0.01%
==========================================
Files 1365 1365
Lines 317523 317523
Branches 10269 10269
==========================================
- Hits 305301 305296 -5
- Misses 9786 9789 +3
- Partials 2436 2438 +2 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes AB#1726853
#6427 re-implemented CodeAnalysisTreatWarningsAsErrors support using global config files instead of appending CA rule IDs to WarningsAsErrors and WarningsNotAsErrors properties. However, this new implementation only works as expected for
CodeAnalysisTreatWarningsAsErrors = truecase, but doesn't prevent escalating CA warnings to errors forCodeAnalysisTreatWarningsAsErrors = falseandTreatWarningsAsErrors = truecase. I am restoring the WarningsNotAsErrors logic deleted from #6427 to fix this case.I am also planning to file a separate tracking issue in Roslyn to see if we can implement this whole CodeAnalysisTreatWarningsAsErrors functionality in the compiler layer itself, instead of the current hybrid implementations in this repo, which seems to be quite a bit fragile.