Skip to content

add AvoidMultipleTypesParameter Rule #1705

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
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
50 changes: 50 additions & 0 deletions RuleDocumentation/AvoidMultipleTypesParameter.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# AvoidMultipleTypesParameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# AvoidMultipleTypesParameter
# AvoidMultipleTypeAttributes


**Severity Level: Warning**

## Description

Parameter should not have more than one type specifier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Parameter should not have more than one type specifier.
Parameters should not have more than one type specifier.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a description why we do not want to have more than one type, i.e. what is the impact? \Just a runtime error or potentially also unpredictable or unintuitive behavior?


## How

Make each parameter has only 1 type spcifier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Make each parameter has only 1 type spcifier.
Ensure each parameter has only 1 type specifier.


## Example

### Wrong

``` PowerShell
function Test-Script
{
[CmdletBinding()]
Param
(
[String]
$Param1,

[switch]
[boolean]
$Switch=$True
)
...
}
```

### Correct

``` PowerShell
function Test-Script
{
[CmdletBinding()]
Param
(
[String]
$Param1,

[switch]
$Switch=$False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$Switch=$False
$Switch

)
...
}
```
1 change: 1 addition & 0 deletions RuleDocumentation/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
|[AvoidGlobalVars](./AvoidGlobalVars.md) | Warning | |
|[AvoidInvokingEmptyMembers](./AvoidInvokingEmptyMembers.md) | Warning | |
|[AvoidLongLines](./AvoidLongLines.md) | Warning | |
|[AvoidMultipleTypesParameter](./AvoidMultipleTypesParameter.md) | Warning | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|[AvoidMultipleTypesParameter](./AvoidMultipleTypesParameter.md) | Warning | |
|[AvoidMultipleTypeAttributes](./AvoidMultipleTypesParameter.md) | Warning | |

|[AvoidOverwritingBuiltInCmdlets](./AvoidOverwritingBuiltInCmdlets.md) | Warning | |
|[AvoidNullOrEmptyHelpMessageAttribute](./AvoidNullOrEmptyHelpMessageAttribute.md) | Warning | |
|[AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | |
Expand Down
98 changes: 98 additions & 0 deletions Rules/AvoidMultipleTypesParameter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Management.Automation.Language;
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
#if !CORECLR
using System.ComponentModel.Composition;
#endif
using System.Globalization;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
/// <summary>
/// AvoidMultipleTypesParameter: Check that parameter does not be assigned to multiple types.
/// </summary>
#if !CORECLR
[Export(typeof(IScriptRule))]
#endif
public sealed class AvoidMultipleTypesParameter : IScriptRule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public sealed class AvoidMultipleTypesParameter : IScriptRule
public sealed class AvoidMultipleTypeAttributesRule: IScriptRule

{
/// <summary>
/// AvoidMultipleTypesParameter: Check that parameter does not be assigned to multiple types.
/// </summary>
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
if (ast is null)
{
throw new ArgumentNullException(Strings.NullAstErrorMessage);
}


// Finds all ParamAsts.
IEnumerable<Ast> paramAsts = ast.FindAll(testAst => testAst is ParameterAst, true);

// Iterates all ParamAsts and check the number of its types.
foreach (ParameterAst paramAst in paramAsts)
{
if (paramAst.Attributes.Where(typeAst => typeAst is TypeConstraintAst).Count() > 1)
{
yield return new DiagnosticRecord(
String.Format(CultureInfo.CurrentCulture, Strings.AvoidMultipleTypesParameterError, paramAst.Name),
paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
yield return new DiagnosticRecord(
String.Format(CultureInfo.CurrentCulture, Strings.AvoidMultipleTypesParameterError, paramAst.Name),
paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
yield return new DiagnosticRecord(
String.Format(CultureInfo.CurrentCulture, Strings.AvoidMultipleTypesParameterError, paramAst.Name),
paramAst.Name.Extent,
GetName(),
DiagnosticSeverity.Warning,
fileName);

}
}
}

/// <summary>
/// GetName: Retrieves the name of this rule.
/// </summary>
/// <returns>The name of this rule</returns>
public string GetName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidMultipleTypesParameterName);
}

/// <summary>
/// GetCommonName: Retrieves the common name of this rule.
/// </summary>
/// <returns>The common name of this rule</returns>
public string GetCommonName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidMultipleTypesParameterCommonName);
}

/// <summary>
/// GetDescription: Retrieves the description of this rule.
/// </summary>
/// <returns>The description of this rule</returns>
public string GetDescription()
{
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidMultipleTypesParameterDescription);
}

/// <summary>
/// GetSourceType: Retrieves the type of the rule, Builtin, Managed or Module.
/// </summary>
public SourceType GetSourceType()
{
return SourceType.Builtin;
}

/// <summary>
/// GetSeverity: Retrieves the severity of the rule: error, warning or information.
/// </summary>
/// <returns></returns>
public RuleSeverity GetSeverity()
{
return RuleSeverity.Warning;
}

/// <summary>
/// GetSourceName: Retrieves the name of the module/assembly the rule is from.
/// </summary>
public string GetSourceName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
}
}
}
38 changes: 37 additions & 1 deletion Rules/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 13 additions & 1 deletion Rules/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -1152,4 +1152,16 @@
<data name="InvalidSyntaxAroundProcessBlockError" xml:space="preserve">
<value>When using an explicit process block, no preceding code is allowed, only begin, end and dynamicparams blocks.</value>
</data>
</root>
<data name="AvoidMultipleTypesParameterCommonName" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<data name="AvoidMultipleTypesParameterCommonName" xml:space="preserve">
<data name="AvoidMultipleTypeAttributesCommonName" xml:space="preserve">

<value>Avoid Multiple Types Parameter</value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<value>Avoid Multiple Types Parameter</value>
<value>Avoid multiple type specifiers on parameters.</value>

</data>
<data name="AvoidMultipleTypesParameterDescription" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<data name="AvoidMultipleTypesParameterDescription" xml:space="preserve">
<data name="AvoidMultipleTypeAttributesDescription" xml:space="preserve">

<value>Prameter should not have more than one type specifier.</value>
</data>
<data name="AvoidMultipleTypesParameterError" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<data name="AvoidMultipleTypesParameterError" xml:space="preserve">
<data name="AvoidMultipleTypeAttributesError" xml:space="preserve">

<value>Parameter '{0}' has more than one type specifier.</value>
</data>
<data name="AvoidMultipleTypesParameterName" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<data name="AvoidMultipleTypesParameterName" xml:space="preserve">
<data name="AvoidMultipleTypeAttributesName" xml:space="preserve">

<value>AvoidMultipleTypesParameter</value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<value>AvoidMultipleTypesParameter</value>
<value>AvoidMultipleTypeAttributes</value>

</data>
</root>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
</root>
</root>

2 changes: 1 addition & 1 deletion Tests/Engine/GetScriptAnalyzerRule.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ Describe "Test Name parameters" {

It "get Rules with no parameters supplied" {
$defaultRules = Get-ScriptAnalyzerRule
$expectedNumRules = 65
$expectedNumRules = 66
if ($PSVersionTable.PSVersion.Major -le 4)
{
# for PSv3 PSAvoidGlobalAliases is not shipped because
Expand Down
7 changes: 7 additions & 0 deletions Tests/Rules/AvoidMultipleTypesParameter.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Double typing is not allowed
function F10 ([int] [switch] $s1, [int] $p1){}

# Double typing is not allowed even for switch and boolean, because:
# switch maps to System.Management.Automation.SwitchParameter
# boolean maps to System.Boolean
function F11 ([switch][boolean] $s1, [int] $p1){}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function F11 ([switch][boolean] $s1, [int] $p1){}
function F11 ([switch][boolean] $s1, [int] $p1){}

25 changes: 25 additions & 0 deletions Tests/Rules/AvoidMultipleTypesParameter.tests.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
BeforeAll {
$violationMessage = 'Parameter ''\$s1'' has more than one type specifier.'
$violationName = "PSAvoidMultipleTypesParameter"
$violations = Invoke-ScriptAnalyzer $PSScriptRoot\AvoidMultipleTypesParameter.ps1 | Where-Object {$_.RuleName -eq $violationName}
$noViolations = Invoke-ScriptAnalyzer $PSScriptRoot\AvoidMultipleTypesParameterNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using files and running the analyzer in BeforeAll, I would:

  • Use inline scripts and use the -ScriptDefinition parameter
  • Use the -TestCases parameter on It to parameterise the diagnostics you expect

}

Describe "AvoidMultipleTypesParameter" {
Context "When there are violations" {
It "has two AvoidMultipleTypesParameter violations" {
$violations.Count | Should -Be 2
}

It "has the correct description message" {
$violations[0].Message | Should -Match $violationMessage
$violations[1].Message | Should -Match $violationMessage
}
}

Context "When there are no violations" {
It "returns no violations" {
$noViolations.Count | Should -Be 0
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

3 changes: 3 additions & 0 deletions Tests/Rules/AvoidMultipleTypesParameterNoViolations.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
function F10 ([int] $s1, [int] $p1){}

function F11 ([switch] $s1, [int] $p1){}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function F11 ([switch] $s1, [int] $p1){}
function F11 ([switch] $s1, [int] $p1){}