◐ Shell
clean mode source ↗

add AvoidMultipleTypesParameter Rule by hankyi95 · Pull Request #1705 · PowerShell/PSScriptAnalyzer

rjmholt

@hankyi95

PR Summary

PR Checklist

@ghost

CLA assistant check
All CLA requirements met.

@rjmholt

@hankyi95 please fill out the PR description template

rjmholt

<data name="AvoidMultipleTypesParameterName" xml:space="preserve">
<value>AvoidMultipleTypesParameter</value>
</data>
</root> No newline at end of file

Choose a reason for hiding this comment

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

# 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){} No newline at end of file

Choose a reason for hiding this comment

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

function F11 ([switch][boolean] $s1, [int] $p1){}
function F11 ([switch][boolean] $s1, [int] $p1){}
$noViolations.Count | Should -Be 0
}
}
} No newline at end of file

Choose a reason for hiding this comment

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

@@ -0,0 +1,3 @@
function F10 ([int] $s1, [int] $p1){}

function F11 ([switch] $s1, [int] $p1){} No newline at end of file

Choose a reason for hiding this comment

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

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

Comment on lines +4 to +5

$violations = Invoke-ScriptAnalyzer $PSScriptRoot\AvoidMultipleTypesParameter.ps1 | Where-Object {$_.RuleName -eq $violationName}
$noViolations = Invoke-ScriptAnalyzer $PSScriptRoot\AvoidMultipleTypesParameterNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName}

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
 
Co-authored-by: Robert Holt <rjmholt@gmail.com>

rjmholt
## How

Make each parameter has only 1 type spcifier.

Choose a reason for hiding this comment

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

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

## Description

Parameter should not have more than one type specifier.

Choose a reason for hiding this comment

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

Parameter should not have more than one type specifier.
Parameters should not have more than one type specifier.
/// </summary>
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);

Choose a reason for hiding this comment

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

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

Comment on lines +39 to +41

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

Choose a reason for hiding this comment

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

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);
<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">
<value>Avoid Multiple Types Parameter</value>

Choose a reason for hiding this comment

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

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

Describe 'AvoidMultipleTypesParameter' {

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

bergmeister

$Param1,

[switch]
$Switch=$False

Choose a reason for hiding this comment

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

bergmeister
## Description

Parameter should not have more than one type specifier.

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?

bergmeister

Choose a reason for hiding this comment

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

Just a few comments but looks good from a high level, Rob already pointed out some low level code things to address, I'd be happy to merge after that

rjmholt

/// </summary>
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
if (ast == null)

Choose a reason for hiding this comment

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

if (ast == null)
if (ast is null)
}

Describe 'AvoidMultipleTypesParameter' {
it 'Should find 3 violations for paramters have more than one type spceifiers' {

Choose a reason for hiding this comment

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

it 'Should find 3 violations for paramters have more than one type spceifiers' {
It 'Should find 3 violations for paramters have more than one type spceifiers' {

Below as well

Comment on lines +14 to +20

$def = @'
function F1 ($s1, $p1){}
function F2 ([int] $s2, [int] $p2){}
function F3 ([int][switch] $s3, [int] $p3){}
function F4 ([int][ref] $s4, [int] $p4){}
function F5 ([int][switch][boolean] $s5, [int] $p5){}
'@

Choose a reason for hiding this comment

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

This is the kind of thing that the -TestCases parameter on It is designed for.

Here's a simple example, and a more sophisticated example.

Each example should be its own test, but each test should assert (with Should):

  • The number of violations
  • What rule reported the violation
  • What the violation is

rjmholt

rjmholt

Choose a reason for hiding this comment

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

Just realised the rule name AvoidMultipleTypesParameter doesn't quite work.

My suggestion is AvoidMultipleTypeAttributes (means we could generalise the rule later), but doesn't have to be that. Some other possibilities:

  • AvoidMultipleTypeSpecifiers
  • AvoidConflictingVariableAttributes (rule could be enhanced to deal with ValidateSet too...)
@@ -0,0 +1,50 @@
# AvoidMultipleTypesParameter

Choose a reason for hiding this comment

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

# AvoidMultipleTypesParameter
# AvoidMultipleTypeAttributes
|[AvoidGlobalVars](./AvoidGlobalVars.md) | Warning | |
|[AvoidInvokingEmptyMembers](./AvoidInvokingEmptyMembers.md) | Warning | |
|[AvoidLongLines](./AvoidLongLines.md) | Warning | |
|[AvoidMultipleTypesParameter](./AvoidMultipleTypesParameter.md) | Warning | |

Choose a reason for hiding this comment

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

|[AvoidMultipleTypesParameter](./AvoidMultipleTypesParameter.md) | Warning | |
|[AvoidMultipleTypeAttributes](./AvoidMultipleTypesParameter.md) | Warning | |
#if !CORECLR
[Export(typeof(IScriptRule))]
#endif
public sealed class AvoidMultipleTypesParameter : IScriptRule

Choose a reason for hiding this comment

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

public sealed class AvoidMultipleTypesParameter : IScriptRule
public sealed class AvoidMultipleTypeAttributesRule: IScriptRule
<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">

Choose a reason for hiding this comment

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

<data name="AvoidMultipleTypesParameterCommonName" xml:space="preserve">
<data name="AvoidMultipleTypeAttributesCommonName" xml:space="preserve">
<data name="AvoidMultipleTypesParameterCommonName" xml:space="preserve">
<value>Avoid multiple type specifiers on parameters</value>
</data>
<data name="AvoidMultipleTypesParameterDescription" xml:space="preserve">

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

<value>AvoidMultipleTypesParameter</value>
<value>AvoidMultipleTypeAttributes</value>
# Licensed under the MIT License.

BeforeAll {
$ruleName = "PSAvoidMultipleTypesParameter"

Choose a reason for hiding this comment

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

$ruleName = "PSAvoidMultipleTypesParameter"
$ruleName = "PSAvoidMultipleTypeAttributes"

rjmholt