New PowerShell compatibility rules #1133

rjmholt · 2019-01-29T19:32:51Z

PR Summary

Resolves #805.

Adds three new rules:

UseCompatibleCommands: checks the compatibility of cmdlets and functions available by default with different PowerShell versions/platforms (effectively outmodes UseCompatibleCmdlets)
UseCompatibleTypes: checks the compatibility of types/.NET APIs available by default with different PowerShell versions/platforms
UseCompatibleSyntax: a simpler, by-hand rule that checks for known bad syntaxes (like [type]::new() and using module MyModule) when given PowerShell versions are targeted

The first two rules rely on profiles collected from PowerShell environments. The tools to collect this are included with the PR.

Done since opening PR

Fix typename serialisation in JSON

Add type compatibility check rule

Add more testing

Document code properly

Collect profiles on more platforms

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

bergmeister · 2019-01-29T19:37:53Z

@rjmholt Please resolve the merge conflict or let me know if you need help with that.
That's a lot of code to review, please be patient. Can you make a statement whether this includes breaking changes in terms of the way the rule is being configured?
Also do not hesitate to ask if you have questions/problems getting CI to pass, I myself have spent quite a bit with it in the past and might be able to help or give pointer, the same applies also to running tests locally as there are 2 ways (.\build.ps1 -Test or installing the module manually and then invoking Pester like it is done in CI can lead to different results sometimes depending on various things like OS, PS edition, locale, etc.).

rjmholt · 2019-01-29T19:52:07Z

Hi @bergmeister, sorry this is so big. Wasn't meant to be at the outset. When I'm closer to being done, I'll try and comment on areas where I think I need extra scrutiny.

Please resolve the merge conflict

Easy! Will do.

That's a lot of code to review, please be patient

Yes, absolutely.

Can you make a statement whether this includes breaking changes in terms of the way the rule is being configured?

My immediate intention is to break nothing. For now there's a totally new rule called UseCompatibleCmdlets2. I figured I would worry about the name later. But the only real "breakage" is that you might get double diagnostics for compatibility.

Also do not hesitate to ask if you have questions/problems getting CI to pass, I myself have spent quite a bit with it in the past and might be able to help or give pointer, the same applies also to running tests locally as there are 2 ways (.\build.ps1 -Test or installing the module manually and then invoking Pester like it is done in CI can lead to different results sometimes depending on various things like OS, PS edition, locale, etc.).

Will do! I'm not quite there yet, but haven't had too much trouble figuring out the build or testing yet. I'll let you know if I'm hitting my head against anything.

SteveL-MSFT

Will take some time to review the whole thing, some initial comments

CrossCompatibility/CrossCompatibility.psd1

CrossCompatibility/CrossCompatibility.psm1

rjmholt

I've done all the code changes I think I need at present. Looking for input on:

What other tests I should write
How to fix my current build breakages (documentation tests, etc.)
Feedback on profile collection stuff

Ideally the only changes I make at this point are:

More tests
More documentation
More comments
Possible profile updates

However, I'm removing the WIP tag mainly so that I can get feedback

rjmholt · 2019-02-05T23:24:26Z

CrossCompatibility/CrossCompatibility.psm1

@@ -0,0 +1,1610 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.


This file is quite important, since it's used for the profile collection itself. So it's worth looking over to see if I'm making any mistakes and either missing or including commands/types I shouldn't be.

rjmholt · 2019-02-05T23:26:25Z

CrossCompatibility/CrossCompatibility/Query/CompatibilityProfileData.cs

+
+        public string Id { get; }
+
+        public HashSet<string> ConstituentProfiles { get; }


I would have preferred this to be something readonly, but there's no way to wrap a HashSet<string> in a way that's both readonly and work with .NET452

CrossCompatibility/CrossCompatibility.psm1

Tests/Rules/UseCompatibleTypes.Tests.ps1

build.psm1

.gitignore

bergmeister

I had a first glance at it and made some comments and commits myself. Looks OK so far but I am a bit worried about performance since I have done a prototype last year to include script based rules in PSSA itself and found that execution time went up by a factor of 10. Similar to the already existing PSUseCompatibleCmdlets rule, it is probably best to run it not by default unless it is configured.
In terms of the build, have a look at the structure for PS v3/4 in the output and build.psm1 to get the WMF4 build to work as well, it is best to partially integrate your crosscompatibilty build.ps1 the main build.ps1 and then everything should just work fine end-to-end.
Tip: Focus first on the Windows PowerShell build, there can sometimes be quirks with some PowerShell Core tests in AppVeyor (due to the environment), I can help you with that once the Windows PowerShell build is green.
I am focusing on the integration of this rule into PSSA, not the actual rule itself. Especially since I am not an expert on most of the stuff that you did, please get someone from MSFT to review your main code.
Let me know if that is OK so far in terms of feedback/progression from my side. We can also schedule a Skype call later if it is easier to discuss/debug things together...
One final question: Is there a reason we keep the old rule (i.e. can the old rule do something that your new rules can't?)

CrossCompatibility/CrossCompatibility.psd1

CrossCompatibility/CrossCompatibility/Common/PowerShellVersion.cs

CrossCompatibility/CrossCompatibility/Query/RuntimeData.cs

CrossCompatibility/Tests/UtilityApi.Tests.ps1

Rules/CompatibilityRules/CompatibilityRule.cs

global.json

Engine/Engine.csproj

Rules/CompatibilityRules/CompatibilityRule.cs

bergmeister

@rjmholt Awesome to see the progress from day to day. I think we are getting closer to the finish and see the light at the end of the tunnel, if it continues like this then we could maybe merge next week (assuming other MSFT people will have reviewed the core code as well). I will also take some more time the next days to review some more details of the changes in the core PSSA code that look like general improvements to me.
I now see that the crosscompat dll and rules do not get loaded automatically when running the default rules, that is good and important for performance (especially thinking of users of the ps vscode extension).
The PowerShell 5 and 6 builds are now green, thumbs up. The failing test on Ubuntu is a known culprit that sometimes starts to appear when updating the .Net Core SDK for some reason. Was there a reason why you upgraded PSSA from 2.2.102 to 2.2.103? Reverting to the old version would fix this test and make the Ubuntu build green.
Running it locally using PowerShell 6 seems to be fine but when using PowerShell 5.1 (Windows 10 1809), I see this error in the verbose log when running it the first time (does not appear when re-running the command in the same shell). The same error is visible also in the currently failing WMF4 build so I think it is definitely worthwhile investigating and fixing

C:\Users\cberg\git\PSSA_rjimholt\out\PSScriptAnalyzer [compatibility-rules ≡]> ipmo .\PSScriptAnalyzer.psd1
C:\Users\cberg\git\PSSA_rjimholt\out\PSScriptAnalyzer [compatibility-rules ≡]> Invoke-ScriptAnalyzer -ScriptDefinition foo -Verbose
VERBOSE: Settings object could not be resolved.
WARNING: System.Reflection.ReflectionTypeLoadException: Unable to load one or more of the requested types. Retrieve the
 LoaderExceptions property for more information.
   at System.Reflection.RuntimeModule.GetTypes(RuntimeModule module)
   at System.Reflection.Assembly.GetTypes()
   at System.ComponentModel.Composition.Hosting.AssemblyCatalog.get_InnerCatalog()
   at System.ComponentModel.Composition.Hosting.AssemblyCatalog.GetEnumerator()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
   at Microsoft.Windows.PowerShell.ScriptAnalyzer.SafeDirectoryCatalog..ctor(String folderLocation, IOutputWriter
outputWriter)
VERBOSE: Analyzing Script Definition.
...

Engine/ScriptAnalyzer.cs

rjmholt · 2019-02-07T21:38:39Z

@bergmeister Just looking into the WMF 4 problem. We haven't had any ideas on what might be causing it (the hashes of the all the Newtonsoft.Json.dll assemblies seem to be the same everywhere). I'm currently setting up a Win2012 machine to investigate.

rjmholt · 2019-02-08T01:14:11Z

Not sure what's going on with this Newtonsoft.Json issue. @JamesWTruher and I fired up a Win2012 VM and got all the tests passing. I think we may need to go over the csproj with a fine-toothed comb.

rjmholt · 2019-02-08T01:24:21Z

Also struggling to reproduce the Ubuntu error -- not getting it in my VM.

CrossCompatibility/CrossCompatibility/Utility/CompatibilityProfileLoader.cs

bergmeister · 2019-02-11T22:14:20Z

@rjmholt If it is easier then you could exclude your rule from compilation for v3/4. There are already some rules and functionality that is not available in ps v3/4. One rule is even missing in ps 6 (avoidusingsinularNouns) due to missing .net APIs anyway.

rjmholt · 2019-02-13T22:52:22Z

rjmholt · 2019-02-13T22:53:06Z

I'm still not able to replicate this problem in 2012R2

rjmholt · 2019-02-13T23:19:13Z

Looking into this:

https://stackoverflow.com/questions/22685530/could-not-load-file-or-assembly-newtonsoft-json-or-one-of-its-dependencies-ma

Engine/PSScriptAnalyzer.psm1

rjmholt · 2019-02-15T05:31:41Z

Ok @bergmeister, @JamesWTruher, @SteveL-MSFT I have the build passing and in a state to review now. It's also building and testing the CrossCompatibility module. I've added 272 tests.

//cc @TylerLeonhardt + @daxian-dbw (if you've got time to give a quick review as well)

Also @SydneyhSmith - this is now ready.

adityapatwardhan · 2019-02-15T18:04:37Z

CrossCompatibility/CrossCompatibility/Common/AccessorType.cs

+        [EnumMember]
+        Set
+    }
+}


CrossCompatibility/CrossCompatibility/Common/Architecture.cs

CrossCompatibility/CrossCompatibility/Common/PowerShellVersion.cs

adityapatwardhan · 2019-02-15T18:39:45Z

CrossCompatibility/CrossCompatibility/Common/PowerShellVersion.cs

+                throw new ArgumentNullException(nameof(versionStr));
+            }
+
+            int[] versionParts = new int[3] { -1, -1, -1 };


Would it be simpler like this: (sudo code in PS )

$versionStr = "6.2.0-preview.2" $parts = $versionStr.Split("-") if ($parts.Count -gt 1) { $label = $parts[1] } $versionParts = $parts[0].Split('.') | % { [int]::Parse($_) }

Agreed this would be simpler. I'm going to leave it as is for now since it's currently working in its present form with some tests. We can change it later if required I think.

CrossCompatibility/CrossCompatibility/Common/PowerShellVersion.cs

CrossCompatibility/CrossCompatibility/CrossCompatibility.csproj

CrossCompatibility/CrossCompatibility/Data/Modules/FunctionData.cs

JamesWTruher

I've looked through the .psm1 file - just a couple of things that really need to be fixed, I'll continue to review, but it's pretty big

CrossCompatibility/CrossCompatibility.psm1

rjmholt · 2019-02-27T18:44:58Z

Just opened these to track work items after this:

Let me know if there are any others we should open

bergmeister

@rjmholt Sounds good. I assume nothing has changed substantially since I last tested it locally as the build is still green, do you want me to give it a final(!) check (otherwise I'd trust you)? As long as one other member of the PS team signs this PR off as well, then feel free to merge. Yes, I agree to raise items for other possible improvements but we can just take the 'minimum viable' and 'YAGNI' approach and improve later based on feedback IMHO.

bergmeister

I had a quick look at the changes again and they still look good but I found 2 issues when running ipmo .\out\PSScriptAnalyzer\PSScriptAnalyzer.psd1; Invoke-ScriptAnalyzer -ScriptDefinition gci -Verbose

The Verbose log tells me that the new rules don't get run by default, which is good. But it tells me that the CrossCompatibility DLL gets loaded. If we cannot avoid this then this is fine. Just something that I noticed. Performance wise, I don't see a negative impact so it is probably fine
When run in Windows PowerShell 5.1 (Windows 1 1809), I still get the following ReflectionTypeLoadException the first time it runs in the verbose log (you can ignore the verbose message about the settings object btw, this is just a red herring message that always shows up when using the -ScriptDefinitiion parameter). This does not happen in PowerShell Core btw.

VERBOSE: Settings object could not be resolved.
WARNING: System.Reflection.ReflectionTypeLoadException: Unable to load one or more of the requested types. Retrieve the LoaderExceptions property for more information.
   at System.Reflection.RuntimeModule.GetTypes(RuntimeModule module)
   at System.Reflection.Assembly.GetTypes()
   at System.ComponentModel.Composition.Hosting.AssemblyCatalog.get_InnerCatalog()
   at System.ComponentModel.Composition.Hosting.AssemblyCatalog.GetEnumerator()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
   at Microsoft.Windows.PowerShell.ScriptAnalyzer.SafeDirectoryCatalog..ctor(String folderLocation, IOutputWriter outputWriter)

At least the exception should be fixed or explained before merging, the rest looks fine to me.

rjmholt · 2019-02-27T23:23:58Z

Ok, I've just gotten signing and release building working all the way through, so that the compatibility module (now called "PSCompatibilityAnalyzer") can be released independently on the gallery

rjmholt · 2019-02-27T23:28:59Z

When run in Windows PowerShell 5.1 (Windows 1 1809), I still get the following ReflectionTypeLoadException the first time it runs in the verbose log (you can ignore the verbose message about the settings object btw, this is just a red herring message that always shows up when using the -ScriptDefinitiion parameter). This does not happen in PowerShell Core btw.

I suspect this is the problem occurring when MEF doesn't understand DLL loading the way PowerShell does.

The Add-Type I added fixes it, but also breaks the whole lazy load concept with MEF (the [Export] attributes)... My personal feeling is that MEF just needs to be removed in favour of how PSCore loads things, but that's a discussion for another time.

I'll just expand the Add-Type and see if that works.

This issue is probably also related to the CrossCompatibility.dll being loaded by default. That would be easy to fix if we fix the loading logic.

rjmholt · 2019-02-27T23:35:53Z

@JamesWTruher, @SteveL-MSFT, @daxian-dbw this PR is now in a "complete" state and is ready for signoff. Please take a quick look if you can.

rjmholt · 2019-02-27T23:59:06Z

@bergmeister looks like that fix did the trick

JamesWTruher

thanks for addressing my issues

JamesWTruher · 2019-02-28T00:29:02Z

@SteveL-MSFT , @bergmeister if you guys are ok with this - I will merge

bergmeister · 2019-02-28T00:34:34Z

LGTM. Creating an issue in PSSA or the PowerShell docs to explain the MEF issue of WPS would be good though.

SteveL-MSFT · 2019-02-28T20:44:28Z

PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Query/CommonPowerShellData.cs

@@ -0,0 +1,57 @@
+using System;


Check that all files have the copyright header

SteveL-MSFT · 2019-02-28T20:48:02Z

Rules/CompatibilityRules/UseCompatibleSyntax.cs

@@ -0,0 +1,398 @@
+using System;


Missing copyright header

SteveL-MSFT

LGTM

bergmeister · 2019-03-01T08:15:30Z

Tests/Engine/InvokeScriptAnalyzer.tests.ps1

+        BeforeAll {
+            if ($IsCoreCLR)
+            {
+                $pwshExe = (Get-Process -Id $PID).Path


Thanks for fixing this, now this test should not fail any more when updating the .Net Core SDK :-)

rjmholt requested review from bergmeister and JamesWTruher January 29, 2019 19:32

rjmholt requested a review from SteveL-MSFT January 29, 2019 19:46

rjmholt requested a review from SydneyhSmith January 29, 2019 21:14

rjmholt force-pushed the compatibility-rules branch from 27bc01f to 4420f0f Compare January 29, 2019 22:17

SteveL-MSFT requested changes Jan 30, 2019

View reviewed changes

CrossCompatibility/CrossCompatibility.psd1 Outdated Show resolved Hide resolved

CrossCompatibility/CrossCompatibility.psm1 Outdated Show resolved Hide resolved

rjmholt changed the title ~~WIP: New PowerShell compatibility rules~~ New PowerShell compatibility rules Feb 5, 2019

rjmholt commented Feb 6, 2019

View reviewed changes

bergmeister reviewed Feb 6, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

bergmeister reviewed Feb 6, 2019

View reviewed changes

bergmeister reviewed Feb 7, 2019

View reviewed changes

Engine/ScriptAnalyzer.cs Outdated Show resolved Hide resolved

rjmholt commented Feb 7, 2019

View reviewed changes

Engine/ScriptAnalyzer.cs Show resolved Hide resolved

bergmeister reviewed Feb 8, 2019

View reviewed changes

CrossCompatibility/CrossCompatibility/Utility/CompatibilityProfileLoader.cs Outdated Show resolved Hide resolved

rjmholt commented Feb 9, 2019

View reviewed changes

CrossCompatibility/CrossCompatibility/Utility/CompatibilityProfileLoader.cs Outdated Show resolved Hide resolved

rjmholt force-pushed the compatibility-rules branch from c02d143 to 2ff8b5d Compare February 14, 2019 18:55

rjmholt commented Feb 15, 2019

View reviewed changes

Engine/PSScriptAnalyzer.psm1 Outdated Show resolved Hide resolved

rjmholt requested review from TylerLeonhardt and daxian-dbw February 15, 2019 02:54

adityapatwardhan reviewed Feb 15, 2019

View reviewed changes

JamesWTruher reviewed Feb 15, 2019

View reviewed changes

bergmeister reviewed Feb 27, 2019

View reviewed changes

Robert Holt added 3 commits February 27, 2019 12:01

Fix signing XML

3bd9b68

Correct framework signing to net452

811b19c

Remove AuthenticodeDual

f809e97

bergmeister requested changes Feb 27, 2019

View reviewed changes

Update signing for PSCompatibilityAnalyzer

de673f0

Robert Holt added 2 commits February 27, 2019 15:34

Update VSCode search exclusion for profiles

a838929

Fix loading problem in PowerShell 5

f964820

Fix Add-Type Newtonsoft in PS 5

a3f8c22

JamesWTruher approved these changes Feb 28, 2019

View reviewed changes

bergmeister approved these changes Feb 28, 2019

View reviewed changes

rjmholt mentioned this pull request Feb 28, 2019

Migrate away from MEF for loading DLLs in Windows PowerShell #1154

Open

SteveL-MSFT requested changes Feb 28, 2019

View reviewed changes

rjmholt requested a review from SteveL-MSFT February 28, 2019 21:12

Add missing copyright headers

245fd55

SteveL-MSFT approved these changes Feb 28, 2019

View reviewed changes

Add other copyright headers

4a62b0b

JamesWTruher merged commit 7d850bb into PowerShell:development Feb 28, 2019

rjmholt mentioned this pull request Feb 28, 2019

End support for PowerShell v3/v4 in VSCode-PowerShell v2.0.0 PowerShell/vscode-powershell#1310

Closed

bergmeister reviewed Mar 1, 2019

View reviewed changes

JamesWTruher mentioned this pull request Mar 10, 2019

WIP: add UseCompatibleTypes Rule, including related resources. Refactor UseCompatibleCmdlets Rule. #960

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New PowerShell compatibility rules #1133

New PowerShell compatibility rules #1133

rjmholt commented Jan 29, 2019 •

edited

Loading

bergmeister commented Jan 29, 2019 •

edited

Loading

rjmholt commented Jan 29, 2019 •

edited

Loading

SteveL-MSFT left a comment

rjmholt left a comment

rjmholt Feb 5, 2019

rjmholt Feb 5, 2019

bergmeister left a comment •

edited

Loading

bergmeister left a comment •

edited

Loading

rjmholt commented Feb 7, 2019

rjmholt commented Feb 8, 2019

rjmholt commented Feb 8, 2019

bergmeister commented Feb 11, 2019 •

edited

Loading

rjmholt commented Feb 13, 2019

rjmholt commented Feb 13, 2019

rjmholt commented Feb 13, 2019

rjmholt commented Feb 15, 2019 •

edited

Loading

adityapatwardhan Feb 15, 2019

adityapatwardhan Feb 15, 2019

rjmholt Feb 26, 2019

JamesWTruher left a comment

rjmholt commented Feb 27, 2019 •

edited

Loading

bergmeister left a comment •

edited

Loading

bergmeister left a comment •

edited

Loading

rjmholt commented Feb 27, 2019

rjmholt commented Feb 27, 2019 •

edited

Loading

rjmholt commented Feb 27, 2019

rjmholt commented Feb 27, 2019

JamesWTruher left a comment

JamesWTruher commented Feb 28, 2019

bergmeister commented Feb 28, 2019

SteveL-MSFT Feb 28, 2019

SteveL-MSFT Feb 28, 2019

SteveL-MSFT left a comment

bergmeister Mar 1, 2019

		@@ -0,0 +1,1610 @@
		# Copyright (c) Microsoft Corporation. All rights reserved.


		public string Id { get; }

		public HashSet<string> ConstituentProfiles { get; }

New PowerShell compatibility rules #1133

New PowerShell compatibility rules #1133

Conversation

rjmholt commented Jan 29, 2019 • edited Loading

PR Summary

Done since opening PR

PR Checklist

bergmeister commented Jan 29, 2019 • edited Loading

rjmholt commented Jan 29, 2019 • edited Loading

SteveL-MSFT left a comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

rjmholt commented Feb 7, 2019

rjmholt commented Feb 8, 2019

rjmholt commented Feb 8, 2019

bergmeister commented Feb 11, 2019 • edited Loading

rjmholt commented Feb 13, 2019

rjmholt commented Feb 13, 2019

rjmholt commented Feb 13, 2019

rjmholt commented Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher left a comment

Choose a reason for hiding this comment

rjmholt commented Feb 27, 2019 • edited Loading

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

rjmholt commented Feb 27, 2019

rjmholt commented Feb 27, 2019 • edited Loading

rjmholt commented Feb 27, 2019

rjmholt commented Feb 27, 2019

JamesWTruher left a comment

Choose a reason for hiding this comment

JamesWTruher commented Feb 28, 2019

bergmeister commented Feb 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Jan 29, 2019 •

edited

Loading

bergmeister commented Jan 29, 2019 •

edited

Loading

rjmholt commented Jan 29, 2019 •

edited

Loading

bergmeister left a comment •

edited

Loading

bergmeister left a comment •

edited

Loading

bergmeister commented Feb 11, 2019 •

edited

Loading

rjmholt commented Feb 15, 2019 •

edited

Loading

rjmholt commented Feb 27, 2019 •

edited

Loading

bergmeister left a comment •

edited

Loading

bergmeister left a comment •

edited

Loading

rjmholt commented Feb 27, 2019 •

edited

Loading