Fix NullReferenceException when expanding property functions that return null#6414
Conversation
rainersigwald
left a comment
There was a problem hiding this comment.
LGTM and I agree that we should take this in 16.10. @marcpopMSFT, any objection? The regression was reported by 1ES.
|
|
||
| Expander<ProjectPropertyInstance, ProjectItemInstance> expander = new Expander<ProjectPropertyInstance, ProjectItemInstance>(pg, FileSystems.Default); | ||
|
|
||
| string result = expander.ExpandIntoStringLeaveEscaped("$([System.Convert]::ChangeType(,$(SomeStuff.GetType())))", ExpanderOptions.ExpandProperties, MockElementLocation.Instance); |
There was a problem hiding this comment.
Was this just a fancy way to get null before, and you're using the easier approach now?
There was a problem hiding this comment.
It was not evaluating to null before so not hitting the intended code path. Presumably because the (, argument omission makes us pass an empty string.
benvillalobos
left a comment
There was a problem hiding this comment.
Took me a sec to realize the "meat" of the change is just making sure propertyValue doesn't get added if null. LGTM
| string result = expander.ExpandIntoStringLeaveEscaped("$([System.Convert]::ChangeType(,$(SomeStuff.GetType())))", ExpanderOptions.ExpandProperties, MockElementLocation.Instance); | ||
|
|
||
| // The null-returning function is the only thing in the expression. | ||
| string result = expander.ExpandIntoStringLeaveEscaped("$([System.Environment]::GetEnvironmentVariable(`_NonExistentVar`))", ExpanderOptions.ExpandProperties, MockElementLocation.Instance); |
There was a problem hiding this comment.
I'm now reminded that if an environment var doesn't exist, GetEnvironmentVariable returns null.
Fixes #6413
Context
This is a regression introduced in #6128. MSBuild crashes when evaluating a project where a property function returns null and its result is concatenated with another non-empty value.
Changes Made
Add a null check.
Testing
Fixed and extended the relevant test case.
Notes
Enable "Hide whitespace changes" when reviewing this change.