Make AnalysisService use the latest version of PSScriptAnalyzer #677

rjmholt · 2018-06-05T00:23:42Z

Fixes PowerShell/vscode-powershell#1298 and #657.

Changes are:

Load the latest version of PSScriptAnalyzer we can find on the path.
Eliminate the VerifyPSScriptAnalyzerAvailable, since ImportPSModule() executes the same code internally.
Do all the runspace manipulation in a factory method rather than in a constructor...
Get rid of a bunch of needless private variables that just backed public ones.
Get rid of _hasScriptAnalyzerModule, since it only got checked after the object was constructed (meaning it was always true...).
Added some null checks on AnalysisService usage, since it can definitely be null.

rkeithhill · 2018-06-05T01:17:49Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

@@ -29,19 +30,16 @@ public class AnalysisService : IDisposable

        private const string PSSA_MODULE_NAME = "PSScriptAnalyzer";

+        private static readonly Version s_pssaMinimumVersion = new Version(1, 16);


Considering 1.17 is shipping now, should we move to that version?

Yes that crossed my mind. On one hand I feel like minimum version should encode what our lowest minimum supported version of PSSA is. On the other, we don't necessarily have a good answer for what that is, and I can't think of any downside to keeping it fresh...

rkeithhill · 2018-06-05T01:18:34Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                activeRules = value;
-            }
-        }
+        public string[] ActiveRules { get; set; }


Ah, soo much nicer. Thx. :-)

rkeithhill · 2018-06-05T01:34:53Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-            {
-                var sb = new StringBuilder();
-                var commands = InvokePowerShell(
+            var commands = InvokePowerShell(


In a separate PR I'd like to change this from Get-Command to Get-Module -Name PSScriptAnalyzer. From the resulting PSModuleInfo, we can get the Version of the module as well as the ExportedCmdlets. Unless you're feeling spunky and want to make this change. :-)

rkeithhill · 2018-06-05T01:38:54Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-            {
-                var sb = new StringBuilder();
-                var commands = InvokePowerShell(
+            var commands = InvokePowerShell(
                "Get-Command",
                new Dictionary<string, object>
                {
                    {"Module", "PSScriptAnalyzer"}


I wonder if we should use PSSA_MODULE_NAME here?

rkeithhill

I have a number of comments but they're all pretty minor.

rjmholt · 2018-06-05T01:57:24Z

Uhoh, my last commit broke it! Will fix.

SeeminglyScience · 2018-06-05T03:28:52Z

### Testing API usage for PowerShell v3...
Analysis\AnalysisService.cs(476,41): error CS1503: Argument 1: cannot convert from 'Microsoft.PowerShell.Commands.ModuleSpecification[]' to 'string[]' 
 [C:\projects\powershelleditorservices\src\PowerShellEditorServices\PowerShellEditorServices.csproj]

Might have to switch back to the method of loading a specific path until we can drop PSv3 :(

rjmholt · 2018-06-05T03:30:19Z

@SeeminglyScience Yes I just saw that 🤕. The annoying part is that there's no documentation about that fact 😠

rkeithhill · 2018-06-05T03:53:37Z

I feel your pain!

SeeminglyScience

Changes look great, just one concern around the version check

SeeminglyScience · 2018-06-05T13:51:09Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+        /// session state.
+        /// </summary>
+        /// <returns>A runspace pool with PSScriptAnalyzer loaded for running script analysis tasks.</returns>
+#if PowerShellv5


Not sure if that'll work like you expect unfortunately. We don't actually build separate assemblies for each version (except as a test), the compiler directives just to allow it to compile.

The way we get this to work in the debugger is by having an interface (IVersionSpecificOperations) and several implementations for each PowerShell version required. Then at runtime the implementation for that version is picked. That way no missing member exceptions get thrown.

Here's the version specific interface stuff

IVersionSpecificOperations
PowerShell3Operations
PowerShell4Operations
PowerShell5Operations

And here's where that's used in PowerShellContext.

At least, I'm pretty sure that's how it works. It's possible that was done for readability though. If y'all have a PSv3 machine there I'd give it a go real quick to confirm. If it crashes immediately, that's probably why.

Bugger! I was just basing it on the defined constants in the csproj. I'll see if I can test it on a v3 machine. And then ideally we should save the better solution for later somewhere (not that I'm necessarily suggesting leaving it in as a comment)

rjmholt · 2018-06-05T16:33:28Z

Ok, I've just reverted to the old way now, but include a cache for logging at least.

The better way for when we drop v3/4 support is here: https://gist.github.com/rjmholt/9820861c669610941a6d62b36d121fa7

rjmholt · 2018-06-05T23:46:03Z

This probably merits some interactive testing, but I'm happy to merge when others are. I tried to test it on a machine running PowerShell v4 but there was some problem with reference assembly linking...

TylerLeonhardt · 2018-06-06T15:47:03Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

        {
            using (var ps = System.Management.Automation.PowerShell.Create())
            {
+                // Encode an equivalent of the PowerShell pipeline:
+                //   Get-Module -ListAvailable -Name "PSScriptAnalyzer" | Where-Object -Property "Version" -ge "1.16"


I feel like at this point we might as well just grab the latest version rather than looking for ones that meet the min.

I feel like we should address that in an issue and have a proper discussion - just so we know what scenarios there are for version-locking vs flexible drop-in. Mainly for the feedback from @bergmeister and @JamesWTruher.

If I remember correctly, the extension used to install PSSA in its AppData folder but does not do that any more. Is that correct?
I don't know much about the compatibility of PSSA before 1.16.1 but the version of it should not matter too much. For example due to one of the recent PRs, PSES now has a new rule from 1.17.0 but it is no problem if one has only 1.16.1 because PSSA just ignores rules in the pssa settings that it does not recognize.

Currently we install PSSA to a directory internal to the extension itself, and when the extension is loaded it adds that directory to the end of the module path in the internally hosted PowerShell session.

The question is really whether we always take the latest PSSA (i.e. PSSA 17.1) or we use a minimum version (and allow higher versions on the module path to override our bundled one).

Given the recent re-release of PSSA to 17.1, I would imagine it benefits us to only use a minimum version, so that we don't block an update of PSSA unnecessarily (by block I mean, users would only get that new release when we do our release).

Welp that complicates things.

The benefit of obeying Import-Module ordering logic is predictability. If we lose that it feels more like a gamble which module you're going to import. Do we know of anyone who was specifically trying to downgrade PSSA? It isn't super common for folks to build against PSSA as far as I know, so I don't think issues with it will be as common as say Newtonsoft. If we haven't had any specific complaints we may want to switch back to importing the latest only.

Why not just select the version number and pipe to sort to pick the latest one?

The original idea was to use at least the version we shipped with the extension but allow users to use a "newer" version if installed on their machine. So, using latest sounds good to me.

Yeah I think the use-latest approach is the best way -- at least InitialSessionState.ImportPSModulesFromPath seems to have a sane behaviour.

I've opened an issue for our above discussion BTW:
PowerShell/PowerShell#7020

TylerLeonhardt · 2018-06-06T15:49:49Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                {
-                    sb.AppendLine(rule);
-                }
+                sb.AppendLine("        " + cmdletName);


nit: You're using a StringBuilder which is good but you're still concatenating 2 strings here. C# has string interpolation, can you use that instead?

I think it's like:

$" {cmdletName}"

oh actually you do it a few lines up 😆

I deliberately avoided it here on the basis that we're already using string builders for performance. In fact, it looks like we should split this out into new string builder calls. I'll make the change to that.

TylerLeonhardt · 2018-06-06T15:50:30Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+            sb.AppendLine("    Available Rules:");
+            foreach (string ruleName in GetPSScriptAnalyzerRules())
+            {
+                sb.AppendLine("        " + ruleName);


TylerLeonhardt · 2018-06-06T16:48:46Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                this._analysisRunspacePool = null;
+                _analysisRunspacePool.Close();
+                _analysisRunspacePool.Dispose();
+                _analysisRunspacePool = null;


is the nulling needed here? I'm not sure.

Well really we should implement the Dispose pattern. I'm guessing the nulling is to allow GC to collect, but that's protecting against the scenario where the parent is disposed and the reference still carried around for a while. It doesn't hurt I guess.

…plement dispose pattern

rjmholt · 2018-06-07T18:00:20Z

Ok, we're now using the latest version of PSScriptAnalyzer again 😄

SeeminglyScience

LGTM, just that pesky _ on the IDisposable refactor

SeeminglyScience · 2018-06-08T11:55:46Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+
+        #region IDisposable Support
+
+        private bool disposedValue = false; // To detect redundant calls


I wonder if there's a way to set this refactor to prefix this field with _. I always forget to change this

bergmeister · 2018-06-08T17:44:34Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs


        /// <summary>
        /// Defines the list of Script Analyzer rules to include by default if
        /// no settings file is specified.
        /// </summary>
-        private static readonly string[] IncludedRules = new string[]
+        private static readonly string[] s_includedRules = new string[]


What is the s in there (I guess a typo, not Hungarian notation).
Just a thought in general as I see that you change the style in general to be more consistent. You should make sure that those changes make it into version 1.x of the extension so that when you branch off to version 2.0, it is still easily possible to merge back commits to version 1.x

The s_ is to indicate a private static variable - so I suppose it is a sort of Hungarian-notation-lite - in the same vein as the _ before private variables. It's the convention used in all the other Microsoft code I've seen. There are places within this codebase where it's not done as much, but I think we should change it over as we touch the code for both clarity and consistency with related projects.

I don't quite follow the latter part of your comment, but if you're saying we should try and make as much as possible work with 1.x versions, I am strongly in agreement. I don't think the v2 branch will be taking in commits from master for a little while yet. I'm not sure it's worth one big commit to change the style over - I imagine opinions might differ on that - although I'd be happy to do it personally.

As, I see, I have never seen it this way (or probably I've just never recognized it). To re-word my other comment, which was more about the workflow in the (distant) future: Style changes should not be made any more once v2 has actually branched off (i.e. has become the mainstream branch) to allow for merge-ability of commits in v2 back to v1 (or style changes should be applied to both branches otherwise).

Yes agreed. My opinion there then is that we should invest in a sweeping style change PR. The style in PSES is already inconsistent but is moving over to be much closer to the conventions recommended by Microsoft and used in the other PowerShell projects (PascalCase properties, _-prefixed private variables, etc.). Otherwise we'll be stuck with code that is less familiar and (in my opinion) not as clear.

TylerLeonhardt

Alright just a few more comments/questions but it's looking good!

TylerLeonhardt · 2018-06-09T18:40:41Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                this._logger.Write(LogLevel.Verbose, sb.ToString());
+            if (_pssaModuleInfo == null)
+            {
+                throw new Exception("Unable to find loaded PSScriptAnalyzer module for logging");


Will this fail gracefully? We don't want all of PSES to crash if it can't find PSSA.

This could be a Error log instead of throwing.

Good catch. The invocation of AnalysisService.Create() is already wrapped in a try/catch, and I would prefer not to do both. Do you have a preference on catching all exceptions outside vs inside?

Ah turns out this is already handled -- Create catches all exceptions, logs them as exceptions and returns null.

TylerLeonhardt · 2018-06-09T18:45:34Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+        /// <summary>
+        /// Create a new runspace pool around a PSScriptAnalyzer module for asynchronous script analysis tasks.
+        /// This uses the PowerShell module API to load the first PSScriptAnalyzer module on the module path with
+        /// with the required minimum version and then creates a runspace pool with that module loaded in the initial


nit: update the comment to say we load the latest on the machine

TylerLeonhardt · 2018-06-09T18:46:47Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

                {
-                    return false;
+                    throw new Exception("Unable to find PSScriptAnalyzer module", e);


Same question from above about failing gracefully

TylerLeonhardt · 2018-06-09T18:49:24Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+
+                if (pssaModuleInfo == null)
+                {
+                    throw new Exception("Unable to find PSScriptAnalyzer module");


TylerLeonhardt · 2018-06-09T18:52:35Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+                {
+                    _analysisRunspacePool.Close();
+                    _analysisRunspacePool.Dispose();
+                    _analysisRunspacePool = null;


I'm pretty sure you only need to do the Close, here. I think the Close does a Dispose. I'm also curious if the null line is needed.

Had a quick look at the code and it looks like Dispose() calls Close():
https://github.com/PowerShell/PowerShell/blob/c1c5344a8897262433141ecbc13bb06ac2c4bbef/src/System.Management.Automation/engine/hostifaces/RunspacePoolInternal.cs#L823-L836

I'm not sure that Close() calls Dispose().

Dunno how many resources or how much memory a runspace pool allocates, but the comments generated with the Dispose pattern snippet indicate large resources should be set to null.

TylerLeonhardt · 2018-06-09T18:52:47Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+        public void Dispose()
+        {
+            // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
+            Dispose(true);


Love this pattern :)

TylerLeonhardt · 2018-06-09T18:53:37Z

src/PowerShellEditorServices/Session/EditorSession.cs

@@ -164,7 +164,7 @@ internal void InstantiateAnalysisService(string settingsPath = null)
            // Script Analyzer binaries are not included.
            try
            {
-                this.AnalysisService = new AnalysisService(settingsPath, this.logger);
+                this.AnalysisService = AnalysisService.Create(settingsPath, this.logger);


If only .NET Core used the composer pattern with NamedPipes 🙃

rjmholt · 2018-06-11T17:34:24Z

I've converted all the Exceptions I threw to be AnalysisServiceLoadExceptions, so we know when we have an unexpected failure mode. AnalysisService.Create() catches all exceptions so I've removed the try/catch in the caller.

I've also updated the comment to reflect the truth of what we do now (with the latest version) and improved our use of the dispose pattern.

rjmholt · 2018-06-12T16:31:25Z

@tylerl0706 can you have a quick look at this and if you approve we can merge it

TylerLeonhardt

LGTM! 🎉

Robert Holt added 3 commits June 4, 2018 16:41

Add minimum version to PSSA import, refactor redundant code

e83b1ea

Make AnalysisService constructor private

1d3f874

Add null exception defense for AnalysisService

cb2fd46

rjmholt requested review from TylerLeonhardt, rkeithhill and SeeminglyScience June 5, 2018 00:23

rkeithhill reviewed Jun 5, 2018

View reviewed changes

rkeithhill approved these changes Jun 5, 2018

View reviewed changes

rjmholt added 2 commits June 4, 2018 19:54

Use Get-Module to document PSScriptAnalyzer features

3a02da6

Fix XML comments

8fb6350

rjmholt added 2 commits June 4, 2018 20:59

Use old ImportPSModule API for older PowerShell versions

cf50bfd

Ensure older PowerShells get a good version of PSSA

50974a8

SeeminglyScience reviewed Jun 5, 2018

View reviewed changes

Revert to old PSScriptAnalyzer import, with cached result

bfd9f85

Add some further comments to document

5e3958e

rkeithhill mentioned this pull request Jun 5, 2018

Log the version of PSSA we import #673

Closed

TylerLeonhardt reviewed Jun 6, 2018

View reviewed changes

Robert Holt added 3 commits June 6, 2018 10:20

Address @tylerl0706's feedback, convert logging to string builder, im…

73d6f27

…plement dispose pattern

Add XML comment to dispose method

2ed90f8

Use latest PSScriptAnalyzer version rather than hunt for minimum version

7fce107

rjmholt changed the title ~~Make AnalysisService respect a minimum version~~ Make AnalysisService use the latest version of PSScriptAnalyzer Jun 7, 2018

rjmholt mentioned this pull request Jun 7, 2018

InitialSessionState.ImportPSModule just selects a random module on the module path PowerShell/PowerShell#7020

Closed

SeeminglyScience approved these changes Jun 8, 2018

View reviewed changes

Put _ on private disposedvalue variable

0b1d01a

bergmeister reviewed Jun 8, 2018

View reviewed changes

TylerLeonhardt reviewed Jun 9, 2018

View reviewed changes

Handle exceptions more formally, update comments, improve disposal

a6d8b3c

TylerLeonhardt approved these changes Jun 13, 2018

View reviewed changes

Merge branch 'master' into pssa-module-version-check-1298

72ce1f4

rjmholt merged commit b9c428a into PowerShell:master Jun 13, 2018

rjmholt deleted the pssa-module-version-check-1298 branch December 12, 2018 06:00

		@@ -29,19 +30,16 @@ public class AnalysisService : IDisposable

		private const string PSSA_MODULE_NAME = "PSScriptAnalyzer";

		private static readonly Version s_pssaMinimumVersion = new Version(1, 16);


		#region IDisposable Support

		private bool disposedValue = false; // To detect redundant calls

Make AnalysisService use the latest version of PSScriptAnalyzer #677

Make AnalysisService use the latest version of PSScriptAnalyzer #677

Uh oh!

Conversation

rjmholt commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes are:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill left a comment

Choose a reason for hiding this comment

Uh oh!

rjmholt commented Jun 5, 2018

Uh oh!

SeeminglyScience commented Jun 5, 2018

Uh oh!

rjmholt commented Jun 5, 2018

Uh oh!

rkeithhill commented Jun 5, 2018

Uh oh!

SeeminglyScience left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt commented Jun 5, 2018

Uh oh!

rjmholt commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt commented Jun 7, 2018

Uh oh!

rjmholt commented Jun 5, 2018 •

edited

Loading

rjmholt Jun 6, 2018 •

edited

Loading

rjmholt Jun 6, 2018 •

edited

Loading

rjmholt Jun 6, 2018 •

edited

Loading

bergmeister Jun 8, 2018 •

edited

Loading