Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -670,14 +670,20 @@ private static List<CompletionResult> GetParameterCompletionResults(string param
{
Diagnostics.Assert(bindingInfo.InfoType.Equals(PseudoBindingInfoType.PseudoBindingSucceed), "The pseudo binding should succeed");
List<CompletionResult> result = new List<CompletionResult>();
Assembly commandAssembly = null;
if (bindingInfo.CommandInfo is CmdletInfo cmdletInfo)
{
commandAssembly = cmdletInfo.CommandMetadata.CommandType.Assembly;
}

if (parameterName == string.Empty)
{
result = GetParameterCompletionResults(
parameterName,
bindingInfo.ValidParameterSetsFlags,
bindingInfo.UnboundParameters,
withColon);
withColon,
commandAssembly);
return result;
}

Expand All @@ -699,7 +705,8 @@ private static List<CompletionResult> GetParameterCompletionResults(string param
parameterName,
bindingInfo.ValidParameterSetsFlags,
bindingInfo.UnboundParameters,
withColon);
withColon,
commandAssembly);
}

return result;
Expand All @@ -714,7 +721,8 @@ private static List<CompletionResult> GetParameterCompletionResults(string param
parameterName,
bindingInfo.ValidParameterSetsFlags,
bindingInfo.BoundParameters.Values,
withColon);
withColon,
commandAssembly);
}

return result;
Expand Down Expand Up @@ -778,19 +786,34 @@ private static List<CompletionResult> GetParameterCompletionResults(string param
parameterName,
bindingInfo.ValidParameterSetsFlags,
bindingInfo.UnboundParameters,
withColon);
withColon,
commandAssembly);
return result;
}

MergedCompiledCommandParameter param = bindingInfo.BoundParameters[matchedParameterName];

WildcardPattern pattern = WildcardPattern.Get(parameterName + "*", WildcardOptions.IgnoreCase);
string parameterType = "[" + ToStringCodeMethods.Type(param.Parameter.Type, dropNamespaces: true) + "] ";

string helpMessage = string.Empty;
if (param.Parameter.CompiledAttributes is not null)
{
foreach (Attribute attr in param.Parameter.CompiledAttributes)
{
if (attr is ParameterAttribute pattr && TryGetParameterHelpMessage(pattr, commandAssembly, out string attrHelpMessage))
{
helpMessage = $" - {attrHelpMessage}";
break;
}
}
}

string colonSuffix = withColon ? ":" : string.Empty;
if (pattern.IsMatch(matchedParameterName))
{
string completionText = "-" + matchedParameterName + colonSuffix;
string tooltip = parameterType + matchedParameterName;
string completionText = $"-{matchedParameterName}{colonSuffix}";
string tooltip = $"{parameterType}{matchedParameterName}{helpMessage}";
result.Add(new CompletionResult(completionText, matchedParameterName, CompletionResultType.ParameterName, tooltip));
}
else
Expand All @@ -804,27 +827,67 @@ private static List<CompletionResult> GetParameterCompletionResults(string param
$"-{alias}{colonSuffix}",
alias,
CompletionResultType.ParameterName,
parameterType + alias));
$"{parameterType}{alias}{helpMessage}"));
}
}
}

return result;
}

#nullable enable
/// <summary>
/// Try and get the help message text for the parameter attribute.
/// </summary>
/// <param name="attr">The attribute to check for the help message.</param>
/// <param name="assembly">The assembly to lookup resources messages, this should be the assembly the cmdlet is defined in.</param>
/// <param name="message">The help message if it was found otherwise null.</param>
/// <returns>True if the help message was set or false if not.></returns>
private static bool TryGetParameterHelpMessage(
ParameterAttribute attr,
Assembly? assembly,
[NotNullWhen(true)] out string? message)
{
message = null;

if (attr.HelpMessage is not null)
{
message = attr.HelpMessage;
return true;
}

if (assembly is null || attr.HelpMessageBaseName is null || attr.HelpMessageResourceId is null)
{
return false;
}

try
{
message = ResourceManagerCache.GetResourceString(assembly, attr.HelpMessageBaseName, attr.HelpMessageResourceId);
return message is not null;
}
catch (Exception)
{
return false;
}
}
#nullable disable

/// <summary>
/// Get the parameter completion results by using the given valid parameter sets and available parameters.
/// </summary>
/// <param name="parameterName"></param>
/// <param name="validParameterSetFlags"></param>
/// <param name="parameters"></param>
/// <param name="withColon"></param>
/// <param name="commandAssembly">Optional assembly used to lookup parameter help messages.</param>
/// <returns></returns>
private static List<CompletionResult> GetParameterCompletionResults(
string parameterName,
uint validParameterSetFlags,
IEnumerable<MergedCompiledCommandParameter> parameters,
bool withColon)
bool withColon,
Assembly commandAssembly = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same.

Copy link
Collaborator Author

@jborean93 jborean93 Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't but it saved me from specifying null for the argument where it is called. I can certainly make it without a default if you prefer that.

I do think the default is a nice way to see that the argument is nullable without having to turn on the nullability checks and annotating it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. This is not critical, it just makes me think that there are a number of call sites that use this default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case there still are, this is why I added the default in the first place because there are some calls where there is no assembly object to provide.

{
var result = new List<CompletionResult>();
var commonParamResult = new List<CompletionResult>();
Expand All @@ -840,6 +903,7 @@ private static List<CompletionResult> GetParameterCompletionResults(

string name = param.Parameter.Name;
string type = "[" + ToStringCodeMethods.Type(param.Parameter.Type, dropNamespaces: true) + "] ";
string helpMessage = null;
bool isCommonParameter = Cmdlet.CommonParameters.Contains(name, StringComparer.OrdinalIgnoreCase);
List<CompletionResult> listInUse = isCommonParameter ? commonParamResult : result;

Expand All @@ -855,20 +919,27 @@ private static List<CompletionResult> GetParameterCompletionResults(
{
foreach (var attr in compiledAttributes)
{
var pattr = attr as ParameterAttribute;
if (pattr != null && pattr.DontShow)
if (attr is ParameterAttribute pattr)
{
showToUser = false;
addCommonParameters = false;
break;
if (pattr.DontShow)
{
showToUser = false;
addCommonParameters = false;
break;
}

if (helpMessage is null && TryGetParameterHelpMessage(pattr, commandAssembly, out string attrHelpMessage))
{
helpMessage = $" - {attrHelpMessage}";
}
}
}
}

if (showToUser)
{
string completionText = "-" + name + colonSuffix;
string tooltip = type + name;
string completionText = $"-{name}{colonSuffix}";
string tooltip = $"{type}{name}{helpMessage}";
listInUse.Add(new CompletionResult(completionText, name, CompletionResultType.ParameterName,
tooltip));
}
Expand Down
157 changes: 157 additions & 0 deletions test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,163 @@ param([ValidatePattern(
$res.CompletionMatches[1].CompletionText | Should -BeExactly '$DemoVar2'
}

It 'Should include parameter help message in tool tip - SingleMatch <SingleMatch>' -TestCases @(
@{ SingleMatch = $true }
@{ SingleMatch = $false }
) {
param ($SingleMatch)

Function Test-Function {
param (
[Parameter(HelpMessage = 'Some help message')]
$ParamWithHelp,

$ParamWithoutHelp
)
}

$expected = '[Object] ParamWithHelp - Some help message'

if ($SingleMatch) {
$Script = 'Test-Function -ParamWithHelp'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches
}
else {
$Script = 'Test-Function -'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-ParamWithHelp'
}

$res.Count | Should -Be 1
$res.CompletionText | Should -BeExactly '-ParamWithHelp'
$res.ToolTip | Should -BeExactly $expected
Comment on lines +1145 to +1156
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to check all the cases explicitly to avoid regression in any form (in tests below too):

Suggested change
if ($SingleMatch) {
$Script = 'Test-Function -ParamWithHelp'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches
}
else {
$Script = 'Test-Function -'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-ParamWithHelp'
}
$res.Count | Should -Be 1
$res.CompletionText | Should -BeExactly '-ParamWithHelp'
$res.ToolTip | Should -BeExactly $expected
if ($SingleMatch) {
$Script = 'Test-Function -ParamWithHelp'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches
$res.Count | Should -Be 1
$res.CompletionText | Should -BeExactly '-ParamWithHelp'
$res.ToolTip | Should -BeExactly $expected
}
else {
$Script = 'Test-Function -'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-ParamWithHelp'
$res.Count | Should -Be 2
$res[0].CompletionText | Should -BeExactly '-ParamWithHelp'
$res[0].ToolTip | Should -BeExactly $expected
$res[1].CompletionText | Should -BeExactly '-ParamWithoutHelp'
$res[1].ToolTip | Should -BeExactly '[Object] ParamWithoutHelp'
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we are only testing the ToolTip message for each returned option but even if you wanted to have multiple tests there are a few problems with this approach:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not apply to this change directly, but if it is possible to make an additional check, why not add it and exclude regression in case someone reworks this code?
My suggestion is to expand this test a bit and check that the text from the first parameter is not randomly displayed for the second one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if attributes for another parameter are being applied to another we have a major problem that would/should be picked up by other tests. By testing it here as well we complicate an already somewhat complicated test that isn’t designed to check for this particular problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iSazonov any further response to @jborean93's response? We have implemented the appropriate parts of PSES to support this change and I'd like to pick this up again and move it forward if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JustinGrote I'm sorry, but I can't help you. Firstly, the contributor rejected my requests and expressed a desire to continue only with the MSFT team, and they do not have this work in their plans. Secondly, time is lost, because it's time to stabilize before the next release. So there is very little chance that it will be accepted soon.

}

It 'Should include parameter help resource message in tool tip - SingleMatch <SingleMatch>' -TestCases @(
@{ SingleMatch = $true }
@{ SingleMatch = $false }
) {
param ($SingleMatch)

$expected = '`[string`] Activity - *'

if ($SingleMatch) {
$Script = 'Write-Progress -Activity'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches
}
else {
$Script = 'Write-Progress -'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-Activity'
}

$res.Count | Should -Be 1
$res.CompletionText | Should -BeExactly '-Activity'
$res.ToolTip | Should -BeLikeExactly $expected
}

It 'Should skip empty parameter HelpMessage with multiple parameters - SingleMatch <SingleMatch>' -TestCases @(
@{ SingleMatch = $true }
@{ SingleMatch = $false }
) {
param ($SingleMatch)

Function Test-Function {
[CmdletBinding(DefaultParameterSetName = 'SetWithoutHelp')]
param (
[Parameter(ParameterSetName = 'SetWithHelp', HelpMessage = 'Help Message')]
[Parameter(ParameterSetName = 'SetWithoutHelp')]
[string]
$ParamWithHelp,

[Parameter(ParameterSetName = 'SetWithHelp')]
[switch]
$ParamWithoutHelp
)
}

$expected = '[string] ParamWithHelp - Help Message'

if ($SingleMatch) {
$Script = 'Test-Function -ParamWithHelp'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches
}
else {
$Script = 'Test-Function -'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-ParamWithHelp'
}

$res.Count | Should -Be 1
$res.CompletionText | Should -BeExactly '-ParamWithHelp'
$res.ToolTip | Should -BeExactly $expected
}

It 'Should retrieve help message from dynamic parameter' {
Function Test-Function {
[CmdletBinding()]
param ()
dynamicparam {
$attr = [System.Management.Automation.ParameterAttribute]@{
HelpMessage = "Howdy partner"
}
$attrCollection = [System.Collections.ObjectModel.Collection[System.Attribute]]::new()
$attrCollection.Add($attr)

$dynParam = [System.Management.Automation.RuntimeDefinedParameter]::new('DynamicParam', [string], $attrCollection)

$paramDictionary = [System.Management.Automation.RuntimeDefinedParameterDictionary]::new()
$paramDictionary.Add('DynamicParam', $dynParam)
$paramDictionary
}

end {}
}

$expected = '[string] DynamicParam - Howdy partner'
$Script = 'Test-Function -'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-DynamicParam'
$res.Count | Should -Be 1
$res.CompletionText | Should -BeExactly '-DynamicParam'
$res.ToolTip | Should -BeExactly $expected
}

It 'Should have type and name for parameter without help message' {
Function Test-Function {
param (
[Parameter()]
$WithParamAttribute,

$WithoutParamAttribute
)
}

$Script = 'Test-Function -'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches |
Where-Object CompletionText -in '-WithParamAttribute', '-WithoutParamAttribute' |
Sort-Object CompletionText
$res.Count | Should -Be 2

$res.CompletionText[0] | Should -BeExactly '-WithoutParamAttribute'
$res.ToolTip[0] | Should -BeExactly '[Object] WithoutParamAttribute'

$res.CompletionText[1] | Should -BeExactly '-WithParamAttribute'
$res.ToolTip[1] | Should -BeExactly '[Object] WithParamAttribute'
}

It 'Should ignore errors when faling to get HelpMessage resource' {
Function Test-Function {
param (
[Parameter(HelpMessageBaseName="invalid", HelpMessageResourceId="SomeId")]
$InvalidHelpParam
)
}

$expected = '[Object] InvalidHelpParam'
$Script = 'Test-Function -InvalidHelpParam'
$res = (TabExpansion2 -inputScript $Script).CompletionMatches
$res.Count | Should -Be 1
$res.CompletionText | Should -BeExactly '-InvalidHelpParam'
$res.ToolTip | Should -BeExactly $expected
}

Context 'Start-Process -Verb parameter completion' {
BeforeAll {
function GetProcessInfoVerbs([string]$path, [switch]$singleQuote, [switch]$doubleQuote) {
Expand Down
Loading