Conversation
This reverts commit 211a2e1.
JaclynCodes
left a comment
There was a problem hiding this comment.
jaclyncodes:revert-13125-issue-13082-remove-dead-code
There was a problem hiding this comment.
Pull request overview
This PR reverts PR #13125, which removed dead code related to System.ValueTuple handling in the ValidateMSBuildPackageDependencyVersions task. The revert restores special case logic for System.ValueTuple binding redirects and associated commented-out validation code.
Changes:
- Restores conditional logic to allow System.ValueTuple to have a binding redirect to version 4.0.0.0 when MSBuild ships version 4.0.3.0
- Re-adds commented-out code for tracking whether System.ValueTuple binding redirect was found
- Moves the error logging for version mismatches into an else block
| // if (!foundSystemValueTuple) | ||
| // { | ||
| // Log.LogError("Binding redirect for 'System.ValueTuple' missing."); | ||
| // } |
There was a problem hiding this comment.
These commented-out lines are part of the unreachable System.ValueTuple validation logic. Since System.ValueTuple is in the assembliesToIgnore array on line 23, it's filtered out at line 84, making the special case handling on lines 90-98 unreachable. This validation check can never execute, so the commented-out code should be removed entirely.
| if (String.Equals(name, "System.ValueTuple", StringComparison.OrdinalIgnoreCase) && | ||
| String.Equals(version, "4.0.0.0") && String.Equals(assemblyVersion, "4.0.3.0")) | ||
| { | ||
| // foundSystemValueTuple = true; | ||
| } | ||
| else | ||
| { | ||
| Log.LogError( | ||
| subcategory: null, | ||
| errorCode: null, | ||
| helpKeyword: null, | ||
| file: appConfigPath, | ||
| lineNumber: bindingRedirectLineNumber, | ||
| columnNumber: 0, | ||
| endLineNumber: 0, | ||
| endColumnNumber: 0, | ||
| message: $"Binding redirect for '{name}' redirects to a different version ({version}) than MSBuild ships ({assemblyVersion})."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // if (!foundSystemValueTuple) | ||
| // { | ||
| // Log.LogError("Binding redirect for 'System.ValueTuple' missing."); | ||
| // } |
There was a problem hiding this comment.
This special case handling for System.ValueTuple is unreachable code. On line 23, "System.ValueTuple" is included in the assembliesToIgnore array, and on line 84, the code checks !assembliesToIgnore.Contains(name, StringComparer.OrdinalIgnoreCase) before entering this block. This means when name is "System.ValueTuple", the entire block (lines 86-112) is skipped, making this special case check on lines 94-98 impossible to reach. The original PR #13125 was correct to remove this as dead code.
| if (String.Equals(name, "System.ValueTuple", StringComparison.OrdinalIgnoreCase) && | |
| String.Equals(version, "4.0.0.0") && String.Equals(assemblyVersion, "4.0.3.0")) | |
| { | |
| // foundSystemValueTuple = true; | |
| } | |
| else | |
| { | |
| Log.LogError( | |
| subcategory: null, | |
| errorCode: null, | |
| helpKeyword: null, | |
| file: appConfigPath, | |
| lineNumber: bindingRedirectLineNumber, | |
| columnNumber: 0, | |
| endLineNumber: 0, | |
| endColumnNumber: 0, | |
| message: $"Binding redirect for '{name}' redirects to a different version ({version}) than MSBuild ships ({assemblyVersion})."); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // if (!foundSystemValueTuple) | |
| // { | |
| // Log.LogError("Binding redirect for 'System.ValueTuple' missing."); | |
| // } | |
| Log.LogError( | |
| subcategory: null, | |
| errorCode: null, | |
| helpKeyword: null, | |
| file: appConfigPath, | |
| lineNumber: bindingRedirectLineNumber, | |
| columnNumber: 0, | |
| endLineNumber: 0, | |
| endColumnNumber: 0, | |
| message: $"Binding redirect for '{name}' redirects to a different version ({version}) than MSBuild ships ({assemblyVersion})."); | |
| } | |
| } | |
| } | |
| } |
|
@JaclynCodes this PR was raised without an issue or clear description of its goals, and all it does is add some commented-out, dead code. I'm closing this as a result. In the unlikely case that there's some actual impact to your scenarios that this change would fix, please first raise an issue with your scenario before raising a PR. |
Reverts #13125