add AvoidMultipleTypesParameter Rule#1705

hankyi95 · 2021-08-19T10:55:46Z

PR Summary

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

ghost · 2021-08-19T10:55:58Z

All CLA requirements met.

rjmholt · 2021-08-19T16:53:17Z

@hankyi95 please fill out the PR description template

Rules/AvoidMultipleTypesParameter.cs

rjmholt · 2021-08-19T16:55:13Z

Rules/Strings.resx

+  <data name="AvoidMultipleTypesParameterName" xml:space="preserve">
+    <value>AvoidMultipleTypesParameter</value>
+  </data>
+</root>


Suggested change

</root>

</root>

rjmholt · 2021-08-19T16:55:33Z

Tests/Rules/AvoidMultipleTypesParameter.ps1

+# 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){}


Suggested change

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

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

rjmholt · 2021-08-19T16:55:50Z

Tests/Rules/AvoidMultipleTypesParameter.tests.ps1

+            $noViolations.Count | Should -Be 0
+        }
+    }
+}


Suggested change

}

}

rjmholt · 2021-08-19T16:55:58Z

Tests/Rules/AvoidMultipleTypesParameterNoViolations.ps1

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


Suggested change

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

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

rjmholt · 2021-08-19T17:01:38Z

Tests/Rules/AvoidMultipleTypesParameter.tests.ps1

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


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 · 2021-08-24T16:50:04Z

RuleDocumentation/AvoidMultipleTypesParameter.md

+
+## How
+
+Make each parameter has only 1 type spcifier.


Suggested change

Make each parameter has only 1 type spcifier.

Ensure each parameter has only 1 type specifier.

rjmholt · 2021-08-24T16:50:16Z

RuleDocumentation/AvoidMultipleTypesParameter.md

+
+## Description
+
+Parameter should not have more than one type specifier.


Suggested change

Parameter should not have more than one type specifier.

Parameters should not have more than one type specifier.

rjmholt · 2021-08-24T17:08:00Z

Rules/AvoidMultipleTypesParameter.cs

+        /// </summary>
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);


Suggested change

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

if (ast is null)

{

throw new ArgumentNullException(Strings.NullAstErrorMessage);

}

rjmholt · 2021-08-24T17:37:34Z

Rules/AvoidMultipleTypesParameter.cs

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


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);

rjmholt · 2021-08-24T17:38:31Z

Rules/Strings.resx

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


Suggested change

<value>Avoid Multiple Types Parameter</value>

<value>Avoid multiple type specifiers on parameters.</value>

rjmholt · 2021-08-24T17:41:31Z

Tests/Rules/AvoidMultipleTypesParameter.tests.ps1

+    }
+}
+
+Describe 'AvoidMultipleTypesParameter' {


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 · 2021-08-24T21:50:12Z

RuleDocumentation/AvoidMultipleTypesParameter.md

+        $Param1,
+
+        [switch]
+        $Switch=$False


Suggested change

$Switch=$False

$Switch

bergmeister · 2021-08-24T21:53:25Z

RuleDocumentation/AvoidMultipleTypesParameter.md

+
+## Description
+
+Parameter should not have more than one type specifier.


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

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

…ub.com/hankyi95/PSScriptAnalyzer into hankyi/feature/avoidmultipletypesparam

rjmholt · 2021-08-25T17:25:32Z

Rules/AvoidMultipleTypesParameter.cs

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


Suggested change

if (ast == null)

if (ast is null)

rjmholt · 2021-08-25T17:28:25Z

Tests/Rules/AvoidMultipleTypesParameter.tests.ps1

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


Suggested change

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

rjmholt · 2021-08-25T17:34:54Z

Tests/Rules/AvoidMultipleTypesParameter.tests.ps1

+        $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){}
+'@


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

RuleDocumentation/AvoidMultipleTypesParameter.md

rjmholt

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

rjmholt · 2021-08-26T17:30:02Z

RuleDocumentation/AvoidMultipleTypesParameter.md

@@ -0,0 +1,50 @@
+# AvoidMultipleTypesParameter


Suggested change

# AvoidMultipleTypesParameter

# AvoidMultipleTypeAttributes

rjmholt · 2021-08-26T17:30:21Z

RuleDocumentation/README.md

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


rjmholt · 2021-08-26T17:30:38Z

Rules/AvoidMultipleTypesParameter.cs

+#if !CORECLR
+    [Export(typeof(IScriptRule))]
+#endif
+    public sealed class AvoidMultipleTypesParameter : IScriptRule


Suggested change

public sealed class AvoidMultipleTypesParameter : IScriptRule

public sealed class AvoidMultipleTypeAttributesRule: IScriptRule

rjmholt · 2021-08-26T17:31:03Z

Rules/Strings.resx

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


Suggested change

<data name="AvoidMultipleTypesParameterCommonName" xml:space="preserve">

<data name="AvoidMultipleTypeAttributesCommonName" xml:space="preserve">

rjmholt · 2021-08-26T17:31:16Z

Rules/Strings.resx

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


Suggested change

<data name="AvoidMultipleTypesParameterDescription" xml:space="preserve">

<data name="AvoidMultipleTypeAttributesDescription" xml:space="preserve">

rjmholt · 2021-08-26T17:31:30Z

Rules/Strings.resx

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


Suggested change

<data name="AvoidMultipleTypesParameterError" xml:space="preserve">

<data name="AvoidMultipleTypeAttributesError" xml:space="preserve">

rjmholt · 2021-08-26T17:31:50Z

Rules/Strings.resx

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


Suggested change

<data name="AvoidMultipleTypesParameterName" xml:space="preserve">

<data name="AvoidMultipleTypeAttributesName" xml:space="preserve">

rjmholt · 2021-08-26T17:32:03Z

Rules/Strings.resx

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


Suggested change

<value>AvoidMultipleTypesParameter</value>

<value>AvoidMultipleTypeAttributes</value>

rjmholt · 2021-08-26T17:32:18Z

Tests/Rules/AvoidMultipleTypesParameter.tests.ps1

+# Licensed under the MIT License.
+
+BeforeAll {
+    $ruleName = "PSAvoidMultipleTypesParameter"


Suggested change

$ruleName = "PSAvoidMultipleTypesParameter"

$ruleName = "PSAvoidMultipleTypeAttributes"

…ub.com/hankyi95/PSScriptAnalyzer into hankyi/feature/avoidmultipletypesparam

add AvoidMultipleTypesParameter Rule

9cac970

rjmholt reviewed Aug 19, 2021

View reviewed changes

rjmholt mentioned this pull request Aug 19, 2021

Feature Request: Strict import scope and function signature semantics PowerShell/PowerShell#14084

Closed

hankyi95 and others added 2 commits August 20, 2021 16:48

resolve some comments

6f3e303

Update Rules/AvoidMultipleTypesParameter.cs

5498863

Co-authored-by: Robert Holt <rjmholt@gmail.com>

rjmholt reviewed Aug 24, 2021

View reviewed changes

bergmeister reviewed Aug 24, 2021

View reviewed changes

bergmeister approved these changes Aug 24, 2021

View reviewed changes

hankyi95 added 2 commits August 25, 2021 15:56

add more test case

00b03cc

Merge branch 'hankyi/feature/avoidmultipletypesparam' of https://gith…

83c286e

…ub.com/hankyi95/PSScriptAnalyzer into hankyi/feature/avoidmultipletypesparam

rjmholt suggested changes Aug 25, 2021

View reviewed changes

refactor test case

2c5c3e8

rjmholt approved these changes Aug 26, 2021

View reviewed changes

RuleDocumentation/AvoidMultipleTypesParameter.md Outdated Show resolved Hide resolved

RuleDocumentation/AvoidMultipleTypesParameter.md Outdated Show resolved Hide resolved

Apply suggestions from code review

28caac8

rjmholt enabled auto-merge (squash) August 26, 2021 17:29

rjmholt disabled auto-merge August 26, 2021 17:29

rjmholt suggested changes Aug 26, 2021

View reviewed changes

hankyi95 added 2 commits August 27, 2021 14:12

change rule name to AvoidMultipleTypeAttributes

9f8aa7e

Merge branch 'hankyi/feature/avoidmultipletypesparam' of https://gith…

1d3705f

…ub.com/hankyi95/PSScriptAnalyzer into hankyi/feature/avoidmultipletypesparam

hankyi95 requested a review from rjmholt August 27, 2021 06:16

rjmholt approved these changes Aug 27, 2021

View reviewed changes

rjmholt merged commit 8db488d into PowerShell:master Aug 27, 2021

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

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

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

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

	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.

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

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

	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' {

	\|[AvoidMultipleTypesParameter](./AvoidMultipleTypesParameter.md) \| Warning \| \|
	\|[AvoidMultipleTypeAttributes](./AvoidMultipleTypesParameter.md) \| Warning \| \|

	public sealed class AvoidMultipleTypesParameter : IScriptRule
	public sealed class AvoidMultipleTypeAttributesRule: IScriptRule

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

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

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

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

	<value>AvoidMultipleTypesParameter</value>
	<value>AvoidMultipleTypeAttributes</value>

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

Conversation

hankyi95 commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

ghost commented Aug 19, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmholt commented Aug 19, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hankyi95 commented Aug 19, 2021 •

edited

Loading

ghost commented Aug 19, 2021 •

edited by ghost

Loading