Merge pull request #424 from PowerShell/FixBugs

Fix bugs related variable scopes not being considered

Author: Quoc Truong

Engine/Helper.cs

@@ -100,6 +100,10 @@ internal set
         /// </summary>
         private Dictionary<Ast, VariableAnalysis> VariableAnalysisDictionary;
 
+        private string[] functionScopes = new string[] { "global:", "local:", "script:", "private:" };
+
+        private string[] variableScopes = new string[] { "global:", "local:", "script:", "private:", "variable:", ":" };
+
         #endregion
 
         /// <summary>
@@ -269,6 +273,60 @@ item is TypeDefinitionAst
             return false;
         }
 
+        private string NameWithoutScope(string name, string[] scopes)
+        {
+            if (String.IsNullOrWhiteSpace(name) || scopes == null)
+            {
+                return name;
+            }            
+
+            // checks whether function name starts with scope
+            foreach (string scope in scopes)
+            {
+                // trim the scope part
+                if (name.IndexOf(scope) == 0)
+                {
+                    return name.Substring(scope.Length);
+                }
+            }
+
+            // no scope
+            return name;
+        }
+
+        /// <summary>
+        /// Given a function name, strip the scope of the name
+        /// </summary>
+        /// <param name="functionName"></param>
+        /// <returns></returns>
+        public string FunctionNameWithoutScope(string functionName)
+        {
+            return NameWithoutScope(functionName, functionScopes);
+        }
+
+        /// <summary>
+        /// Given a variable name, strip the scope
+        /// </summary>
+        /// <param name="variableName"></param>
+        /// <returns></returns>
+        public string VariableNameWithoutScope(VariablePath variablePath)
+        {
+            if (variablePath == null || variablePath.UserPath == null)
+            {
+                return null;
+            }
+
+            // strip out the drive if there is one
+            if (!string.IsNullOrWhiteSpace(variablePath.DriveName)
+                // checks that variable starts with drivename:
+                && variablePath.UserPath.IndexOf(string.Concat(variablePath.DriveName, ":")) == 0)
+            {
+                return variablePath.UserPath.Substring(variablePath.DriveName.Length + 1);
+            }
+
+            return NameWithoutScope(variablePath.UserPath, variableScopes);
+        }
+
         /// <summary>
         /// Given a commandast, checks whether it uses splatted variable
         /// </summary>

Engine/VariableAnalysis.cs

@@ -92,10 +92,13 @@ private void ProcessParameters(IEnumerable<ParameterAst> parameters)
                         {
                             foreach (NamedAttributeArgumentAst arg in args)
                             {
-                                if (String.Equals(arg.ArgumentName, "mandatory", StringComparison.OrdinalIgnoreCase)
-                                    && String.Equals(arg.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase))
-                                {
-                                    isSwitchOrMandatory = true;
+                                if (String.Equals(arg.ArgumentName, "mandatory", StringComparison.OrdinalIgnoreCase))
+                               {
+                                    // check for the case mandatory=$true and just mandatory
+                                    if (arg.ExpressionOmitted || (!arg.ExpressionOmitted && String.Equals(arg.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase)))
+                                    {
+                                        isSwitchOrMandatory = true;
+                                    }
                                 }
                             }
                         }

Rules/AvoidReservedCharInCmdlet.cs

@@ -15,6 +15,7 @@
 using System.Linq;
 using System.Management.Automation.Language;
 using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer;
 using System.ComponentModel.Composition;
 using System.Globalization;
 
@@ -43,7 +44,9 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
 
             foreach (FunctionDefinitionAst funcAst in funcAsts)
             {
-                if (funcAst.Name != null && funcAst.Name.Intersect(reservedChars).Count() > 0)
+                string funcName = Helper.Instance.FunctionNameWithoutScope(funcAst.Name);
+
+                if (funcName != null && funcName.Intersect(reservedChars).Count() > 0)
                 {
                     yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ReservedCmdletCharError, funcAst.Name),
                         funcAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);

Rules/UseApprovedVerbs.cs

@@ -54,7 +54,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) {
 
             foreach (FunctionDefinitionAst funcAst in funcAsts)
             {
-                funcName = funcAst.Name;
+                funcName = Helper.Instance.FunctionNameWithoutScope(funcAst.Name);
 
                 if (funcName != null && funcName.Contains('-'))
                 {

Rules/UseDeclaredVarsMoreThanAssignments.cs

@@ -53,13 +53,17 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
 
                     if (assignmentVarAst != null)
                     {
-                        //Ignore if variable is global or environment variable
+                        //Ignore if variable is global or environment variable or scope is function
                         if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst, ast)
-                            && !assignmentVarAst.VariablePath.IsScript)
+                            && !assignmentVarAst.VariablePath.IsScript
+                            && !string.Equals(assignmentVarAst.VariablePath.DriveName, "function", StringComparison.OrdinalIgnoreCase))
                         {
-                            if (!assignments.ContainsKey(assignmentVarAst.VariablePath.UserPath))
+
+                            string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath);
+
+                            if (!assignments.ContainsKey(variableName))
                             {
-                                assignments.Add(assignmentVarAst.VariablePath.UserPath, assignmentAst);
+                                assignments.Add(variableName, assignmentAst);
                             }
                         }
                     }
@@ -70,7 +74,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
             {
                 foreach (VariableExpressionAst varAst in varAsts)
                 {
-                    varKey = varAst.VariablePath.UserPath;
+                    varKey = Helper.Instance.VariableNameWithoutScope(varAst.VariablePath);
                     inAssignment = false;
 
                     if (assignments.ContainsKey(varKey))

Rules/UseShouldProcessCorrectly.cs

@@ -54,7 +54,9 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
 
                 attributeAsts = funcDefAst.FindAll(testAst => testAst is NamedAttributeArgumentAst, true);
                 foreach (NamedAttributeArgumentAst attributeAst in attributeAsts) {
-                    hasShouldProcessAttribute |= attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.Argument.Extent.Text.Equals(trueString, StringComparison.OrdinalIgnoreCase);
+                    hasShouldProcessAttribute |= ((attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.Argument.Extent.Text.Equals(trueString, StringComparison.OrdinalIgnoreCase))
+                        // checks for the case if the user just use the attribute without setting it to true
+                        || (attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.ExpressionOmitted));
                 }
 
                 memberAsts = funcDefAst.FindAll(testAst => testAst is MemberExpressionAst, true);

Tests/Rules/GoodCmdlet.ps1

@@ -107,8 +107,117 @@ function Get-File
     }
 }
 
+<#
+.Synopsis
+   Short description
+.DESCRIPTION
+   Long description
+.EXAMPLE
+   Example of how to use this cmdlet
+.EXAMPLE
+   Another example of how to use this cmdlet
+.INPUTS
+   Inputs to this cmdlet (if any)
+.OUTPUTS
+   Output from this cmdlet (if any)
+.NOTES
+   General notes
+.COMPONENT
+   The component this cmdlet belongs to
+.ROLE
+   The role this cmdlet belongs to
+.FUNCTIONALITY
+   The functionality that best describes this cmdlet
+#>
+function Get-Folder
+{
+    [CmdletBinding(DefaultParameterSetName='Parameter Set 1', 
+                  SupportsShouldProcess, 
+                  PositionalBinding=$false,
+                  HelpUri = 'http://www.microsoft.com/',
+                  ConfirmImpact='Medium')]
+    [Alias()]
+    [OutputType([String], [System.Double], [Hashtable], "MyCustom.OutputType")]
+    [OutputType("System.Int32", ParameterSetName="ID")]
+
+    Param
+    (
+        # Param1 help description
+        [Parameter(Mandatory=$true, 
+                   ValueFromPipeline=$true,
+                   ValueFromPipelineByPropertyName=$true, 
+                   ValueFromRemainingArguments=$false, 
+                   Position=0,
+                   ParameterSetName='Parameter Set 1')]
+        [ValidateNotNull()]
+        [ValidateNotNullOrEmpty()]
+        [ValidateCount(0,5)]
+        [ValidateSet("sun", "moon", "earth")]
+        [Alias("p1")] 
+        $Param1,
+
+        # Param2 help description
+        [Parameter(ParameterSetName='Parameter Set 1')]
+        [AllowNull()]
+        [AllowEmptyCollection()]
+        [AllowEmptyString()]
+        [ValidateScript({$true})]
+        [ValidateRange(0,5)]
+        [int]
+        $Param2,
+
+        # Param3 help description
+        [Parameter(ParameterSetName='Another Parameter Set')]
+        [ValidatePattern("[a-z]*")]
+        [ValidateLength(0,15)]
+        [String]
+        $Param3,
+        [bool]
+        $Force
+    )
+
+    Begin
+    {
+    }
+    Process
+    {
+        if ($pscmdlet.ShouldProcess("Target", "Operation"))
+        {
+            Write-Verbose "Write Verbose"
+            Get-Process
+        }
+
+        $a = 4.5
+
+        if ($true)
+        {
+            $a
+        }
+
+        $a | Write-Warning
+
+        $b = @{"hash"="table"}
+
+        Write-Debug @b
+
+        [pscustomobject]@{
+            PSTypeName = 'MyCustom.OutputType'
+            Prop1 = 'SomeValue'
+            Prop2 = 'OtherValue'
+        }
+
+        return @{"hash"="true"}
+    }
+    End
+    {
+        if ($pscmdlet.ShouldContinue("Yes", "No")) {
+        }
+        [System.Void] $Param3
+    }
+}
+
 #Write-Verbose should not be required because this is not an advanced function
-function Get-SimpleFunc
+function global:Get-SimpleFunc
 {
 
 }

Tests/Rules/UseDeclaredVarsMoreThanAssignmentsNoViolations.ps1

@@ -5,4 +5,14 @@ $foo.property = "This also should not raise errors"
 
 # Output field separator builtin special variable
 $OFS = ', '
-[string]('apple', 'banana', 'orange')
+[string]('apple', 'banana', 'orange')
+
+# Using scope
+$private:x = 42
+$x
+
+$variable:a = 52
+$a
+
+#function
+$function:prompt = [ScriptBlock]::Create($newPrompt)