Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upTrigger AvoidPositionalParameters rule for function defined and called inside a script. #963
Conversation
|
Thanks for contributing. |
| @@ -54,7 +54,6 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) | |||
| if ((Helper.Instance.IsCmdlet(cmdAst) || declaredFunctionNames.Contains(cmdAst.GetCommandName())) && | |||
| (Helper.Instance.PositionalParameterUsed(cmdAst))) | |||
| { | |||
| Console.WriteLine("Cmdlet or function call"); | |||
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 6, 2018
•
Collaborator
Do you know that you can just attach the debugger to it and step through it by attaching to the PowerShell process? In Debug, the cmdlet even has an -AttachAndDebug switch to automatically attach to Visual Studio.
This comment has been minimized.
This comment has been minimized.
| if (HasSplattedVariable(cmdAst)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (commandInfo != null && commandInfo.CommandType == System.Management.Automation.CommandTypes.Cmdlet) |
This comment has been minimized.
This comment has been minimized.
kalgiz
Apr 6, 2018
Author
Contributor
As in this function we probably should care only about the number of arguments which have no corresponding parameters, analyzing switch parameters seems unnecessary in here...
LaurentDardenne
commented
Apr 10, 2018
|
this code triggers the rule : $sb={
Function Get-MyCommand {
param(
$A
)
"Test"
}
Get-MyCommand -A 'Get-ChildItem'
Get-MyCommand 'Get-ChildItem'
}
Invoke-ScriptAnalyzer -ScriptDefinition "$sb"this code too : $sb={
Function Get-MyCommand {
param(
[Parameter(Mandatory=$true,Position=1)]
$A,
[Parameter(Position=2)]
$B,
[Parameter(Position=3)]
$C
)
"Test"
}
Get-MyCommand -A 'Get-ChildItem' -B 'Microsoft.PowerShell.Management' -C 'System.Management.Automation.Cmdlet'
}
Invoke-ScriptAnalyzer -ScriptDefinition "$sb" |
|
Counting positional parameters should be fixed. |
LaurentDardenne
commented
Apr 11, 2018
|
@kalgiz Thank you. $sb={
Function Get-MyCommand {
param(
[Parameter(Mandatory=$true,Position=1)]
$A,
[Parameter(Position=2)]
$B,
[Parameter(Position=3)]
$C
)
"Test"
}
Get-MyCommand P1 P2 P3 #OK -> Error
Get-MyCommand -A P1 P2 P3 #OK -> No error
'Test' | Get-MyCommand -A P1 P2 P3 #OK -> No error
'Test' | Get-MyCommand P1 P2 P3 #NOK ?? -> No error
}
Invoke-ScriptAnalyzer -ScriptDefinition "$sb" |
|
@kalgiz Thank you for your efforts. If the PR is ready for review and test, please remove |
|
Overall, looks very good! Only some minor comments. All tests showed no regression. For the record, the result were:
|
| // Because of the way we count, we will also count the cmdlet as an argument so we have to -1 | ||
| int arguments = -1; | ||
| int argumentsWithoutParameters = -1; | ||
| bool wasParameter = false; |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 11, 2018
Collaborator
Are argumentsWithoutPositionalParameters and wasPositionalParameter more descriptive? (optional comment)
This comment has been minimized.
This comment has been minimized.
kalgiz
Apr 11, 2018
Author
Contributor
I changed names for more descriptive: argumentsWithoutProcedingParameters, parameterPreceding
| continue; | ||
| wasParameter = true; | ||
| } else { | ||
| if (!wasParameter) { |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 11, 2018
Collaborator
Please use consistent bracing style of the surrounding code. For C# code, braces should be on their own line.
This comment has been minimized.
This comment has been minimized.
|
|
||
| foreach (Ast foundAst in functionDefinitionAsts) { | ||
| FunctionDefinitionAst functionDefinitionAst = (FunctionDefinitionAst)foundAst; | ||
| if (string.IsNullOrEmpty(functionDefinitionAst.Name)) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| HashSet<String> declaredFunctionNames = new HashSet<String>(); | ||
|
|
||
| foreach (Ast foundAst in functionDefinitionAsts) { | ||
| FunctionDefinitionAst functionDefinitionAst = (FunctionDefinitionAst)foundAst; |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 11, 2018
Collaborator
I think you can inline the cast in the foreach loop: foreach (FunctionDefinitionAst functionDefinitionAst in functionDefinitionAsts)
This comment has been minimized.
This comment has been minimized.
| @@ -129,7 +129,7 @@ private bool MandatoryParameterExists(CommandAst cmdAst) | |||
| return true; | |||
| } | |||
|
|
|||
| if (mandParams.Count() == 0 || Helper.Instance.PositionalParameterUsed(cmdAst)) | |||
| if (mandParams.Count() == 0 || (Helper.Instance.IsCmdlet(cmdAst) && Helper.Instance.PositionalParameterUsed(cmdAst))) | |||
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 11, 2018
Collaborator
!mandParams.Any() performs in general better than mandParams.Count() == 0 for IEnumerable types because the former stops on the first element but the latter enumerates over all elements. But in this case it is a List, therefore we can even just use the property mandParams.Count == 0, which does not need to enumerate at all. I know, this code was already like that before, I am not criticising you but we should take the opportunity and make this faster for free.
This comment has been minimized.
This comment has been minimized.
| } | ||
| $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "$sb" | ||
| $warnings.Count | Should -BeGreaterThan 0 | ||
| $warnings.RuleName | Should -Contain "PSAvoidUsingPositionalParameters" |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 11, 2018
Collaborator
the variable $violationName = "PSAvoidUsingPositionalParameters" defined at the top should be re-used
This comment has been minimized.
This comment has been minimized.
| Foo "a" "b" "c" | ||
| } | ||
| $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "$sb" | ||
| $warnings.Count | Should -BeGreaterThan 0 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
kalgiz
Apr 11, 2018
Author
Contributor
The problem is that the other violation appears PSAvoidTrailingWhitespace if I write the script like this:
<$sb=
{
Function Foo {
param(
[Parameter(Mandatory=$true,Position=1)] $A,
[Parameter(Position=2)]$B,
[Parameter(Position=3)]$C)
}
Foo "a" "b" "c"
} >
The warning doesn't show up when the script looks like:
<$sb=
{
Function Foo {
param(
[Parameter(Mandatory=$true,Position=1)] $A,
[Parameter(Position=2)]$B,
[Parameter(Position=3)]$C)
}
Foo "a" "b" "c"}>
But I don't like its readability.
I am not sure if PSAvoidTrailingWhitespace is an expected behaviour in here?
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 12, 2018
•
Collaborator
The trailing whitespace is because of the indentation of the last brace, if you were to put the brace to the very left the rule would start to appear. Therefore this is by design and expected because it would be the same as if the last line in a script had space characters. Asserting against the exact number is more important than minor optical issues. I would be ok to just call Invoke-ScriptAnalyzer with -ExcludeRule PSAvoidTrailingWhitespace instead as an alternative. You should then also use the -Be operators for the other assertions. I proposed in older PRs to filter out only the relevant rules using Invoke-ScriptAnalyzer -ScriptDefintion $scriptDefinition | Where-Object { $_.RuleName -eq 'PSAvoidUsingPositionalParameters' } but Jim did not like this idea either (and he is also not in favour of testing the error message itself).
This comment has been minimized.
This comment has been minimized.
kalgiz
Apr 12, 2018
Author
Contributor
I got rid of AvoidTrailingWhitespaces trigger and of testing error message.
| /// <param name="cmdAst"></param> | ||
| /// <param name="moreThanThreePositional">only return true if more than three positional parameters are used</param> | ||
| /// <returns></returns> | ||
| public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositional = false) |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 11, 2018
Collaborator
Shouldn't this be moreThanTwoPositionalParameters since the rule is >=3?
This comment has been minimized.
This comment has been minimized.
|
Looks good to me. Thanks! |
|
Ok, sorry for that, I wanted to make git history cleaner. |
8775306
to
717bccd
717bccd
to
8775306
| // Because of the way we count, we will also count the cmdlet as an argument so we have to -1 | ||
| int arguments = -1; | ||
| int argumentsWithoutProcedingParameters = -1; | ||
| bool parameterPreceding = false; | ||
|
|
||
| foreach (CommandElementAst ceAst in cmdAst.CommandElements) |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
May 16, 2018
Member
I think this can all be written more simply:
var commandElementCollection = cmdAst.CommandElements;
if ( commandElementCollection.Count = 1 ) { return false; }
for(int i = 1; i < commandElementCollection.Length; i++) {
if ( commandElementCollection[i] isnot CommandParameterAst && commandElementCollection[i-1] isnot CommandParameterAst ) {
argumentsWithoutProcedingParameters++;
}
}
this should catch things like test-cmd a -a -b c,f d,z $a $b -q $() -e -f @() @{ one = 1 } -z "this is a test" "this is a test"
it doesn't validate that there may be missing parameter arguments, but it should catch any positional parameters
This comment has been minimized.
This comment has been minimized.
401b195
to
d760aae
d760aae
to
479fbc4
|
It seems this PR introduced a regression for a test case that is not in the test suite, which is that |
…d inside a script. (PowerShell#963) * Trigger AvoidPositionalParameters rule for function defined and called inside a sript given as argument to Script Analyzer. * Get rid of debug print. * Tests fpr PSAvoidPOsitionalParameters rule. * Tests for AvoidPositionalParameters rule fixes. * Counting positional parameters fix. * Code cleanup for AvoidPositionalParameter rule fix. * Trigger AvoidPositionalParameters rule for function defined and called inside a sript given as argument to Script Analyzer. * Tests fpr PSAvoidPOsitionalParameters rule. * Counting positional parameters fix. * Tests fixes for AvoidPositionalParameters rule. * Code cleanup
… 1.18.0 whereby PSAvoidUsingPositionalParameters was not taking aliases into account any more (PowerShell#1175) * Allow aliases or external script definitions as well so that positionalparameters triggers on them as well * add test

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

kalgiz commentedApr 6, 2018
•
edited
PR Summary
Trigger AvoidPositionalParameters rule for function defined and called inside a script.
fixes: #893
The rule doesn't trigger for the function defined and called in the script, because the script is not run and the function definition is not added to the powershell session runspace. Hence, the called function is not recognized as Cmdlet.
For the temporary solution I pull the function definitions from the script, store them and search these function calls for positional parameters.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready