Skip to content
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

add AvoidMultipleTypesParameter Rule #1705

Merged
Merged
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

Parameters should not have more than one type specifier. Multiple type specifiers on parameters will cause a runtime error.

## How

Ensure each parameter has only 1 type specifier.

## Example

### Wrong

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

[switch]
[bool]
$Switch
)
...
}
```

### Correct

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

[switch]
$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
104 changes: 104 additions & 0 deletions Rules/AvoidMultipleTypesParameter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// 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 is null)
{
throw new ArgumentNullException(Strings.NullAstErrorMessage);
}

// Finds all ParamAsts.
IEnumerable<Ast> paramAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: 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);
}
}
}

/// <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.

12 changes: 12 additions & 0 deletions 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>
<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 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>
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
42 changes: 42 additions & 0 deletions Tests/Rules/AvoidMultipleTypesParameter.tests.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.

BeforeAll {
$ruleName = "PSAvoidMultipleTypesParameter"
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
$ruleName = "PSAvoidMultipleTypesParameter"
$ruleName = "PSAvoidMultipleTypeAttributes"


$settings = @{
IncludeRules = @($ruleName)
}
}

Describe 'AvoidMultipleTypesParameter' {
Copy link
Contributor

Choose a reason for hiding this comment

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

So I would rewrite your test structure like this:

  • No Contexts
  • Each test scenario is an It
  • Invoke-ScriptAnalyzer is done in the It rather than in a BeforeAll
  • The It tests the count and properties of all the violations

We could also use a few more tests:

  • When no type specifiers are supplied
  • When three are
  • When [ref] is used
  • When an attribute like [ValidateSet()] is also used

It 'Correctly diagnoses and corrects <Script>' -TestCases @(
@{ Script = 'function F1 ($s1, $p1){}' }
@{ Script = 'function F2 ([int] $s2, [int] $p2){}' }
@{ Script = 'function F3 ([int][switch] $s3, [int] $p3){}';Extent = @{ StartCol = 28; EndCol = 31 }; Message = 'Parameter ''$s3'' has more than one type specifier.' }
@{ Script = 'function F4 ([int][ref] $s4, [int] $p4){}';Extent = @{ StartCol = 25; EndCol = 28 }; Message = 'Parameter ''$s4'' has more than one type specifier.' }
@{ Script = 'function F5 ([int][switch][boolean] $s5, [int] $p5){}';Extent = @{ StartCol = 37; EndCol = 40 }; Message = 'Parameter ''$s5'' has more than one type specifier.' }
@{ Script = 'function F6 ([ValidateSet()][int] $s6, [int] $p6){}' }
@{ Script = 'function F7 ([Parameter(Mandatory=$true)][ValidateSet()][int] $s7, [int] $p7){}' }
) {
param([string]$Script, $Extent, $Message)

$diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $Script

if (-not $Extent)
{
$diagnostics | Should -BeNullOrEmpty
return
}

$expectedStartLine = if ($Extent.StartLine) { $Extent.StartLine } else { 1 }
$expectedEndLine = if ($Extent.EndLine) { $Extent.EndLine } else { 1 }

$diagnostics.Extent.StartLineNumber | Should -BeExactly $expectedStartLine
$diagnostics.Extent.EndLineNumber | Should -BeExactly $expectedEndLine
$diagnostics.Extent.StartColumnNumber | Should -BeExactly $Extent.StartCol
$diagnostics.Extent.EndColumnNumber | Should -BeExactly $Extent.EndCol

$diagnostics.Message | Should -BeExactly $Message
}
}