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

Invoke-ScriptAnalyzer throw a NullReferenceException #602

Closed
LaurentDardenne opened this issue Aug 22, 2016 · 16 comments
Closed

Invoke-ScriptAnalyzer throw a NullReferenceException #602

LaurentDardenne opened this issue Aug 22, 2016 · 16 comments
Assignees
Milestone

Comments

@LaurentDardenne
Copy link

This code return a string type instead a DiagnosticRecord instance:

$ModuleFullPath='C:\temp\TestRule.psm1'

@'
Function RuleOne{         
 [CmdletBinding()]
 [OutputType([Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord[]])]

 Param(
       [Parameter(Mandatory = $true)]
       [ValidateNotNullOrEmpty()]
       [System.Management.Automation.Language.FunctionDefinitionAst]
      $FunctionDefinitionAst
 )

  process {
   try {         
    $Result=New-object System.Collections.Arraylist
    $FunctionName=$FunctionDefinitionAst.Name
    $Result.Add('[String]') > $null
#Other error case    
#     $DR = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord]@{"Message"  = $Messages.MeasureRequiresRunAsAdministrator; 
#                                                 "Extent"   = $assignmentAst.Extent;
#                                                 "RuleName" = $PSCmdlet.MyInvocation.InvocationName;
#                                                 "Severity" = "Information"}
#     $Result.Add($DR)
    return $result
   }
   catch
   {
      $ER= New-Object -Typename System.Management.Automation.ErrorRecord -Argumentlist $_.Exception, 
                                                                             "TestRule-$FunctionName", 
                                                                             "NotSpecified",
                                                                             $FunctionDefinitionAst
      $PSCmdlet.ThrowTerminatingError($ER) 
   }          
  }#process
}#

Export-ModuleMember -Function RuleOne
'@ > $ModuleFullPath 

$E=$T=$null
$Ast=[System.Management.Automation.Language.Parser]::ParseFile(
        $ModuleFullPath,
        [ref]$T,
        [ref]$E
      )

RuleOne $ast.Endblock.Statements[0]  
#RuleOne 
Invoke-ScriptAnalyzer -Path $ModuleFullPath -CustomRulePath  $ModuleFullPath   
#Invoke-ScriptAnalyzer : La référence d'objet n'est pas définie à une instance d'un objet.
@kapilmb
Copy link

kapilmb commented Aug 22, 2016

The code returns string type because -

  • A string is being added to the array that is returned.
  • Declaring outputtype does not convert the output to the desired output type (https://technet.microsoft.com/en-us/library/hh847785.aspx.). The author needs to ensure that the return type and output type are identical. We also have a rule to checks this - PSuseOutputTypeCorrectly

For the other error, where you uncomment $DR assignment and comment out $Result.Add('[String]') > $null, you need to check if $FunctionDefinitionAst is of type FunctionDefinitionAst. ScriptAnalyzer engine gives input of type Ast to the RuleOne function. The function needs to cast to appropriate *Ast type.

Function RuleOne{         
 [CmdletBinding()]
 [OutputType([Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord[]])]
 Param(
       [Parameter(Mandatory = $true)]
       [ValidateNotNullOrEmpty()]
       [System.Management.Automation.Language.Ast]
      $ast
 )

  process {
   try {
    $Result=New-object System.Collections.Arraylist
    $FunctionDefinitionAst = $ast -as [System.Management.Automation.Language.FunctionDefinitionAst]
    if ($FunctionDefinitionAst -eq $null)
    {        
        $Result
    }           
    $FunctionName=$FunctionDefinitionAst.Name
    $DR = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord]@{"Message"  = $Messages.MeasureRequiresRunAsAdministrator; 
                                                 "Extent"   = $assignmentAst.Extent;
                                                 "RuleName" = $PSCmdlet.MyInvocation.InvocationName;
                                                 "Severity" = "Information"}
     $Result.Add($DR)
     $Result
   }
   catch
   {
      $ER= New-Object -Typename System.Management.Automation.ErrorRecord -Argumentlist $_.Exception, 
                                                                             "TestRule-$FunctionName", 
                                                                             "NotSpecified",
                                                                             $FunctionDefinitionAst
      $PSCmdlet.ThrowTerminatingError($ER) 
   }          
  }#process
}#

@LaurentDardenne
Copy link
Author

The author needs to ensure that the return type and output type are identical.

I agree.

I reformulate :
When a custom rule emit a type other than a DiagnosticRecord instance, PSScripanalyzer throw a exception.
In this case why not clearly provide the reason ?
By example : "The rule 'xxx' must emit a DiagnosticRecord instance."

ScriptAnalyzer engine gives input of type Ast to the RuleOne function. The function needs to cast to appropriate *Ast type.

Ah :-/
It is not indicate inside this file.
And in CommunityAnalyzerRules module no rules applies this cast.
I thought the custom rule received a single type of ast that indicated in the parameter.

@kapilmb
Copy link

kapilmb commented Aug 23, 2016

In this case why not clearly provide the reason ?

Yes, you are right. There is clearly a room for improvement here. Will create an issue to track this work item.

I thought the custom rule received a single type of ast that indicated in the parameter.

Correct. The custom rule is given an input of type Ast. Instead of casting it to any *Ast type, which was a bad example that I gave, rules typically use the Ast.Find or Ast.FindAll methods to access the desired node in the AST. If you set your input parameter to FunctionDefinitionAst type or any other *Ast type, it causes downcasting from Ast to FunctionDefinitionAst and it is not guaranteed to succeed unless the given Ast is of type FunctionDefinitionAst.

@LaurentDardenne
Copy link
Author

LaurentDardenne commented Aug 23, 2016

There is clearly a room for improvement here. Will create an issue to track this work item.

In this case, here are anothers examples.
I thinks that the exception management of PSScriptAnalyzer should be most user friendly.

$ModuleFullPath='C:\temp\TestRule.psm1'
@'
Function RuleOne{         
 [CmdletBinding()]
 [OutputType([Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord[]])]

 Param(
       [Parameter(Mandatory = $true)]
       [ValidateNotNullOrEmpty()]
       [System.Management.Automation.Language.FunctionDefinitionAst]
      $FunctionDefinition #there is no respect for the naming convention (ending with 'ast' or 'token')
 )

  process {             
   try {         
    $Result=New-object System.Collections.Arraylist
    $DR = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord]@{"Message"  = 'RuleOne'; 
                                                 "Extent"   = $FunctionDefinitionast.Extent;
                                                 "RuleName" = $PSCmdlet.MyInvocation.InvocationName;
                                                 "Severity" = "Information"}
    $Result.Add($DR) >$null
    $result        

   }
   catch
   {
      $ER= New-Object -Typename System.Management.Automation.ErrorRecord -Argumentlist $_.Exception, 
                                                                             "Test use case-$FunctionDefinitionAst.Name", 
                                                                             "NotSpecified",
                                                                             $FunctionDefinitionAst
      $PSCmdlet.ThrowTerminatingError($ER) 
   }          
  }#process
}#RuleOne

Export-ModuleMember -Function RuleOne
'@ > $ModuleFullPath 
Invoke-ScriptAnalyzer -Path $ModuleFullPath -CustomRulePath  $ModuleFullPath
# Invoke-ScriptAnalyzer : Une exception de type 'System.Exception' a été levée.
# At line:1 char:1
# + Invoke-ScriptAnalyzer -Path $ModuleFullPath -CustomRulePath  $ModuleF ...
# + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#     + CategoryInfo          : ResourceExists: (Microsoft.Windo....ScriptAnalyzer:ScriptAnalyzer) [Invoke-ScriptAnalyzer], Exception
#     + FullyQualifiedErrorId : Cannot find ScriptAnalyzer rules in the specified path,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

The naming convention (ending with 'ast' or 'token') must be explicit in the documentation.

IHMO, the text of the exception is to fuzzy.
CategoryInfo is not correct, because ResourceExists is for "An error that occurs when a resource already exists."
I thinks that 'ObjectNotFound' or 'InvalidData' is preferable.

FullyQualifiedErrorId contains the reason of the error, when i use the 'ErrorView' variable the result is for me useless :

$ErrorView='CategoryView'
Invoke-ScriptAnalyzer -Path $ModuleFullPath -CustomRulePath  $ModuleFullPath
ResourceExists: (Microsoft.Windo....ScriptAnalyzer:ScriptAnalyzer) [Invoke-ScriptAnalyzer], Exception

A example with Get-Childitem :

dir z:\
ObjectNotFound: (z:String) [Get-ChildItem], DriveNotFoundException
$ErrorView='normalView'
dir z:\
dir : Cannot find drive. A drive with the name 'z' does not exist.
At line:1 char:1
+ dir z:\
+ ~~~~~~~
    + CategoryInfo          : ObjectNotFound: (z:String) [Get-ChildItem], DriveNotFoundException
    + FullyQualifiedErrorId : DriveNotFound,Microsoft.PowerShell.Commands.GetChildItemCommand

An another example :

$ModuleFullPath='C:\temp\TestRule.psm1'
@'
Function RuleOne{         
 [CmdletBinding()]
 [OutputType([Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord[]])]

 Param(
       [Parameter(Mandatory = $true)]
       [ValidateNotNullOrEmpty()]
       [System.Management.Automation.Language.FunctionDefinitionAst]
      $FunctionDefinitionast
 )

  process  # !!!  no '{'             
   try {         
    $Result=New-object System.Collections.Arraylist
    $DR = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord]@{"Message"  = 'RuleOne'; 
                                                 "Extent"   = $FunctionDefinitionast.Extent;
                                                 "RuleName" = $PSCmdlet.MyInvocation.InvocationName;
                                                 "Severity" = "Information"}
    $Result.Add($DR) >$null
    $result        
   }
   catch
   {
      $ER= New-Object -Typename System.Management.Automation.ErrorRecord -Argumentlist $_.Exception, 
                                                                             "Test use case-$FunctionDefinitionAst.Name", 
                                                                             "NotSpecified",
                                                                             $FunctionDefinitionAst
      $PSCmdlet.ThrowTerminatingError($ER) 
   }          
  }#process
}#

Export-ModuleMember -Function RuleOne
'@ > $ModuleFullPath 
Invoke-ScriptAnalyzer -Path $ModuleFullPath -CustomRulePath  $ModuleFullPath
WARNING: Cannot find rule extension 'C:\temp\TestRule.psm1'.
Invoke-ScriptAnalyzer : Une exception de type 'System.Exception' a été levée.

In this case i get a warning but the principal is that i dont know what happened here.
The exception on syntax error in the module is not propagated.

ipmo $ModuleFullPath -EV E -EA 'SilentlyContinue'
$E

@kapilmb
Copy link

kapilmb commented Aug 23, 2016

@LaurentDardenne Thanks for pointing out the limitations. I will leave this issue open till we address the issues reported here. Please feel free to post any other exceptions that you find needs improvement.

@LaurentDardenne
Copy link
Author

In this following case

Function TestParameterSet{
 [CmdletBinding(DefaultParameterSetName='F2')]       
  Param (
    [Parameter(Position=2")]
   [string] $A
   )
   Write-Host"ParameterSetName =$($PsCmdlet.ParameterSetName)"
}

The error message is correct :

$ErrorView='NormalView'
Invoke-ScriptAnalyzer -Path 'C:\temp\Rule 4.ps1'

# Invoke-ScriptAnalyzer : Parse error in file C:\temp\Rule 4.ps1:  Missing statement after '=' in named argument at line
# 4 column 25.
# At line:1 char:1
# + Invoke-ScriptAnalyzer -Path 'C:\temp\Rule 4.ps1'
# + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#     + CategoryInfo          : ParserError: (MissingExpressionInNamedArgument:String) [Invoke-ScriptAnalyzer], ParseException
#     + FullyQualifiedErrorId : Parse error in file C:\temp\Rule 4.ps1:  Missing statement after '=' in named argument at line 
#                               4 column 25.,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

But we have 7 errors :

$ErrorView='CategoryView'
Invoke-ScriptAnalyzer -Path 'C:\temp\Rule 4.ps1'

ParserError: (MissingExpressionInNamedArgument:String) [Invoke-ScriptAnalyzer], ParseException
ParserError: (TerminatorExpectedAtEndOfString:String) [Invoke-ScriptAnalyzer], ParseException
ParserError: (MissingEndParenthesisInExpression:String) [Invoke-ScriptAnalyzer], ParseException
ParserError: (InvalidFunctionParameter:String) [Invoke-ScriptAnalyzer], ParseException
ParserError: (MissingEndParen...onParameterList:String) [Invoke-ScriptAnalyzer], ParseException
ParserError: (MissingEndCurlyBrace:String) [Invoke-ScriptAnalyzer], ParseException
ParserError: (ParameterAttrib...ntOrScriptBlock:String) [Invoke-ScriptAnalyzer], ParseException

The first should be sufficient.

@LaurentDardenne
Copy link
Author

Just a question about the exceptions mentioned above.
According to this:

It generates DiagnosticResults (errors and warnings) to inform users about potential code defects and suggests possible solutions for improvements.

A file with an incorrect syntax it should not send into the pipeline a DiagnosticRecord with severity error instead of an exception ?

@kapilmb
Copy link

kapilmb commented Sep 8, 2016

PSScriptAnalyzer only relays the parse errors generated by the PowerShell engine. DiagnosticRecords do not wrap ParseErrors because they serve different functions. A DiagnosticRecords is intended to convey issues that are encoutered on top of ParseError-free PowerShell code.

@LaurentDardenne
Copy link
Author

This code throw an exception (it is correct) :

$code=@'
function SuppressTwoVariables{
    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "b")]
    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "noexist")]
    [CmdletBinding()]
    Param(
        [Parameter(Mandatory=$true)]
        [string]
        $a="unused",

        [Parameter(Mandatory=$true)]
        [int]
        $b=3
        )
}
'@
Invoke-ScriptAnalyzer -ScriptDefinition $code|Format-List

But the error message and the FullyQualifiedErrorId are a bit strange :

Invoke-ScriptAnalyzer : Suppression Message Attribute error at line 3 in script definition : 
Cannot find any DiagnosticRecord with the Rule Suppression ID noexist.
At line:1 char:1
+ Invoke-ScriptAnalyzer -ScriptDefinition $code|Format-List
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (Microsoft.Windo...RuleSuppression:RuleSuppression) [Invoke-ScriptAnalyzer], ArgumentException
    + FullyQualifiedErrorId : Suppression Message Attribute error at line 3 in script definition : 
Cannot find any DiagnosticRecord with the Rule Suppression ID noexist.,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

@LaurentDardenne
Copy link
Author

This error do not contains the original exception :

Invoke-ScriptAnalyzer -Path "$pwd\OptimizationRules.psm1" -CustomRulePath "$pwd\OptimizationRules.psd1" -Verbose
VERBOSE: Settings not provided. Will look for settings file in the given path
G:\PS\PSScriptAnalyzerRules\Modules\OptimizationRules\Logs\OptimizationRules.psm1.
VERBOSE: Cannot find a settings file in the given path
G:\PS\PSScriptAnalyzerRules\Modules\OptimizationRules\Logs\OptimizationRules.psm1.
VERBOSE: Checking module 'G:\PS\PSScriptAnalyzerRules\Modules\OptimizationRules\Logs\OptimizationRules.psd1' ...
WARNING: Cannot find rule extension
'G:\PS\PSScriptAnalyzerRules\Modules\OptimizationRules\Logs\OptimizationRules.psd1'.
Invoke-ScriptAnalyzer : Une exception de type 'System.Exception' a été levée.
At line:1 char:1
+ Invoke-ScriptAnalyzer -Path "$pwd\OptimizationRules.psm1" -CustomRule ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ResourceExists: (Microsoft.Windo....ScriptAnalyzer:ScriptAnalyzer) [Invoke-ScriptAnalyzer], Exception
    + FullyQualifiedErrorId : Cannot find ScriptAnalyzer rules in the specified path,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

With this single message, I do not know the real reason.

In my case the import-module fail because a file was missing :

ipmo .\OptimizationRules.psd1
Import-LocalizedData : Cannot find the Windows PowerShell data file 'OptimizationRules.Resources.psd1' in directory
'G:\PS\PSScriptAnalyzerRules\Modules\OptimizationRules\Logs\fr-FR\', or in any parent culture directories.

@kapilmb kapilmb added this to the 1610 milestone Oct 14, 2016
@kapilmb kapilmb self-assigned this Oct 14, 2016
@kapilmb kapilmb modified the milestones: 1610, 1611 Nov 1, 2016
@kapilmb kapilmb modified the milestones: 1611, 1612 Dec 2, 2016
@bergmeister bergmeister modified the milestones: 1612, Backlog May 11, 2018
@LaurentDardenne
Copy link
Author

Could you consider these points in version 2.0 of PSSA?
In the error report, we do not know which line triggered the error, yet the analysis engine knows it.
We do not know how to distinguish an error from the PSSA module or from a native rule or a rule from a personal module.

@bergmeister
Copy link
Collaborator

In 1.18 PSSA was improved to return a DiagnosticRecord of Severity ParseError when there was a syntax issue. See the Parser Errors section in the main Readme of this repo. Does this not resolve your issue?

@LaurentDardenne
Copy link
Author

There are several error cases here, some have not changed, one has been corrected (an incomprehensible error message) and that on syntax errors has been improved.
But if we encounter a parsing error on code, the processing should stop.

Which is not the case, since here the 'PSReviewUnusedParameter' rule is executed with a false message:

$s=@'
Function TestParameterSet{
  [CmdletBinding(DefaultParameterSetName='F2')]
   Param (
     [Parameter(Position=2")]
     [string] $A
   )
    Write-Host"ParameterSetName =$($PsCmdlet.ParameterSetName)"
 }
'@
 Invoke-ScriptAnalyzer -ScriptDefinition $s

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
MissingExpressionInNamedArgument    ParseError              4     Instruction manquante après «=» dans largument
                                                                  nommé.
TerminatorExpectedAtEndOfString     ParseError              7     Le terminateur " est manquant dans la chaîne.
MissingEndParenthesisInExpression   ParseError              4     Parenthèse fermante «)» manquante dans lexpression.
InvalidFunctionParameter            ParseError              4     Les déclarations de paramètre sont une liste de noms
                                                                  de variable séparés par des virgules avec des
                                                                  expressions dinitialiseur facultatives.
MissingEndParenthesisInFunctionPara ParseError              4     Parenthèse fermante «)» manquante dans la liste de
meterList                                                         paramètres de fonction.
MissingEndCurlyBrace                ParseError              1     Accolade fermante «}» manquante dans le bloc
                                                                  d'instruction ou définition du type manquante.
ParameterAttributeArgumentNeedsToBe ParseError              4     Un argument d'attribut doit correspondre à une
ConstantOrScriptBlock                                             constante ou à un bloc de script.
PSReviewUnusedParameter             Warning                 4     The parameter '__error__' has been declared but not
                                                                  used.
PSAvoidTrailingWhitespace           Information             2     Line has trailing whitespace

And when I try to use a log module to trace errors in my rule modules it is not possible :-/

@rjmholt
Copy link
Contributor

rjmholt commented May 13, 2020

But if we encounter a parsing error on code, the processing should stop.

That's not how the PowerShell parser works though. It returns an AST and a list of tokens in any event, along with a list of parse errors. This is deliberately designed so that you get as many opportunities to fix your program with each parse. In order to accomplish this, the parser is robust to syntax errors; it does its best to determine where the syntax error ends and then resumes parsing the program at that point. That program will never run, but it does contain valid segments.

PSScriptAnalyzer sees the output of this function ([System.Management.Automation.Language.Parser]::ParseInput). At that point it has a data structure representing the program and is able to examine the parts that the parser returned as valid.

I think your issue stems from the parser's error recovery not being as good as it should be, which is an issue in PowerShell. Given that the PowerShell parser returns ASTs with valid parts, that feels like a sign to me that PSScriptAnalzyer should honour that philosophy.

@LaurentDardenne
Copy link
Author

That's not how the PowerShell parser works though.

I agree with that and you know much more about it than I do.
I do not know if this clarifies my words:
But if we encounter a parsing error on the code under analysis, the analysis of the code by PSSA should stop.

For example this is not executed by Powershell:

 @'
 Function TestParameterSet{
   [CmdletBinding(DefaultParameterSetName='F2')]
    Param (
      [Parameter(Position=2")]
      [string] $A
    )
     Write-Host"ParameterSetName =$($PsCmdlet.ParameterSetName)"
  }
'@ > c:\temp\myScript.ps1

c:\temp\myScript.ps1
Au caractère C:\temp\myScript.ps1:4 : 26
+      [Parameter(Position=2")]
+                          ~
Instruction manquante après « = » dans l’argument nommé.
Au caractère C:\temp\myScript.ps1:7 : 63
+     Write-Host"ParameterSetName =$($PsCmdlet.ParameterSetName)"
+                                                               ~
Le terminateur " est manquant dans la chaîne.
Au caractère C:\temp\myScript.ps1:4 : 26
+      [Parameter(Position=2")]
...

The implicit contract is not fulfilled, namely "I can only execute code if it is syntactically correct".
Here I accept this behavior of Powershell, but I do not understand why PSSA chains the execution of rules on code which is not syntactically valid.

Then the other point is that during the development of PSSA rules with Powershell I am unable to determine simply who triggers an error and where, it is my responsibility to determine the cause.

@rjmholt
Copy link
Contributor

rjmholt commented May 13, 2020

It's probably not all that clear cut, but my personal feeling is that there will be some PSSA rules that shouldn't try to run on ASTs that contain errors, but others that might. Perhaps it's a question of severity? A big reason to still run rules when we hit a parser error is that it means you can get meaningful PSScriptAnalyzer results in interactive contexts; if you're in the middle of writing your script in VSCode and you didn't close a scriptblock, your outstanding diagnostics probably shouldn't disappear because of that. In fact you might be in the middle of fixing them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants