Skip to content

Commit d2f2467

Browse files
Copilotbergmeister
Copilot
and
bergmeister
authored
Fix AvoidAlias review findings
Co-authored-by: bergmeister <9250262+bergmeister@users.noreply.github.com>
1 parent c550c39 commit d2f2467

2 files changed

Lines changed: 236 additions & 5 deletions

File tree

‎Rules/AvoidAlias.cs‎

Lines changed: 178 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,8 @@ private bool IsScriptDefinedFunction(
373373
out Dictionary<string, List<FunctionDefinitionAst>> functionNames)
374374
&& functionNames.TryGetValue(commandName, out List<FunctionDefinitionAst> functionDefinitions)
375375
&& functionDefinitions.Any(function =>
376-
(!function.Name.StartsWith("private:", StringComparison.OrdinalIgnoreCase)
376+
!IsDescendantOf(function, cmdAst)
377+
&& (!function.Name.StartsWith("private:", StringComparison.OrdinalIgnoreCase)
377378
|| currentScope == scope)
378379
&& !IsFunctionDeclaredAfterCommand(function, cmdAst, currentScope, functionAnalysis)))
379380
{
@@ -398,11 +399,35 @@ private bool IsFunctionDeclaredAfterCommand(
398399
ScriptBlockAst commandScope,
399400
FunctionAnalysisContext functionAnalysis)
400401
{
401-
if (commandScope == null || commandScope != GetFunctionScope(function, functionAnalysis))
402+
if (commandScope == null)
402403
{
403404
return false;
404405
}
405406

407+
ScriptBlockAst functionScope = GetFunctionScope(function, functionAnalysis);
408+
if (functionScope == null)
409+
{
410+
return false;
411+
}
412+
413+
CommandAst immediateInvocation = null;
414+
if (commandScope != functionScope
415+
&& !IsImmediatelyInvokedChildScope(commandScope, functionScope, out immediateInvocation))
416+
{
417+
return false;
418+
}
419+
420+
int functionNamedBlockPhase = GetNamedBlockExecutionPhase(function, out FunctionDefinitionAst functionOwner);
421+
int commandNamedBlockPhase = GetNamedBlockExecutionPhase(cmdAst, out FunctionDefinitionAst commandOwner);
422+
if (functionOwner != null
423+
&& functionOwner == commandOwner
424+
&& functionNamedBlockPhase >= 0
425+
&& commandNamedBlockPhase >= 0
426+
&& functionNamedBlockPhase != commandNamedBlockPhase)
427+
{
428+
return functionNamedBlockPhase > commandNamedBlockPhase;
429+
}
430+
406431
int functionPhase = GetForEachObjectExecutionPhase(function, out CommandAst functionCallback);
407432
int commandPhase = GetForEachObjectExecutionPhase(cmdAst, out CommandAst commandCallback);
408433
if (functionCallback == commandCallback
@@ -413,7 +438,82 @@ private bool IsFunctionDeclaredAfterCommand(
413438
return functionPhase > commandPhase;
414439
}
415440

416-
return function.Extent.StartOffset > cmdAst.Extent.StartOffset;
441+
int commandExecutionOffset = immediateInvocation == null
442+
? cmdAst.Extent.StartOffset
443+
: immediateInvocation.Extent.StartOffset;
444+
return function.Extent.StartOffset > commandExecutionOffset;
445+
}
446+
447+
private bool IsImmediatelyInvokedChildScope(
448+
ScriptBlockAst commandScope,
449+
ScriptBlockAst functionScope,
450+
out CommandAst invocation)
451+
{
452+
invocation = null;
453+
if (!(commandScope.Parent is ScriptBlockExpressionAst scriptBlock))
454+
{
455+
return false;
456+
}
457+
458+
CommandAst command = GetScriptBlockCommand(scriptBlock);
459+
if (command == null || command.InvocationOperator != TokenKind.Ampersand)
460+
{
461+
return false;
462+
}
463+
464+
ScriptBlockAst parentScope = null;
465+
for (Ast parent = commandScope.Parent; parent != null; parent = parent.Parent)
466+
{
467+
if (parent is ScriptBlockAst scope)
468+
{
469+
parentScope = scope;
470+
break;
471+
}
472+
}
473+
474+
if (parentScope != functionScope)
475+
{
476+
return false;
477+
}
478+
479+
invocation = command;
480+
return true;
481+
}
482+
483+
private int GetNamedBlockExecutionPhase(Ast ast, out FunctionDefinitionAst containingFunction)
484+
{
485+
NamedBlockAst namedBlock = null;
486+
containingFunction = null;
487+
for (Ast current = ast; current != null; current = current.Parent)
488+
{
489+
if (namedBlock == null && current is NamedBlockAst block)
490+
{
491+
namedBlock = block;
492+
}
493+
494+
if (current != ast && current is FunctionDefinitionAst function)
495+
{
496+
containingFunction = function;
497+
break;
498+
}
499+
}
500+
501+
if (namedBlock == null || containingFunction == null)
502+
{
503+
return -1;
504+
}
505+
506+
switch (namedBlock.BlockKind)
507+
{
508+
case TokenKind.Begin:
509+
return 0;
510+
case TokenKind.Process:
511+
return 1;
512+
case TokenKind.End:
513+
return 2;
514+
default:
515+
return -1;
516+
}
417517
}
418518

419519
private int GetForEachObjectExecutionPhase(Ast ast, out CommandAst callbackCommand)
@@ -611,7 +711,8 @@ private bool IsCallerScopedScriptBlock(
611711
string commandName = GetCallbackCommandName(commandAst);
612712
if (IsForEachObjectCommand(commandName))
613713
{
614-
if (HasCommandParameter(commandAst, "MemberName"))
714+
if (HasCommandParameter(commandAst, "MemberName")
715+
|| HasPositionalCommandParameter(commandAst, scriptBlock, "MemberName"))
615716
{
616717
functionAnalysis.CallerScopedScriptBlockCache[scope] = result;
617718
return result;
@@ -768,7 +869,8 @@ private CommandAst GetScriptBlockCommand(ScriptBlockExpressionAst scriptBlock)
768869
continue;
769870
}
770871

771-
if (current.Parent is CommandExpressionAst
872+
if (current.Parent is ParenExpressionAst
873+
|| current.Parent is CommandExpressionAst
772874
|| current.Parent is PipelineAst
773875
|| current.Parent is StatementBlockAst)
774876
{
@@ -802,6 +904,10 @@ private string GetCallbackCommandName(CommandAst commandAst)
802904
{
803905
commandName = commandName.Substring(coreModulePrefix.Length);
804906
}
907+
else if (commandName.EndsWith("\\Start-ThreadJob", StringComparison.OrdinalIgnoreCase))
908+
{
909+
commandName = "Start-ThreadJob";
910+
}
805911
else if (commandName.IndexOf('\\') >= 0)
806912
{
807913
return null;
@@ -900,6 +1006,73 @@ private string GetScriptBlockParameterName(
9001006
return null;
9011007
}
9021008

1009+
private bool HasPositionalCommandParameter(
1010+
CommandAst commandAst,
1011+
ScriptBlockExpressionAst scriptBlock,
1012+
string parameterName)
1013+
{
1014+
CommandInfo commandInfo = Helper.Instance.GetCommandInfo(GetCallbackCommandName(commandAst));
1015+
Dictionary<string, ParameterMetadata> parameters = commandInfo == null
1016+
? null
1017+
: GetCommandParameters(commandInfo);
1018+
if (parameters == null)
1019+
{
1020+
return false;
1021+
}
1022+
1023+
CommandElementAst argumentElement = commandAst.CommandElements
1024+
.FirstOrDefault(element => element == scriptBlock
1025+
|| IsDescendantOf(scriptBlock, element));
1026+
int scriptBlockIndex = argumentElement == null
1027+
? -1
1028+
: commandAst.CommandElements.IndexOf(argumentElement);
1029+
if (scriptBlockIndex < 0)
1030+
{
1031+
return false;
1032+
}
1033+
1034+
int positionalIndex = 0;
1035+
for (int i = 1; i < scriptBlockIndex; i++)
1036+
{
1037+
CommandElementAst commandElement = commandAst.CommandElements[i];
1038+
if (!(commandElement is CommandParameterAst parameter))
1039+
{
1040+
if (IsPositionalParameter(parameters, positionalIndex, parameterName))
1041+
{
1042+
return true;
1043+
}
1044+
1045+
positionalIndex++;
1046+
continue;
1047+
}
1048+
1049+
ParameterMetadata metadata = ResolveParameter(parameters, parameter.ParameterName);
1050+
if (parameter.Argument != null || metadata?.ParameterType == typeof(SwitchParameter))
1051+
{
1052+
continue;
1053+
}
1054+
1055+
if (i + 1 < scriptBlockIndex
1056+
&& !(commandAst.CommandElements[i + 1] is CommandParameterAst))
1057+
{
1058+
i++;
1059+
}
1060+
}
1061+
1062+
return false;
1063+
}
1064+
1065+
private bool IsPositionalParameter(
1066+
Dictionary<string, ParameterMetadata> parameters,
1067+
int position,
1068+
string parameterName)
1069+
{
1070+
return parameters.Values.Any(parameter =>
1071+
String.Equals(parameter.Name, parameterName, StringComparison.OrdinalIgnoreCase)
1072+
&& parameter.Attributes.OfType<ParameterAttribute>()
1073+
.Any(attribute => attribute.Position == position));
1074+
}
1075+
9031076
private ParameterMetadata ResolveParameter(
9041077
Dictionary<string, ParameterMetadata> parameters,
9051078
string parameterName)

‎Tests/Rules/AvoidUsingAlias.tests.ps1‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,22 @@ gci -Path C:\
108108
$violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb'
109109
}
110110

111+
It "does not resolve declarations after an immediately invoked child block" {
112+
$target = '& { Verb }; function Verb {}'
113+
$violations = @(Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName)
114+
115+
$violations.Count | Should -Be 1
116+
$violations[0].Extent.Text | Should -Be 'Verb'
117+
$violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb'
118+
}
119+
120+
It "respects function begin and process execution order" {
121+
$target = 'function Invoke-Example { process { Verb } begin { function Verb {} } }; Invoke-Example'
122+
$violations = @(Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName)
123+
124+
$violations.Count | Should -Be 0
125+
}
126+
111127
It "respects execution order across ForEach-Object callback phases" -TestCases @(
112128
@{
113129
Target = '1 | ForEach-Object -Process { Verb } -Begin { function Verb {} }'
@@ -307,6 +323,23 @@ Script Test {
307323
Should -Be ($target.Substring(0, $target.LastIndexOf(' Verb')) + ' Get-Verb')
308324
}
309325

326+
It "does not let callback declarations change command resolution at invocation time" -TestCases @(
327+
@{
328+
Target = 'function private:Verb {}; 1 | ForEach-Object { function ForEach-Object { param([scriptblock]$Process) & $Process } }; function Unrelated {}; 1 | ForEach-Object -Process { Verb }'
329+
}
330+
@{
331+
Target = 'function private:Verb {}; 1 | ForEach-Object { function ForEach-Object { param([scriptblock]$Process) & $Process } }; 1 | ForEach-Object -Process { Verb }'
332+
}
333+
) {
334+
param($Target)
335+
336+
$violations = @(Invoke-ScriptAnalyzer -ScriptDefinition $Target -IncludeRule $violationName)
337+
338+
$violations.Count | Should -Be 1
339+
$violations[0].Extent.Text | Should -Be 'Verb'
340+
$violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb'
341+
}
342+
310343
It "respects caller scope for script blocks inside array callback arguments" {
311344
$target = 'function private:Verb {}; 1 | ForEach-Object -Process {}, { Verb }'
312345
$violations = @(Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName)
@@ -394,6 +427,31 @@ Script Test {
394427
$violations.Count | Should -Be 1
395428
$violations[0].Extent.Text | Should -Be 'Verb'
396429
$violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb'
430+
431+
$target = '1 | ForEach-Object Equals { function Verb {} }; Verb'
432+
$violations = @(Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName)
433+
434+
$violations.Count | Should -Be 1
435+
$violations[0].Extent.Text | Should -Be 'Verb'
436+
$violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb'
437+
}
438+
439+
It "respects caller scope for parenthesized literal callbacks" {
440+
$target = 'function private:Verb {}; 1 | ForEach-Object -Process ({ Verb })'
441+
$violations = @(Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName)
442+
443+
$violations.Count | Should -Be 0
444+
$settings = @{ Rules = @{ PSAvoidUsingCmdletAliases = @{ Enable = $true } } }
445+
Invoke-Formatter -ScriptDefinition $target -Settings $settings | Should -Be $target
446+
}
447+
448+
It "does not find functions outside module-qualified thread jobs" -skip:($null -eq (Get-Command Start-ThreadJob -ErrorAction SilentlyContinue)) {
449+
$target = 'function Verb {}; ThreadJob\Start-ThreadJob -ScriptBlock { Verb }'
450+
$violations = @(Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName)
451+
452+
$violations.Count | Should -Be 1
453+
$violations[0].Extent.Text | Should -Be 'Verb'
454+
$violations[0].SuggestedCorrections[0].Text | Should -Be 'Get-Verb'
397455
}
398456

399457
It "does not treat an explicitly disabled NoNewScope switch as caller-scoped" {

0 commit comments

Comments
 (0)