-
Notifications
You must be signed in to change notification settings - Fork 384
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
Changes from 7 commits
9cac970
6f3e303
5498863
00b03cc
83c286e
2c5c3e8
28caac8
9f8aa7e
1d3705f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
# AvoidMultipleTypesParameter | ||
|
||
**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 | ||
) | ||
... | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ | |||||
|[AvoidGlobalVars](./AvoidGlobalVars.md) | Warning | | | ||||||
|[AvoidInvokingEmptyMembers](./AvoidInvokingEmptyMembers.md) | Warning | | | ||||||
|[AvoidLongLines](./AvoidLongLines.md) | Warning | | | ||||||
|[AvoidMultipleTypesParameter](./AvoidMultipleTypesParameter.md) | Warning | | | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|[AvoidOverwritingBuiltInCmdlets](./AvoidOverwritingBuiltInCmdlets.md) | Warning | | | ||||||
|[AvoidNullOrEmptyHelpMessageAttribute](./AvoidNullOrEmptyHelpMessageAttribute.md) | Warning | | | ||||||
|[AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | | | ||||||
|
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
{ | ||||||
/// <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); | ||||||
} | ||||||
} | ||||||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<value>Avoid multiple type specifiers on parameters</value> | ||||||
</data> | ||||||
<data name="AvoidMultipleTypesParameterDescription" xml:space="preserve"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<value>Prameter should not have more than one type specifier.</value> | ||||||
</data> | ||||||
<data name="AvoidMultipleTypesParameterError" xml:space="preserve"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<value>Parameter '{0}' has more than one type specifier.</value> | ||||||
</data> | ||||||
<data name="AvoidMultipleTypesParameterName" xml:space="preserve"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<value>AvoidMultipleTypesParameter</value> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
</data> | ||||||
</root> |
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" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
$settings = @{ | ||||||
IncludeRules = @($ruleName) | ||||||
} | ||||||
} | ||||||
|
||||||
Describe 'AvoidMultipleTypesParameter' { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I would rewrite your test structure like this:
We could also use a few more tests:
|
||||||
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 | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.