Skip to content

Fix AvoidDefaultValueForMandatoryParameter documentation, rule and tests #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions RuleDocumentation/AvoidDefaultValueForMandatoryParameter.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,35 @@

## Description

Just like non-global scoped variables, parameters must have a default value if they are not mandatory, i.e `Mandatory=$false`.
Having optional parameters without default values leads to uninitialized variables leading to potential bugs.

## How

Specify a default value for all parameters that are not mandatory.
Mandatory parameters should not have a default values because there is no scenario where the default can be used because `PowerShell` will prompt anyway if the parameter value is not specified when calling the function.

## Example

### Wrong

``` PowerShell
function Test($Param1)
function Test
{
$Param1

[CmdletBinding()]
Param
(
[Parameter(Mandatory=$true)]
$Parameter1 = 'default Value'
)
}
```

### Correct

``` PowerShell
function Test($Param1 = $null)
function Test
{
$Param1
[CmdletBinding()]
Param
(
[Parameter(Mandatory=$true)]
$Parameter1
)
}
```
25 changes: 4 additions & 21 deletions Rules/AvoidDefaultValueForMandatoryParameter.cs
Original file line number Diff line number Diff line change
@@ -1,19 +1,8 @@
//
// Copyright (c) Microsoft Corporation.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Linq;
using System.Collections.Generic;
using System.Management.Automation;
using System.Management.Automation.Language;
#if !CORECLR
using System.ComponentModel.Composition;
Expand All @@ -24,15 +13,15 @@
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
/// <summary>
/// ProvideDefaultParameterValue: Check if any uninitialized variable is used.
/// AvoidDefaultValueForMandatoryParameter: Check if a mandatory parameter does not have a default value.
/// </summary>
#if !CORECLR
[Export(typeof(IScriptRule))]
#endif
public class AvoidDefaultValueForMandatoryParameter : IScriptRule
{
/// <summary>
/// AnalyzeScript: Check if any uninitialized variable is used.
/// AnalyzeScript: Check if a mandatory parameter has a default value.
/// </summary>
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
Expand All @@ -46,12 +35,6 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
if (funcAst.Body != null && funcAst.Body.ParamBlock != null
&& funcAst.Body.ParamBlock.Attributes != 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)))
{
continue;
}

foreach (var paramAst in funcAst.Body.ParamBlock.Parameters)
{
bool mandatory = false;
Expand Down
42 changes: 0 additions & 42 deletions Tests/Rules/AvoidDefaultValueForMandatoryParameter.ps1

This file was deleted.

22 changes: 12 additions & 10 deletions Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1
Original file line number Diff line number Diff line change
@@ -1,24 +1,26 @@
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"
$ruleName = 'PSAvoidDefaultValueForMandatoryParameter'

Describe "AvoidDefaultValueForMandatoryParameter" {
Context "When there are violations" {
It "has 1 provide default value for mandatory parameter violation" {
It "has 1 provide default value for mandatory parameter violation with CmdletBinding" {
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ [CmdletBinding()]Param([Parameter(Mandatory)]$Param1=''defaultValue'') }' |
Where-Object { $_.RuleName -eq $ruleName }
$violations.Count | Should -Be 1
}

It "has the correct description message" {
$violations[0].Message | Should -Match $violationMessage
It "has 1 provide default value for mandatory=$true parameter violation without CmdletBinding" {
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$true)]$Param1=''defaultValue'') }' |
Where-Object { $_.RuleName -eq $ruleName }
$violations.Count | Should -Be 1
}
}

Context "When there are no violations" {
It "returns no violations" {
$noViolations.Count | Should -Be 0
It "has 1 provide default value for mandatory parameter violation" {
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$false)]$Param1=''val1'', [Parameter(Mandatory)]$Param2=''val2'', $Param3=''val3'') }' |
Where-Object { $_.RuleName -eq $ruleName }
$violations.Count | Should -Be 0
}
}
}
19 changes: 0 additions & 19 deletions Tests/Rules/AvoidDefaultValueForMandatoryParameterNoViolations.ps1

This file was deleted.