Skip to content

Expose C# API for hosted scenarios + allow AST to be passed directly #1056

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

Closed
rjmholt opened this issue Aug 18, 2018 · 7 comments
Closed

Expose C# API for hosted scenarios + allow AST to be passed directly #1056

rjmholt opened this issue Aug 18, 2018 · 7 comments

Comments

@rjmholt
Copy link
Contributor

rjmholt commented Aug 18, 2018

A fairly large ask, but I would be very happy doing the work. Just want to discuss it with everyone.

In PowerShell EditorServices, we have a hosted PowerShell environment and do a lot of parsing and AST analysis separately from PSScriptAnalyzer. And we are often generating a new AST with every document change. This means PSScriptAnalyzer is doing a fair amount of duplicate work when we call it, since it only accepts input as a string or a filepath.

Our ideal would be to be able to embed PSScriptAnalyzer with a C# API call, pass in our own hosted PowerShell session as its engine in the constructor, and then send pre-parsed ASTs to PSScriptAnalyzer for analysis. The cmdlets would then wrap or be an alternative entry point for this API.

@JamesWTruher and I have discussed something like this before, but I just want to document the desire. He and I also discussed being able to only use rules that use the parser, rather than the whole engine.

Anyway, to summarise:

  • We'd like to pass AST objects directly to PSSA, rather than having to reparse strings
  • Ideally we could do this in C# (no ps.AddCommand("Invoke-ScriptAnalyzer").Invoke())
  • Even more ideally we could pass in our own hosted PowerShell runtime for the analyzer to use, rather than have it spin up its own.
@SeeminglyScience
Copy link
Collaborator

The public API already exists and was used in the past. Microsoft.Windows.PowerShell.ScriptAnalyzer.ScriptAnalyzer has a AnalyzeSyntaxTree method that does everything we need.

If I remember correctly the switch was due to some static helper clean up that wasn't happening, resulting in a large amount of memory usage.

@rjmholt
Copy link
Contributor Author

rjmholt commented Aug 19, 2018

Yes I had that suspicion. I had a very brief play with the code in PowerShell and didn't see hook-in points, but I'm guessing there's some singleton dance (shudder) to get at the script analyzer object.

Well in that case I suppose we should make some attempt to use that and if it doesn't work, this issue can track making that usable...?

@SeeminglyScience
Copy link
Collaborator

@rjmholt it does have a static property Instance so I'd agree that it's probably intended to be a singleton. However, the constructor is public. Not sure if that's intended, seems to work though.

using namespace System.Management.Automation
using namespace Microsoft.Windows.PowerShell.ScriptAnalyzer

class NullOutputWriter : IOutputWriter {
    [void] WriteError([ErrorRecord] $errorRecord) { }
    [void] WriteWarning([string] $message) { }
    [void] WriteVerbose([string] $message) { }
    [void] WriteDebug([string] $message) { }
    [void] ThrowTerminatingError([ErrorRecord] $errorRecord) { throw $errorRecord }
}

$rs = [runspacefactory]::CreateRunspace($Host, [initialsessionstate]::CreateDefault())
try {
    $rs.Open()
    $analyzer = [ScriptAnalyzer]::new()
    $analyzer.Initialize(
        <# runspace:            #> $rs,
        <# outputWriter:        #> [NullOutputWriter]::new(),
        <# customizedRulePath:  #> @(),
        <# includeRuleNames:    #> @(),
        <# excludeRuleNames:    #> @(),
        <# severity:            #> @(),
        <# includeDefaultRules: #> $true,
        <# suppressedOnly:      #> $false,
        <# profile:             #> [string]::Empty)

    $completionData = [CommandCompletion]::MapStringInputToParsedInput('gci', 3)
    $analyzer.AnalyzeSyntaxTree($completionData.Item1, $completionData.Item2, [string]::Empty)

    # returns:
    # RuleName                   Severity  ScriptName Line  Message
    # --------                   --------  ---------- ----  -------
    # PSAvoidUsingCmdletAliases  Warning              1     'gci' is an alias of 'Get-ChildItem'. Alias can
    #                                                       introduce possible problems and make scripts hard to
    #                                                       maintain. Please consider changing alias to its full
    #                                                       content.
} finally {
    if ($analyzer) {
        $analyzer.CleanUp()
    }

    $rs.Dispose()
}

@bergmeister
Copy link
Collaborator

@SeeminglyScience is right, the API is already there but there were memory problems in the past due to static members not being disposed in the Helper class (leading to big memory consumption) in Helper.cs

@SeeminglyScience
Copy link
Collaborator

@bergmeister thanks for confirming. I do wonder though, was the memory issue due to our failure to invoke a cleanup method? I'd think the same problem would still be occurring otherwise. Right now we invoke PSSA as a cmdlet in it's own runspace pool, but since the singleton instances would be app domain wide, to me that means we just weren't doing something that the cmdlets do themselves.

@bergmeister
Copy link
Collaborator

@SeeminglyScience
Sorry, I forgot to respond. This is the original memory issue of PSSA that was never followed up
PowerShell/PowerShellEditorServices#269
Now that we have the preview, we could try experimenting with calling the API and see if the memory issue still applies and if so try fixing it.

@bergmeister
Copy link
Collaborator

@rjmholt I will close this since we agreed that PSSA already exposes an API (and given that it was used in the past, it seems that the available API is suitable for PSES). Should problems show up because of a bug or memory issues, then we will of course fix it in PSSA but given that PSSA has changed quite a lot it is possible that it is already fixed. If you think the available APIs are not sufficient for PSES, please re-open with details of what kind of API you'd need to be added.

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

No branches or pull requests

3 participants