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

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.

All CLA requirements met.

hankyi95 please fill out the PR description template

Suggested change

      
            </root>
          
            </root>

Suggested change

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

Suggested change

Suggested change

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

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>

Suggested change

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

Suggested change

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

Suggested change

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

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

Suggested change

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

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

Suggested change

      
                    $Switch=$False
          
                    $Switch

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?

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

Suggested change

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

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

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

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

Suggested change

      
            # AvoidMultipleTypesParameter
          
            # AvoidMultipleTypeAttributes

Suggested change

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

Suggested change

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

Suggested change

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

Suggested change

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

Suggested change

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

Suggested change

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

Suggested change

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

Suggested change

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

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

add AvoidMultipleTypesParameter Rule

9cac970

Rob Holt (rjmholt) reviewed Aug 19, 2021

View reviewed changes

Rob Holt (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>

Rob Holt (rjmholt) reviewed Aug 24, 2021

View reviewed changes

Christoph Bergmeister (bergmeister) reviewed Aug 24, 2021

View reviewed changes

Christoph Bergmeister (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

Rob Holt (rjmholt) suggested changes Aug 25, 2021

View reviewed changes

refactor test case

2c5c3e8

Rob Holt (rjmholt) approved these changes Aug 26, 2021

View reviewed changes

Comment thread RuleDocumentation/AvoidMultipleTypesParameter.md Outdated

Comment thread RuleDocumentation/AvoidMultipleTypesParameter.md Outdated

Apply suggestions from code review

28caac8

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

Rob Holt (rjmholt) disabled auto-merge August 26, 2021 17:29

Rob Holt (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 Rob Holt (rjmholt) August 27, 2021 06:16

Rob Holt (rjmholt) approved these changes Aug 27, 2021

View reviewed changes

Rob Holt (rjmholt) merged commit 8db488d into PowerShell:master Aug 27, 2021

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

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

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

+              #if !CORECLR
+                  [Export(typeof(IScriptRule))]
+              #endif
+                  public sealed class AvoidMultipleTypesParameter : 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">

	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.

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

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

+                  }
+              }
+              Describe 'AvoidMultipleTypesParameter' {

	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>

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

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

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

Uh oh!

add AvoidMultipleTypesParameter Rule - #1705

add AvoidMultipleTypesParameter Rule#1705Rob Holt (rjmholt) merged 9 commits intoPowerShell:masterPowerShell/PSScriptAnalyzer:masterfrom hankyi95:hankyi/feature/avoidmultipletypesparamhankyi95/PSScriptAnalyzer:hankyi/feature/avoidmultipletypesparamCopy head branch name to clipboard

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!

Deleted user (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!

Rob Holt (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!

Christoph Bergmeister (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!

Rob Holt (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

add AvoidMultipleTypesParameter Rule#1705
Rob Holt (rjmholt) merged 9 commits into
PowerShell:masterfrom
hankyi95:hankyi/feature/avoidmultipletypesparam

hankyi95 commented Aug 19, 2021 •
edited
Loading

Deleted user (ghost) commented Aug 19, 2021 •
edited by ghost
Loading