Merge pull request #647 from PowerShell/kapilmb/FixUseCompatibleCmdlets

Fix UseCompatibleCmdlets rule

Author: Kapil Borle

CHANGELOG.MD

@@ -1,6 +1,12 @@
 ## [unreleased](https://github.com/PowerShell/PSScriptAnalyzer/tree/development)
 ### Fixed
 - Regular expression in `target` parameter in SuppressMessageAttribute (#638)
+- Filtering on severity level for DSC rules (#642)
+- PSUseCompatibleCmdlets rule
+    + to check for commands in Microsoft.PowerShell.Core snapin
+    + to ignore aliases
+    + to ignore custom defind commands
+
 
 ## [1.8.1](https://github.com/PowerShell/PSScriptAnalyzer/tree/1.8.1) - 2016-10-13
 ### Added

Engine/Settings/core-6.0.0-alpha-linux.json

@@ -33,6 +33,13 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 #endif
     public class UseCompatibleCmdlets : AstVisitor, IScriptRule
     {
+        private struct RuleParameters
+        {
+            public string mode;
+            public string[] compatibility;
+            public string reference;
+        }
+
         private List<DiagnosticRecord> diagnosticRecords;
         private Dictionary<string, HashSet<string>> psCmdletMap;
         private readonly List<string> validParameters;
@@ -41,10 +48,14 @@ public class UseCompatibleCmdlets : AstVisitor, IScriptRule
         private Dictionary<string, dynamic> platformSpecMap;
         private string scriptPath;
         private bool IsInitialized;
+        private bool hasInitializationError;
+        private string reference;
+        private readonly string defaultReference = "desktop-5.1.14393.206-windows";
+        private RuleParameters ruleParameters;
 
         public UseCompatibleCmdlets()
         {
-            validParameters = new List<string> { "mode", "uri", "compatibility" };
+            validParameters = new List<string> { "mode", "uri", "compatibility", "reference" };
             IsInitialized = false;
         }
 
@@ -124,6 +135,11 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                 Initialize();
             }
 
+            if (hasInitializationError)
+            {
+                yield break;
+            }
+
             if (ast == null)
             {
                 throw new ArgumentNullException("ast");
@@ -168,12 +184,22 @@ public override AstVisitAction VisitCommand(CommandAst commandAst)
         /// </summary>
         private void GenerateDiagnosticRecords()
         {
-            foreach (var curCmdletCompat in curCmdletCompatibilityMap)
+            bool referenceCompatibility = curCmdletCompatibilityMap[reference];
+
+            // If the command is present in reference platform but not in any of the target platforms.
+            // Or if the command is not present in reference platform but present in any of the target platforms
+            // then declare it as an incompatible cmdlet.
+            // If it is present neither in reference platform nor any target platforms, then it is probably a
+            // non-builtin command and hence do not declare it as an incompatible cmdlet.
+            // Since we do not check for aliases, the XOR-ing will also make sure that aliases are not flagged
+            // as they will be found neither in reference platform nor in target platforms
+            foreach (var platform in ruleParameters.compatibility)
             {
-                if (!curCmdletCompat.Value)
+                var curCmdletCompat = curCmdletCompatibilityMap[platform];
+                if (!curCmdletCompat && referenceCompatibility)
                 {
                     var cmdletName = curCmdletAst.GetCommandName();
-                    var platformInfo = platformSpecMap[curCmdletCompat.Key];
+                    var platformInfo = platformSpecMap[platform];
                     var funcNameTokens = Helper.Instance.Tokens.Where(
                                                 token =>
                                                 Helper.ContainsExtent(curCmdletAst.Extent, token.Extent)
@@ -215,6 +241,9 @@ private void Initialize()
         /// </summary>
         private void SetupCmdletsDictionary()
         {
+            // If the method encounters any error, it returns early
+            // which implies there is an initialization error
+            hasInitializationError = true;
             Dictionary<string, object> ruleArgs = Helper.Instance.GetRuleArguments(GetName());
             if (ruleArgs == null)
             {
@@ -251,45 +280,93 @@ private void SetupCmdletsDictionary()
                 }
             }
 
-            foreach (var compat in compatibilityList)
+            ruleParameters.compatibility = compatibilityList.ToArray();
+            reference = defaultReference;
+#if DEBUG
+            // Setup reference file
+            object referenceObject;
+            if (ruleArgs.TryGetValue("reference", out referenceObject))
             {
-                string psedition, psversion, os;
-
-                // ignore (warn) invalid entries
-                if (GetVersionInfoFromPlatformString(compat, out psedition, out psversion, out os))
+                reference = referenceObject as string;
+                if (reference == null)
                 {
-                    platformSpecMap.Add(compat, new { PSEdition = psedition, PSVersion = psversion, OS = os });
-                    curCmdletCompatibilityMap.Add(compat, true);
+                    reference = GetStringArgFromListStringArg(referenceObject);
+                    if (reference == null)
+                    {
+                        return;
+                    }
                 }
             }
+#endif
+            ruleParameters.reference = reference;
+
+            // check if the reference file has valid platformSpec
+            if (!IsValidPlatformString(reference))
+            {
+                return;
+            }
 
+            string settingsPath;
+            settingsPath = GetShippedSettingsDirectory();
+#if DEBUG
             object modeObject;
             if (ruleArgs.TryGetValue("mode", out modeObject))
             {
                 // This is for testing only. User should not be specifying mode!
                 var mode = GetStringArgFromListStringArg(modeObject);
+                ruleParameters.mode = mode;
                 switch (mode)
                 {
                     case "offline":
-                        ProcessOfflineModeArgs(ruleArgs);
+                        settingsPath = GetStringArgFromListStringArg(ruleArgs["uri"]);
                         break;
 
                     case "online": // not implemented yet.
                     case null:
                     default:
-                        break;
+                        return;
                 }
 
+            }
+#endif
+            if (settingsPath == null
+                || !ContainsReferenceFile(settingsPath))
+            {
                 return;
             }
 
-            var settingsPath = GetSettingsDirectory();
-            if (settingsPath == null)
+            var extentedCompatibilityList = compatibilityList.Concat(Enumerable.Repeat(reference, 1));
+            foreach (var compat in extentedCompatibilityList)
+            {
+                string psedition, psversion, os;
+
+                // ignore (warn) invalid entries
+                if (GetVersionInfoFromPlatformString(compat, out psedition, out psversion, out os))
+                {
+                    platformSpecMap.Add(compat, new { PSEdition = psedition, PSVersion = psversion, OS = os });
+                    curCmdletCompatibilityMap.Add(compat, true);
+                }
+            }
+
+            ProcessDirectory(
+                settingsPath,
+                extentedCompatibilityList);
+            if (psCmdletMap.Keys.Count != extentedCompatibilityList.Count())
             {
                 return;
             }
 
-            ProcessDirectory(settingsPath);
+            // reached this point, so no error
+            hasInitializationError = false;
+        }
+
+        /// <summary>
+        /// Checks if the given directory has the reference file
+        /// directory must be non-null
+        /// </summary>
+        private bool ContainsReferenceFile(string directory)
+        {
+            return File.Exists(Path.Combine(directory, reference + ".json"));
         }
 
         /// <summary>
@@ -307,7 +384,7 @@ private void ResetCurCmdletCompatibilityMap()
         /// <summary>
         /// Retrieves the Settings directory from the Module directory structure
         /// </summary>
-        private string GetSettingsDirectory()
+        private string GetShippedSettingsDirectory()
         {
             // Find the compatibility files in Settings folder
             var path = this.GetType().GetTypeInfo().Assembly.Location;
@@ -332,6 +409,16 @@ private string GetSettingsDirectory()
             return settingsPath;
         }
 
+        private bool IsValidPlatformString(string fileNameWithoutExt)
+        {
+            string psedition, psversion, os;
+            return GetVersionInfoFromPlatformString(
+                fileNameWithoutExt,
+                out psedition,
+                out psversion,
+                out os);
+        }
+
         /// <summary>
         /// Gets PowerShell Edition, Version and OS from input string
         /// </summary>
@@ -375,30 +462,10 @@ private string GetStringArgFromListStringArg(object arg)
             return strList[0];
         }
 
-        /// <summary>
-        /// Process arguments when 'offline' mode is specified
-        /// </summary>
-        private void ProcessOfflineModeArgs(Dictionary<string, object> ruleArgs)
-        {
-            var uri = GetStringArgFromListStringArg(ruleArgs["uri"]);
-            if (uri == null)
-            {
-                // TODO: log this
-                return;
-            }
-            if (!Directory.Exists(uri))
-            {
-                // TODO: log this
-                return;
-            }
-
-            ProcessDirectory(uri);
-        }
-
         /// <summary>
         /// Search a directory for files of form [PSEdition]-[PSVersion]-[OS].json
         /// </summary>
-        private void ProcessDirectory(string path)
+        private void ProcessDirectory(string path, IEnumerable<string> acceptablePlatformSpecs)
         {
             foreach (var filePath in Directory.EnumerateFiles(path))
             {
@@ -410,36 +477,14 @@ private void ProcessDirectory(string path)
                 }
 
                 var fileNameWithoutExt = Path.GetFileNameWithoutExtension(filePath);
-                if (!platformSpecMap.ContainsKey(fileNameWithoutExt))
+                if (acceptablePlatformSpecs != null
+                    && !acceptablePlatformSpecs.Contains(fileNameWithoutExt, StringComparer.OrdinalIgnoreCase))
                 {
                     continue;
                 }
 
                 psCmdletMap[fileNameWithoutExt] = GetCmdletsFromData(JObject.Parse(File.ReadAllText(filePath)));
             }
-
-            RemoveUnavailableKeys();
-        }
-
-        /// <summary>
-        /// Remove keys that are not present in psCmdletMap but present in platformSpecMap and curCmdletCompatibilityMap
-        /// </summary>
-        private void RemoveUnavailableKeys()
-        {
-            var keysToRemove = new List<string>();
-            foreach (var key in platformSpecMap.Keys)
-            {
-                if (!psCmdletMap.ContainsKey(key))
-                {
-                    keysToRemove.Add(key);
-                }
-            }
-
-            foreach (var key in keysToRemove)
-            {
-                platformSpecMap.Remove(key);
-                curCmdletCompatibilityMap.Remove(key);
-            }
         }
 
         /// <summary>
@@ -451,11 +496,20 @@ private HashSet<string> GetCmdletsFromData(dynamic deserializedObject)
         {
             var cmdlets = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
             dynamic modules = deserializedObject.Modules;
-            foreach (var module in modules)
+            foreach (dynamic module in modules)
             {
-                foreach (var cmdlet in module.ExportedCommands)
+                if (module.ExportedCommands == null)
                 {
-                    var name = cmdlet.Name.Value as string;
+                    continue;
+                }
+
+                foreach (dynamic cmdlet in module.ExportedCommands)
+                {
+                    var name = cmdlet.Name as string;
+                    if (name == null)
+                    {
+                        name = cmdlet.Name.ToObject<string>();
+                    }
                     cmdlets.Add(name);
                 }
             }

Engine/Settings/core-6.0.0-alpha-osx.json

@@ -38,7 +38,15 @@ Function Test-PSEditionCoreCLRLinux
     (Test-PSEditionCoreCLR) -and $IsLinux
 }
 
+Function Get-Count
+{
+	Begin {$count = 0}
+	Process {$count++}
+	End {$count}
+}
+
 Export-ModuleMember -Function Get-ExtentText
 Export-ModuleMember -Function Test-CorrectionExtent
 Export-ModuleMember -Function Test-PSEditionCoreCLR
-Export-ModuleMember -Function Test-PSEditionCoreCLRLinux
+Export-ModuleMember -Function Test-PSEditionCoreCLRLinux
+Export-ModuleMember -Function Get-Count

Engine/Settings/core-6.0.0-alpha-windows.json

@@ -1,8 +1,11 @@
-Import-Module PSScriptAnalyzer
-$ruleName = "PSUseCompatibleCmdlets"
+$ruleName = "PSUseCompatibleCmdlets"
 $directory = Split-Path $MyInvocation.MyCommand.Path -Parent
+$testRootDirectory = Split-Path -Parent $directory
 $ruleTestDirectory = Join-Path $directory 'UseCompatibleCmdlets'
 
+Import-Module PSScriptAnalyzer
+Import-Module (Join-Path $testRootDirectory 'PSScriptAnalyzerTestHelper.psm1')
+
 Describe "UseCompatibleCmdlets" {
     Context "script has violation" {
         It "detects violation" {
@@ -12,4 +15,44 @@ Describe "UseCompatibleCmdlets" {
             $diagnosticRecords.Count | Should Be 1
         }
     }
+
+    Function Test-Command
+    {
+        param (
+            [Parameter(ValueFromPipeline)]
+            $command,
+            $settings,
+            $expectedViolations
+        )
+        process
+        {
+            It ("found {0} violations for '{1}'" -f $expectedViolations, $command) {
+                Invoke-ScriptAnalyzer -ScriptDefinition $command -IncludeRule $ruleName -Settings $settings | `
+                    Get-Count | `
+                    Should Be $expectedViolations
+            }
+        }
+    }
+
+    $settings = @{rules=@{PSUseCompatibleCmdlets=@{compatibility=@("core-6.0.0-alpha-windows")}}}
+
+    Context "Microsoft.PowerShell.Core" {
+         @('Enter-PSSession', 'Foreach-Object', 'Get-Command') | `
+            Test-Command -Settings $settings -ExpectedViolations 0
+    }
+
+    Context "Non-builtin commands" {
+        @('get-foo', 'get-bar', 'get-baz') | `
+            Test-Command -Settings $settings -ExpectedViolations 0
+    }
+
+    Context "Aliases" {
+        @('where', 'select', 'cd') | `
+            Test-Command -Settings $settings -ExpectedViolations 0
+    }
+
+    Context "Commands present in reference platform but not in target platform" {
+        @("Start-VM", "New-SmbShare", "Get-Disk") | `
+            Test-Command -Settings $settings -ExpectedViolations 1
+    }
 }