X Tutup
The Wayback Machine - https://web.archive.org/web/20200918094650/https://github.com/PowerShell/PSScriptAnalyzer/pull/963
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

@kalgiz
Copy link
Contributor

kalgiz commented Apr 6, 2018

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
@kalgiz 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
Copy link
Collaborator

bergmeister commented Apr 6, 2018

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

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

@bergmeister

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.

@kalgiz

kalgiz Apr 6, 2018 Author Contributor

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

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

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

This comment has been minimized.

@kalgiz

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
Copy link

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"
@kalgiz
Copy link
Contributor Author

kalgiz commented Apr 11, 2018

Counting positional parameters should be fixed.

@LaurentDardenne
Copy link

LaurentDardenne commented Apr 11, 2018

@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
Copy link
Collaborator

bergmeister commented Apr 11, 2018

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

@kalgiz 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
Copy link
Collaborator

bergmeister left a comment

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

@bergmeister

bergmeister Apr 11, 2018 Collaborator

Are argumentsWithoutPositionalParameters and wasPositionalParameter more descriptive? (optional comment)

This comment has been minimized.

@kalgiz

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.

@bergmeister

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.

@kalgiz

kalgiz Apr 11, 2018 Author Contributor

Fixed.


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

This comment has been minimized.

@bergmeister

bergmeister Apr 11, 2018 Collaborator

brace style as per comment above

This comment has been minimized.

@kalgiz

kalgiz Apr 11, 2018 Author Contributor

Fixed.

HashSet<String> declaredFunctionNames = new HashSet<String>();

foreach (Ast foundAst in functionDefinitionAsts) {
FunctionDefinitionAst functionDefinitionAst = (FunctionDefinitionAst)foundAst;

This comment has been minimized.

@bergmeister

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.

@kalgiz

kalgiz Apr 11, 2018 Author Contributor

Fixed.

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

@bergmeister

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.

@kalgiz

kalgiz Apr 11, 2018 Author Contributor

Sure, fixed.

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

This comment has been minimized.

@bergmeister

bergmeister Apr 11, 2018 Collaborator

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

This comment has been minimized.

@kalgiz

kalgiz Apr 11, 2018 Author Contributor

Fixed.

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

This comment has been minimized.

@bergmeister

bergmeister Apr 11, 2018 Collaborator

why not -Be 1?

This comment has been minimized.

@kalgiz

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.

@bergmeister

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.

@kalgiz

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.

@bergmeister

bergmeister Apr 11, 2018 Collaborator

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

This comment has been minimized.

@kalgiz

kalgiz Apr 11, 2018 Author Contributor

Fixed.

Copy link
Collaborator

bergmeister left a comment

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
Copy link
Contributor Author

kalgiz commented Apr 13, 2018

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

@SteveL-MSFT SteveL-MSFT force-pushed the kalgiz:positional-parameter-bug-fix branch from 8775306 to 717bccd Apr 23, 2018
@kalgiz kalgiz force-pushed the kalgiz:positional-parameter-bug-fix branch from 717bccd to 8775306 Apr 23, 2018
// 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.

@JamesWTruher

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.

@kalgiz

kalgiz May 17, 2018 Author Contributor

Fixed.

@kalgiz kalgiz force-pushed the kalgiz:positional-parameter-bug-fix branch 2 times, most recently from 401b195 to d760aae May 17, 2018
@kalgiz kalgiz force-pushed the kalgiz:positional-parameter-bug-fix branch from d760aae to 479fbc4 May 17, 2018
@bergmeister bergmeister merged commit 1c4b704 into PowerShell:development Aug 21, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@bergmeister
Copy link
Collaborator

bergmeister commented Mar 14, 2019

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

bergmeister added a commit that referenced this pull request Mar 15, 2019
…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
bergmeister added a commit to bergmeister/PSScriptAnalyzer that referenced this pull request Mar 22, 2019
…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
bergmeister added a commit to bergmeister/PSScriptAnalyzer that referenced this pull request Mar 22, 2019
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.
X Tutup