-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Any have metadata value when empty Fixes #5113 #8603
Any have metadata value when empty Fixes #5113 #8603
Conversation
Previously, when we saw the Item was empty, we'd jump out early. Fortunately, we already special-cased Count, and it turns out we can do the same with AnyHaveMetadataValue.
src/Build/Evaluation/Expander.cs
Outdated
@@ -1947,7 +1947,8 @@ private static class ItemExpander | |||
if (itemsOfType.Count == 0) | |||
{ | |||
// .. but only if there isn't a function "Count()", since that will want to return something (zero) for an empty list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expand comment please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought about this again. I can't see any reason that this would break someone but it's an observable change. I think it should be in the 17.6 changewave.
345016e
to
e6f221a
Compare
Put it in the change wave. Scenarios I can think of where someone would be broken are a bit ridiculous like trying to use it as IsEmpty: I really hope no one does that. I skipped adding ChangeWave-specific tests; hopefully you're ok with that 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a very long time to check this logic!
AnyHaveMetadataValue returns an empty string if the item is empty. This is a bug fixed in dotnet/msbuild#8603, but this branch does not yet have that change. This workaround should be resilient whether or not the fix is in.
Fixes #5113
Context
AnyHaveMetadataValue returns empty when the itemgroup is empty, which doesn't make sense and doesn't match our documentation. This changes it to return false in that case.
Changes Made
If we see the function we're evaluating is AnyHaveMetadataValue, don't skip out early.
Testing
Made a unit test
Notes