-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add PropertyExists MSBuild condition function to distinguish between undefined and empty properties #12405
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
base: main
Are you sure you want to change the base?
Conversation
|
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. |
Co-authored-by: baronfel <[email protected]>
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| /// </summary> | ||
| internal bool PropertyExists(string propertyName) | ||
| { | ||
| P property = _properties.GetProperty(propertyName, 0, propertyName.Length - 1); |
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.
Can you use the IPropertyProvider<T>.GetProperty(string name) method, instead of doing the length calculations here?
| T GetProperty(string name); |
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.
The main motivation for this change would be to remove the propertyName.Length - 1 expression that looks like an off-by-one error, even though it isn't.
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.
@copilot use the IPropertyProvider.GetProperty(string name) method
This PR adds a new
PropertyExists()condition function to MSBuild that allows checking whether a property is actually defined, rather than relying on the current behavior where undefined properties are coerced to empty strings.Problem
Currently in MSBuild, there's no way to distinguish between:
<Property></Property>Both scenarios result in an empty string when the property is referenced, making it impossible to implement proper coalescing logic where an explicitly empty property should override another property's value.
Solution
Added a new
PropertyExists('PropertyName')function that:trueif the property exists in the project (even if it has an empty value)falseif the property is completely undefinedPropertyExists('$(SomePropertyName)')Usage Examples
Implementation
IConditionEvaluationStateinterface withPropertyExists(string propertyName)methodConditionEvaluationStatethat delegates toExpander.PropertyExists()PropertyExists()method toExpanderclass that checks if property is non-nullPropertyExistsfunction case toFunctionCallExpressionNode.BoolEvaluate()Testing
Exists()andHasTrailingSlash()functionsFixes #12404.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.