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

Alternative way of obtaining function AST. #1659

Merged
merged 1 commit into from
Apr 23, 2021
Merged

Alternative way of obtaining function AST. #1659

merged 1 commit into from
Apr 23, 2021

Conversation

hubuk
Copy link
Contributor

@hubuk hubuk commented Apr 4, 2021

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

@rjmholt
Copy link
Contributor

rjmholt commented Apr 4, 2021

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)

@hubuk
Copy link
Contributor Author

hubuk commented Apr 4, 2021

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

@rjmholt
Copy link
Contributor

rjmholt commented Apr 5, 2021

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.

@rjmholt
Copy link
Contributor

rjmholt commented Apr 6, 2021

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

@hubuk
Copy link
Contributor Author

hubuk commented Apr 6, 2021

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

@hubuk
Copy link
Contributor Author

hubuk commented Apr 6, 2021

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.

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

Successfully merging this pull request may close these issues.

Exception occurred while constructing custom validator attribute.
2 participants