Alternative way of obtaining function AST. by hubuk · Pull Request #1659 · PowerShell/PSScriptAnalyzer

PR Summary

Fixes #966.

Method UseShouldProcessCorrectly.TryGetShouldProcessValueFromAst is trying to obtain function body AST using Find with ScriptBlockAst filter. Unfortunately FunctionInfo exposes only FunctionDefinitionAst so the Find method fails.
This PR improves this behavior by checking existence of FunctionDefinitionAst first and using old code as a fallback mechanism.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation.
  • Changed code is executed only when an exception is thrown in a "regular" execution path. This exception is hard to trigger in a controller environment. No already created tests were checking the changed code.
• 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.

Playing with this a bit, I wonder if we actually just want searchNestedScriptBlocks to be true?

$ast = {
    function Test { "Hello" }
}.Ast.EndBlock.Statements[0]

# No output, because $ast.Body is a nested scriptblock
$ast.Find({ $args[0] -is [System.Management.Automation.Language.ScriptBlockAst] }, $false)

# Finds the body
$ast.Find({ $args[0] -is [System.Management.Automation.Language.ScriptBlockAst] }, $true)

@rjmholt If this is guaranteed that Find will always return function body even if the body itself contains other script blocks then searchNestedScriptBlocks: $true looks OK and I can change my commit.

If this is guaranteed that Find will always return function body even if the body itself contains other script blocks

$ast = {
    function Test { { "decoy" }; "Hello" }
}.Ast.EndBlock.Statements[0]

# Gets the body rather than the decoy scriptblock
$ast.Find({ $args[0] -is [System.Management.Automation.Language.ScriptBlockAst] }, $true)

Looks like the answer is yes 🙂. The AST visitor visits the parents before the children, so the body is the first thing it sees.

A good way to ensure that guarantee though would be to add a regression test

@rjmholt Regression test may be hard to implement as the changed code is executed only when there is an exception thrown due to problem described here: #966 (comment)

The comment in code mentions: // functionInfo.ScriptBlock.Attributes may throw if it cannot resolve an attribute type but I could not trigger this behavior by simply providing an unknown attribute.

I will dig into that a little bit deeper tho.

I updated the changes. Unfortunately was unable to provide test for this :(
After merging this change Issue #966 may be closed.
PR #995 may also be closed without merging.