Trigger AvoidPositionalParameters rule for function defined and called inside a script. #963

kalgiz · 2018-04-06T18:26:05Z

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 x between the square brackets. Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing documentation needed
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

bergmeister · 2018-04-06T20:31:52Z

Thanks for contributing. 😃
First of all as a tip, don't be put off by the many test failures and try to focus only on making the tests of this rule pass. There are unfortunately known issues with this nasty Helper class (static members, etc.), hence why GetCommandInfoLegacy exists but I think it is OK in your case to move it forward to use the proper, new GetCommandInfo method. The reason why not all old method calls to this legacy method were fixed is because it was found that it can have weird side effects and cause other unrelated tests to fail and therefore it was decided to rather not touch it and declare it legacy for the moment being. @JamesWTruher knows about this and is trying hard in PR #953 to improve the situation (we believe there is also a problem with the current tests suite whereby even although AppVeyor reports success for the new PowerShell Core builds on Windows and Ubuntu, but there are 9 tests that fail on my machine and similar for James's machine, see PR #961 for more weirdness in AppVeyor).
Therefore as part of the PR please check and report at the end if you have more/different local test failures than in development (on Windows PowerShell 5.1, there are no failures at least, WMF4 has 1, WMF 3 has 9).
P.S. If you put special keywords in front of the issue that a PR addresses, like e.g. closes or fixes, then the issue will get closed automatically when the PR gets merged. See here for details.
Also, we have a scriptanalyzer channel on Slack: https://powershell.slack.com

bergmeister · 2018-04-06T20:42:00Z

Rules/AvoidPositionalParameters.cs

@@ -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");


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.

I didn't know that. Thanks.
I removed debug line.

kalgiz · 2018-04-06T20:54:22Z

Engine/Helper.cs

            if (HasSplattedVariable(cmdAst))
            {
                return false;
            }

-            if (commandInfo != null && commandInfo.CommandType == System.Management.Automation.CommandTypes.Cmdlet)


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 · 2018-04-10T10:00:53Z

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"

kalgiz · 2018-04-11T00:08:01Z

Counting positional parameters should be fixed.

LaurentDardenne · 2018-04-11T15:44:02Z

@kalgiz Thank you.
Should this case be treated?

 $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"

bergmeister · 2018-04-11T15:58:18Z

@kalgiz Thank you for your efforts. If the PR is ready for review and test, please remove WIP from the title (I will pull then pull your branch and run it against Windows PowerShell 3/4 and PowerShell Core on Ubuntu in my VSTS environment due to known issues with PowerShell Core tests in AppVeyor and missing WMF3 images in order to check against possible regressions)

bergmeister

Overall, looks very good! Only some minor comments. All tests showed no regression. For the record, the result were:

Windows PowerShell 3 (WinServer 2012): 10 (known) test failures
Windows PowerShell 4 (WinServer 2012R2): 0 test failures
Windows PowerShell 5.1 (WinServer 2016 and Win10): 0 test failures
PowerShell Core 6.0.2 (Windows 10): 9 (known) test failures
PowerShell Core 6.0.2 (Hosted Linux agent on VSTS (Ubuntu 16)): 9 (known) test failures

bergmeister · 2018-04-11T19:27:18Z

Engine/Helper.cs

            // 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;


Are argumentsWithoutPositionalParameters and wasPositionalParameter more descriptive? (optional comment)

I changed names for more descriptive: argumentsWithoutProcedingParameters, parameterPreceding

bergmeister · 2018-04-11T19:28:18Z

Engine/Helper.cs

-                        continue;
+                    wasParameter = true;
+                } else {
+                    if (!wasParameter) {


Please use consistent bracing style of the surrounding code. For C# code, braces should be on their own line.

bergmeister · 2018-04-11T19:36:57Z

Rules/AvoidPositionalParameters.cs

+
+            foreach (Ast foundAst in functionDefinitionAsts) {
+                FunctionDefinitionAst functionDefinitionAst = (FunctionDefinitionAst)foundAst;
+                if (string.IsNullOrEmpty(functionDefinitionAst.Name)) {


brace style as per comment above

bergmeister · 2018-04-11T19:37:59Z

Rules/AvoidPositionalParameters.cs

+            HashSet<String> declaredFunctionNames = new HashSet<String>();
+
+            foreach (Ast foundAst in functionDefinitionAsts) {
+                FunctionDefinitionAst functionDefinitionAst = (FunctionDefinitionAst)foundAst;


I think you can inline the cast in the foreach loop: foreach (FunctionDefinitionAst functionDefinitionAst in functionDefinitionAsts)

bergmeister · 2018-04-11T19:45:20Z

Rules/UseCmdletCorrectly.cs

@@ -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)))


!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.

Sure, fixed.

bergmeister · 2018-04-11T19:48:23Z

Tests/Rules/AvoidPositionalParameters.tests.ps1

+            }
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "$sb"
+            $warnings.Count | Should -BeGreaterThan 0
+            $warnings.RuleName | Should -Contain "PSAvoidUsingPositionalParameters"


the variable $violationName = "PSAvoidUsingPositionalParameters" defined at the top should be re-used

bergmeister · 2018-04-11T19:48:58Z

Tests/Rules/AvoidPositionalParameters.tests.ps1

+                Foo "a" "b" "c"
+            }
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "$sb"
+            $warnings.Count | Should -BeGreaterThan 0


why not -Be 1?

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?

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).

I got rid of AvoidTrailingWhitespaces trigger and of testing error message.

bergmeister · 2018-04-11T19:51:52Z

Engine/Helper.cs

+        /// <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)


Shouldn't this be moreThanTwoPositionalParameters since the rule is >=3?

bergmeister

Looks good to me. Thanks!
But please do not rebase during a PR. When the PR gets merged, it gets squashed anyway. It is OK to get upstream changes to avoid future merge conflicts but please use a git pull instead for that because as you can see the diff on GitHub gets confused by that. P.S. When the PR builds run, they run on a PR branch to emulate the behaviour as if your branch was merged into development (to detect possible integration issues if the branch is behind).

kalgiz · 2018-04-13T20:34:51Z

Ok, sorry for that, I wanted to make git history cleaner.

JamesWTruher · 2018-05-16T23:36:46Z

Engine/Helper.cs

            // 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)


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

…d inside a sript given as argument to Script Analyzer.

bergmeister · 2019-03-14T18:04:17Z

It seems this PR introduced a regression for a test case that is not in the test suite, which is that Invoke-ScriptAnalyzer -ScriptDefinition "gcm 'abc' 4 4.3" should return not only the expected PSAvoidUsingCmdletAliases but also a PSAvoidUsingPositionalParameters warning. Since ``Invoke-ScriptAnalyzer -ScriptDefinition "get-command 'abc' 4 4.3"triggers aPSAvoidUsingPositionalParameters` warning, the alias lookup seems to be not invoked or broken

…ereby PSAvoidUsingPositionalParameters was not taking aliases into account any more (#1175) * Allow aliases or external script definitions as well so that positionalparameters triggers on them as well * add test

…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

kalgiz changed the title ~~Trigger AvoidPositionalParameters rule for function defined and called inside a script.~~ WIP - Trigger AvoidPositionalParameters rule for function defined and called inside a script. Apr 6, 2018

bergmeister added the Area - Rules label Apr 6, 2018

bergmeister assigned kalgiz Apr 6, 2018

bergmeister reviewed Apr 6, 2018

View reviewed changes

kalgiz commented Apr 6, 2018

View reviewed changes

kalgiz changed the title ~~WIP - Trigger AvoidPositionalParameters rule for function defined and called inside a script.~~ Trigger AvoidPositionalParameters rule for function defined and called inside a script. Apr 11, 2018

bergmeister reviewed Apr 11, 2018

View reviewed changes

bergmeister approved these changes Apr 13, 2018

View reviewed changes

SteveL-MSFT force-pushed the positional-parameter-bug-fix branch from 8775306 to 717bccd Compare April 23, 2018 17:16

kalgiz force-pushed the positional-parameter-bug-fix branch from 717bccd to 8775306 Compare April 23, 2018 20:23

JamesWTruher reviewed May 16, 2018

View reviewed changes

kalgiz added 10 commits May 17, 2018 10:24

Trigger AvoidPositionalParameters rule for function defined and calle…

7f04508

…d inside a sript given as argument to Script Analyzer.

Get rid of debug print.

b3c577e

Tests fpr PSAvoidPOsitionalParameters rule.

365b516

Tests for AvoidPositionalParameters rule fixes.

5f0f90d

Counting positional parameters fix.

1a2f9fe

Code cleanup for AvoidPositionalParameter rule fix.

29f5017

Trigger AvoidPositionalParameters rule for function defined and calle…

956fcde

…d inside a sript given as argument to Script Analyzer.

Tests fpr PSAvoidPOsitionalParameters rule.

014dd71

Counting positional parameters fix.

147b16f

Tests fixes for AvoidPositionalParameters rule.

b60f00d

kalgiz force-pushed the positional-parameter-bug-fix branch 2 times, most recently from 401b195 to d760aae Compare May 17, 2018 21:35

Code cleanup

479fbc4

kalgiz force-pushed the positional-parameter-bug-fix branch from d760aae to 479fbc4 Compare May 17, 2018 21:40

JamesWTruher approved these changes Aug 20, 2018

View reviewed changes

bergmeister merged commit 1c4b704 into PowerShell:development Aug 21, 2018

bergmeister mentioned this pull request Mar 14, 2019

Fix regression caused by PR #963 during development of PSSA 1.18.0 whereby PSAvoidUsingPositionalParameters was not taking aliases into account any more #1175

Merged

6 tasks

bergmeister mentioned this pull request Jan 18, 2021

PSUseCmdletCorrectly breaks on Write-Verbose -Verbose #1624

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger AvoidPositionalParameters rule for function defined and called inside a script. #963

Trigger AvoidPositionalParameters rule for function defined and called inside a script. #963

kalgiz commented Apr 6, 2018 •

edited

Loading

bergmeister commented Apr 6, 2018 •

edited

Loading

bergmeister Apr 6, 2018 •

edited

Loading

kalgiz Apr 6, 2018

kalgiz Apr 6, 2018

LaurentDardenne commented Apr 10, 2018

kalgiz commented Apr 11, 2018

LaurentDardenne commented Apr 11, 2018

bergmeister commented Apr 11, 2018 •

edited

Loading

bergmeister left a comment

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister Apr 12, 2018 •

edited

Loading

kalgiz Apr 12, 2018

bergmeister Apr 11, 2018

kalgiz Apr 11, 2018

bergmeister left a comment •

edited

Loading

kalgiz commented Apr 13, 2018

JamesWTruher May 16, 2018

kalgiz May 17, 2018

bergmeister commented Mar 14, 2019

Trigger AvoidPositionalParameters rule for function defined and called inside a script. #963

Trigger AvoidPositionalParameters rule for function defined and called inside a script. #963

Conversation

kalgiz commented Apr 6, 2018 • edited Loading

PR Summary

PR Checklist

bergmeister commented Apr 6, 2018 • edited Loading

bergmeister Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentDardenne commented Apr 10, 2018

kalgiz commented Apr 11, 2018

LaurentDardenne commented Apr 11, 2018

bergmeister commented Apr 11, 2018 • edited Loading

bergmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

kalgiz commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister commented Mar 14, 2019

kalgiz commented Apr 6, 2018 •

edited

Loading

bergmeister commented Apr 6, 2018 •

edited

Loading

bergmeister Apr 6, 2018 •

edited

Loading

bergmeister commented Apr 11, 2018 •

edited

Loading

bergmeister Apr 12, 2018 •

edited

Loading

bergmeister left a comment •

edited

Loading