Merge pull request #382 from PowerShell/ProvideDefaultValueForMandatoryParameter

Change Provide Default Parameter Value to Avoid Default Value For Mandatory Parameter

Author: Quoc Truong

Rules/ProvideDefaultParameterValue.cs renamed to Rules/AvoidDefaultValueForMandatoryParameter.cs

@@ -13,18 +13,19 @@
 using System;
 using System.Linq;
 using System.Collections.Generic;
+using System.Management.Automation;
 using System.Management.Automation.Language;
-using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
 using System.ComponentModel.Composition;
 using System.Globalization;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 {
     /// <summary>
     /// ProvideDefaultParameterValue: Check if any uninitialized variable is used.
     /// </summary>
     [Export(typeof(IScriptRule))]
-    public class ProvideDefaultParameterValue : IScriptRule
+    public class AvoidDefaultValueForMandatoryParameter : IScriptRule
     {
         /// <summary>
         /// AnalyzeScript: Check if any uninitialized variable is used.
@@ -36,48 +37,53 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
             // Finds all functionAst
             IEnumerable<Ast> functionAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true);
 
-            // Checks whether this is a dsc resource file (we don't raise this rule for get, set and test-target resource
-            bool isDscResourceFile = !String.IsNullOrWhiteSpace(fileName) && Helper.Instance.IsDscResourceModule(fileName);
-
-            List<string> targetResourcesFunctions = new List<string>(new string[] { "get-targetresource", "set-targetresource", "test-targetresource" });
-
-
             foreach (FunctionDefinitionAst funcAst in functionAsts)
             {
-                // Finds all ParamAsts.
-                IEnumerable<Ast> varAsts = funcAst.FindAll(testAst => testAst is VariableExpressionAst, true);
-
-                // Iterrates all ParamAsts and check if their names are on the list.
-
-                HashSet<string> dscVariables = new HashSet<string>();
-                if (isDscResourceFile && targetResourcesFunctions.Contains(funcAst.Name, StringComparer.OrdinalIgnoreCase))
+                if (funcAst.Body != null && funcAst.Body.ParamBlock != null
+                    && funcAst.Body.ParamBlock.Attributes != null && funcAst.Body.ParamBlock.Parameters != null)
                 {
-                    // don't raise the rules for variables in the param block.
-                    if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null)
+                    // only raise this rule for function with cmdletbinding
+                    if (!funcAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute)))
                     {
-                        dscVariables.UnionWith(funcAst.Body.ParamBlock.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath));
+                        continue;
                     }
-                }
-                // only raise the rules for variables in the param block.
-                if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null)
-                {
+
                     foreach (var paramAst in funcAst.Body.ParamBlock.Parameters)
                     {
-                        if (Helper.Instance.IsUninitialized(paramAst.Name, funcAst) && !dscVariables.Contains(paramAst.Name.VariablePath.UserPath))
+                        bool mandatory = false;
+
+                        // check that param is mandatory
+                        foreach (var paramAstAttribute in paramAst.Attributes)
                         {
-                            yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueError, paramAst.Name.VariablePath.UserPath),
-                            paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName, paramAst.Name.VariablePath.UserPath);
+                            if (paramAstAttribute is AttributeAst)
+                            {
+                                var namedArguments = (paramAstAttribute as AttributeAst).NamedArguments;
+                                if (namedArguments != null)
+                                {
+                                    foreach (NamedAttributeArgumentAst namedArgument in namedArguments)
+                                    {
+                                        if (String.Equals(namedArgument.ArgumentName, "mandatory", StringComparison.OrdinalIgnoreCase))
+                                        {
+                                            // 2 cases: [Parameter(Mandatory)] and [Parameter(Mandatory=$true)]
+                                            if (namedArgument.ExpressionOmitted || (!namedArgument.ExpressionOmitted && String.Equals(namedArgument.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase)))
+                                            {
+                                                mandatory = true;
+                                                break;
+                                            }
+                                        }
+                                    }
+                                }
+                            }
                         }
-                    }
-                }
 
-                if (funcAst.Parameters != null)
-                {
-                    foreach (var paramAst in funcAst.Parameters)
-                    {
-                        if (Helper.Instance.IsUninitialized(paramAst.Name, funcAst) && !dscVariables.Contains(paramAst.Name.VariablePath.UserPath))
+                        if (!mandatory)
+                        {
+                            break;
+                        }
+
+                        if (paramAst.DefaultValue != null)
                         {
-                            yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueError, paramAst.Name.VariablePath.UserPath),
+                            yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterError, paramAst.Name.VariablePath.UserPath),
                             paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName, paramAst.Name.VariablePath.UserPath);
                         }
                     }
@@ -91,7 +97,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         /// <returns>The name of this rule</returns>
         public string GetName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ProvideDefaultParameterValueName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidDefaultValueForMandatoryParameterName);
         }
 
         /// <summary>
@@ -100,7 +106,7 @@ public string GetName()
         /// <returns>The common name of this rule</returns>
         public string GetCommonName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueCommonName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterCommonName);
         }
 
         /// <summary>
@@ -109,7 +115,7 @@ public string GetCommonName()
         /// <returns>The description of this rule</returns>
         public string GetDescription()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueDescription);
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterDescription);
         }
 
         /// <summary>

Rules/ScriptAnalyzerBuiltinRules.csproj

@@ -59,7 +59,7 @@
     <Compile Include="AvoidReservedParams.cs" />
     <Compile Include="AvoidShouldContinueWithoutForce.cs" />
     <Compile Include="AvoidUsingDeprecatedManifestFields.cs" />
-    <Compile Include="ProvideDefaultParameterValue.cs" />
+    <Compile Include="AvoidDefaultValueForMandatoryParameter.cs" />
     <Compile Include="AvoidUsernameAndPasswordParams.cs" />
     <Compile Include="AvoidUsingComputerNameHardcoded.cs" />
     <Compile Include="AvoidUsingConvertToSecureStringWithPlainText.cs" />

Rules/Strings.Designer.cs

@@ -711,17 +711,17 @@
   <data name="DscExamplesPresentNoExamplesError" xml:space="preserve">
     <value>No examples found for resource '{0}'</value>
   </data>
-  <data name="ProvideDefaultParameterValueCommonName" xml:space="preserve">
-    <value>Default Parameter Values</value>
+  <data name="AvoidDefaultValueForMandatoryParameterCommonName" xml:space="preserve">
+    <value>Avoid Default Value For Mandatory Parameter</value>
   </data>
-  <data name="ProvideDefaultParameterValueDescription" xml:space="preserve">
-    <value>Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters</value>
+  <data name="AvoidDefaultValueForMandatoryParameterDescription" xml:space="preserve">
+    <value>Mandatory parameter should not be initialized with a default value in the param block because this value will be ignored.. To fix a violation of this rule, please avoid initializing a value for the mandatory parameter in the param block.</value>
   </data>
-  <data name="ProvideDefaultParameterValueError" xml:space="preserve">
-    <value>Parameter '{0}' is not initialized. Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters</value>
+  <data name="AvoidDefaultValueForMandatoryParameterError" xml:space="preserve">
+    <value>Mandatory Parameter '{0}' is initialized in the Param block. To fix a violation of this rule, please leave it unintialized.</value>
   </data>
-  <data name="ProvideDefaultParameterValueName" xml:space="preserve">
-    <value>ProvideDefaultParameterValue</value>
+  <data name="AvoidDefaultValueForMandatoryParameterName" xml:space="preserve">
+    <value>AvoidDefaultValueForMandatoryParameter</value>
   </data>
   <data name="AvoidUsingDeprecatedManifestFieldsCommonName" xml:space="preserve">
     <value>Avoid Using Deprecated Manifest Fields</value>

Rules/Strings.resx

@@ -6,8 +6,17 @@ Param(
 function SuppressMe ()
 {
     [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideCommentHelp")]
-    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "unused1")]
-    Param([string]$unUsed1, [int] $unUsed2)
+    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "unused1")]
+    [CmdletBinding()]
+    Param(
+        [Parameter(Mandatory=$true)]
+        [string]
+        $unUsed1="unused",
+        
+        [Parameter(Mandatory=$true)]
+        [int]
+        $unUsed2=3
+        )
     {
         Write-Host "I do nothing"
     }
@@ -16,9 +25,18 @@ function SuppressMe ()
 
 function SuppressTwoVariables()
 {
-    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "b")]
-    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "a")]
-    Param([string]$a, [int]$b)
+    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "b")]
+    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "a")]
+    [CmdletBinding()]
+    Param(
+        [Parameter(Mandatory=$true)]
+        [string]
+        $a="unused",
+
+        [Parameter(Mandatory=$true)]
+        [int]
+        $b=3
+        )
     {
     }
 }

Tests/Engine/RuleSuppression.ps1

@@ -49,9 +49,9 @@ Describe "RuleSuppressionWithoutScope" {
 
     Context "RuleSuppressionID" {
         It "Only suppress violations for that ID" {
-            $suppression = $violations | Where-Object {$_.RuleName -eq "PSProvideDefaultParameterValue" }
+            $suppression = $violations | Where-Object {$_.RuleName -eq "PSAvoidDefaultValueForMandatoryParameter" }
             $suppression.Count | Should Be 1
-            $suppression = $violationsUsingScriptDefinition | Where-Object {$_.RuleName -eq "PSProvideDefaultParameterValue" }
+            $suppression = $violationsUsingScriptDefinition | Where-Object {$_.RuleName -eq "PSAvoidDefaultValueForMandatoryParameter" }
             $suppression.Count | Should Be 1
         }
     }

Tests/Engine/RuleSuppression.tests.ps1

@@ -0,0 +1,42 @@
+function BadFunc
+{
+    [CmdletBinding()]
+    param(
+        # this one has default value
+        [Parameter(Mandatory=$true)]
+        [ValidateNotNullOrEmpty()]    
+        [string]
+        $Param1="String",
+        # this parameter has no default value
+        [Parameter(Mandatory=$false)]
+        [ValidateNotNullOrEmpty()]    
+        [string]
+        $Param2
+    )
+    $Param1
+    $Param1 = "test"
+}
+
+function GoodFunc1($Param1)
+{
+    $Param1
+}
+
+# same as BadFunc but this one has no cmdletbinding
+function GoodFunc2
+{
+    param(
+        # this one has default value
+        [Parameter(Mandatory=$true)]
+        [ValidateNotNullOrEmpty()]    
+        [string]
+        $Param1="String",
+        # this parameter has no default value
+        [Parameter(Mandatory=$false)]
+        [ValidateNotNullOrEmpty()]    
+        [string]
+        $Param2
+    )
+    $Param1
+    $Param1 = "test"
+}

Tests/Rules/AvoidDefaultValueForMandatoryParameter.ps1

@@ -0,0 +1,24 @@
+Import-Module PSScriptAnalyzer
+$violationName = "PSAvoidDefaultValueForMandatoryParameter"
+$violationMessage = "Mandatory Parameter 'Param1' is initialized in the Param block. To fix a violation of this rule, please leave it unintialized."
+$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
+$violations = Invoke-ScriptAnalyzer "$directory\AvoidDefaultValueForMandatoryParameter.ps1" | Where-Object {$_.RuleName -match $violationName}
+$noViolations = Invoke-ScriptAnalyzer "$directory\AvoidDefaultValueForMandatoryParameterNoViolations.ps1"
+
+Describe "AvoidDefaultValueForMandatoryParameter" {
+    Context "When there are violations" {
+        It "has 1 provide default value for mandatory parameter violation" {
+            $violations.Count | Should Be 1
+        }
+
+        It "has the correct description message" {
+            $violations[0].Message | Should Match $violationMessage
+        }
+    }
+
+    Context "When there are no violations" {
+        It "returns no violations" {
+            $noViolations.Count | Should Be 0
+        }
+    }
+}