Correct handling of explicit -Empty:$false parameter value in New-Guid by logiclrd · Pull Request #26140 · PowerShell/PowerShell
Fix New-Guid's handling of -Empty to check the switch's value, rather than only its presence, allowing parameter values to be passed through easily.
PR Summary
This PR adds a test to reproduce the bug described in issue #25276, and then updates the implementation of the New-Guid cmdlet to fix the bug, making the test pass.
PR Context
Fixes #25276
Related to #25242
PR Checklist
- PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
- Summarized changes
- Make sure all
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header - This PR is ready to merge. If this PR is a work in progress, please open this as a Draft Pull Request and mark it as Ready to Review when it is ready to merge.
- Breaking changes
- None
- OR
- Experimental feature(s) needed
- Experimental feature name(s):
- User-facing changes
- Not Applicable
- OR
- Documentation needed
- Issue filed:
- Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
…ng the bug described in issue PowerShell#25276. Updated the implementation in NewGuidCommand.cs to correct the handling of explicit $false values passed to -Empty, making the test pass.
Correct handling explicit -Empty:$false parameter value in New-Guid
Correct handling of explicit -Empty:$false parameter value in New-Guid
@microsoft-github-policy-service agree
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
label
| else | ||
| { | ||
| guid = ParameterSetName is "Empty" ? Guid.Empty : Guid.NewGuid(); | ||
| guid = Empty.ToBool() ? Guid.Empty : Guid.NewGuid(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| guid = Empty.ToBool() ? Guid.Empty : Guid.NewGuid(); | |
| guid = Empty.IsPresent ? Guid.Empty : Guid.NewGuid(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be the more common pattern
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of .ToBool() instead of simply checking for presence is the entire point of this PR, actually. The caller could pass in -Empty:$false, in which case it would be present, but the caller's intent is to not get the empty GUID. (By checking which ParameterSet is in effect, the previous code was in effect also just checking for the presence of the switch.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is identical:
| public bool ToBool() | |
| { | |
| return _isPresent; | |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I agree we shouldn't use IsPresent. It may be implemented identically, but this appears to be a design error.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is accepted, then this PR can be rebased to use .IsSpecified. If it is not accepted, then I posit that it is essentially not important whether this PR uses .IsPresent or .ToBool() -- but that .IsPresent makes less sense since the bug being fixed in this instance is precisely related to the fact that a switch might be present with the explicit value $false.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no places that use the implicit conversion
I find 379 references to the implicit operator, for example:
| public SwitchParameter Paging | |
| { | |
| get { return _paging; } | |
| set { _paging = value; } | |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really weird. To test, I deleted the operator from the source code and ran Start-PSBuild, and it built with no errors. 🤔
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see errors at first. But I ran Start-PSBuild -Clean and now get error CS0029.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, fair enough.
