Remove redundant whitespace between parameters with new option (disabled by default) in UseConsistentWhitespace (#1392)

* find extents of parameters elements

* first working prototype

* Productionize, add tests, docs and add to config files. TODO: message names

* tidy up

* add messages

* Apply suggestions from code review

Co-Authored-By: Robert Holt <[email protected]>

* revert suggestion around removal around expectedStartColumnNumberOfRightExtent variable, which broke build.
Variable is used 2 times and makes code more readable, therefore keeping it

* Tweak DiagnosticRecord creation and add more tests

* use named parameters for CorrectionExtent constructor

Co-authored-by: Robert Holt <[email protected]>

Author: bergmeister

Engine/Settings/CodeFormatting.psd1

@@ -38,6 +38,7 @@
             CheckOperator   = $true
             CheckPipe       = $true
             CheckSeparator  = $true
+            CheckParameter  = $false
         }
 
         PSAlignAssignmentStatement = @{

Engine/Settings/CodeFormattingAllman.psd1

@@ -38,6 +38,7 @@
             CheckOperator   = $true
             CheckPipe       = $true
             CheckSeparator  = $true
+            CheckParameter  = $false
         }
 
         PSAlignAssignmentStatement = @{

Engine/Settings/CodeFormattingOTBS.psd1

@@ -38,6 +38,7 @@
             CheckOperator  = $true
             CheckPipe       = $true
             CheckSeparator = $true
+            CheckParameter = $false
         }
 
         PSAlignAssignmentStatement = @{

Engine/Settings/CodeFormattingStroustrup.psd1

@@ -39,6 +39,7 @@
             CheckOperator   = $true
             CheckPipe       = $true
             CheckSeparator  = $true
+            CheckParameter  = $false
         }
 
         PSAlignAssignmentStatement = @{

RuleDocumentation/UseConsistentWhitespace.md

@@ -20,6 +20,7 @@
             CheckOperator   = $true
             CheckPipe       = $true
             CheckSeparator  = $true
+            CheckParameter  = $false
         }
     }
 ```
@@ -53,3 +54,8 @@ Checks if a comma or a semicolon is followed by a space. E.g. `@(1, 2, 3)` or `@
 #### CheckPipe: bool (Default value is `$true`)
 
 Checks if a pipe is surrounded on both sides by a space. E.g. `foo | bar` instead of `foo|bar`.
+
+#### CheckParameter: bool (Default value is `$false` at the moment due to the setting being new)
+
+Checks if there is more than one space between parameters and values. E.g. `foo -bar $baz -bat` instead of `foo   -bar   $baz   -bat`. This eliminates redundant whitespace that was probably added unintentionally.
+The rule does not check for whitespace between parameter and value when the colon syntax `-ParameterName:$ParameterValue` is used as some users prefer either 0 or 1 whitespace in this case.

Rules/Strings.Designer.cs

@@ -1104,4 +1104,7 @@
   <data name="UseProcessBlockForPipelineCommandName" xml:space="preserve">
     <value>UseProcessBlockForPipelineCommand</value>
   </data>
-</root>
+  <data name="UseConsistentWhitespaceErrorSpaceBetweenParameter" xml:space="preserve">
+    <value>Use only 1 whitespace between parameter names or values.</value>
+  </data>
+</root>

Rules/Strings.resx

@@ -22,7 +22,8 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 #endif
     public class UseConsistentWhitespace : ConfigurableRule
     {
-        private enum ErrorKind { BeforeOpeningBrace, Paren, Operator, SeparatorComma, SeparatorSemi, AfterOpeningBrace, BeforeClosingBrace, BeforePipe, AfterPipe };
+        private enum ErrorKind { BeforeOpeningBrace, Paren, Operator, SeparatorComma, SeparatorSemi,
+            AfterOpeningBrace, BeforeClosingBrace, BeforePipe, AfterPipe, BetweenParameter };
         private const int whiteSpaceSize = 1;
         private const string whiteSpace = " ";
         private readonly SortedSet<TokenKind> openParenKeywordWhitelist = new SortedSet<TokenKind>()
@@ -56,6 +57,9 @@ private List<Func<TokenOperations, IEnumerable<DiagnosticRecord>>> violationFind
         [ConfigurableRuleProperty(defaultValue: true)]
         public bool CheckSeparator { get; protected set; }
 
+        [ConfigurableRuleProperty(defaultValue: true)]
+        public bool CheckParameter { get; protected set; }
+
         public override void ConfigureRule(IDictionary<string, object> paramValueMap)
         {
             base.ConfigureRule(paramValueMap);
@@ -110,6 +114,11 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
                 diagnosticRecords = diagnosticRecords.Concat(violationFinder(tokenOperations));
             }
 
+            if (CheckParameter)
+            {
+                diagnosticRecords = diagnosticRecords.Concat(FindParameterViolations(ast));
+            }
+
             return diagnosticRecords.ToArray(); // force evaluation here
         }
 
@@ -203,6 +212,8 @@ private string GetError(ErrorKind kind)
                     return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorSeparatorComma);
                 case ErrorKind.SeparatorSemi:
                     return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorSeparatorSemi);
+                case ErrorKind.BetweenParameter:
+                    return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorSpaceBetweenParameter);
                 default:
                     return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorBeforeParen);
             }
@@ -364,6 +375,46 @@ private IEnumerable<DiagnosticRecord> FindOpenParenViolations(TokenOperations to
             }
         }
 
+        private IEnumerable<DiagnosticRecord> FindParameterViolations(Ast ast)
+        {
+            IEnumerable<Ast> commandAsts = ast.FindAll(
+                    testAst => testAst is CommandAst, true);
+            foreach (CommandAst commandAst in commandAsts)
+            {
+                List<Ast> commandParameterAstElements = commandAst.FindAll(testAst => true, searchNestedScriptBlocks: false).ToList();
+                for (int i = 0; i < commandParameterAstElements.Count - 1; i++)
+                {
+                    IScriptExtent leftExtent = commandParameterAstElements[i].Extent;
+                    IScriptExtent rightExtent = commandParameterAstElements[i + 1].Extent;
+                    if (leftExtent.EndLineNumber != rightExtent.StartLineNumber)
+                    {
+                        continue;
+                    }
+
+                    var expectedStartColumnNumberOfRightExtent = leftExtent.EndColumnNumber + 1;
+                    if (rightExtent.StartColumnNumber > expectedStartColumnNumberOfRightExtent)
+                    {
+                        int numberOfRedundantWhiteSpaces = rightExtent.StartColumnNumber - expectedStartColumnNumberOfRightExtent;
+                        var correction = new CorrectionExtent(
+                            startLineNumber: leftExtent.StartLineNumber,
+                            endLineNumber: leftExtent.EndLineNumber,
+                            startColumnNumber: leftExtent.EndColumnNumber + 1,
+                            endColumnNumber: leftExtent.EndColumnNumber + 1 + numberOfRedundantWhiteSpaces,
+                            text: string.Empty,
+                            file: leftExtent.File);
+
+                        yield return new DiagnosticRecord(
+                            GetError(ErrorKind.BetweenParameter),
+                            leftExtent,
+                            GetName(),
+                            GetDiagnosticSeverity(),
+                            leftExtent.File,
+                            suggestedCorrections: new CorrectionExtent[] { correction });
+                    }
+                }
+            }
+        }
+
         private bool IsSeparator(Token token)
         {
             return token.Kind == TokenKind.Comma || token.Kind == TokenKind.Semi;

Rules/UseConsistentWhitespace.cs

@@ -12,6 +12,7 @@ $ruleConfiguration = @{
     CheckOperator   = $false
     CheckPipe       = $false
     CheckSeparator  = $false
+    CheckParameter  = $false
 }
 
 $settings = @{
@@ -388,4 +389,69 @@ if ($true) { Get-Item `
         }
     }
 
+
+    Context "CheckParameter" {
+        BeforeAll {
+            $ruleConfiguration.CheckInnerBrace = $true
+            $ruleConfiguration.CheckOpenBrace = $false
+            $ruleConfiguration.CheckOpenParen = $false
+            $ruleConfiguration.CheckOperator = $false
+            $ruleConfiguration.CheckPipe = $false
+            $ruleConfiguration.CheckSeparator = $false
+            $ruleConfiguration.CheckParameter = $true
+        }
+
+        It "Should not find no violation when newlines are involved" {
+            $def = {foo -a $b `
+-c d -d $e -f g `
+-h i |
+bar -h i `
+-switch}
+            Invoke-ScriptAnalyzer -ScriptDefinition "$def" -Settings $settings | Should -Be $null
+        }
+
+        It "Should not find no violation if there is always 1 space between parameters except when using colon syntax" {
+            $def = 'foo -bar $baz @splattedVariable -bat -parameterName:$parameterValue'
+            Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Should -Be $null
+        }
+
+        It "Should find 1 violation if there is 1 space too much before a parameter" {
+            $def = 'foo  -bar'
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+            $violations.Count | Should -Be 1
+            $violations[0].Extent.Text | Should -Be 'foo'
+            $violations[0].SuggestedCorrections[0].Text | Should -Be ([string]::Empty)
+        }
+
+        It "Should find 1 violation if there is 1 space too much before a parameter value" {
+            $def = 'foo  $bar'
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+            $violations.Count | Should -Be 1
+            $violations[0].Extent.Text | Should -Be 'foo'
+            $violations[0].SuggestedCorrections[0].Text | Should -Be ([string]::Empty)
+        }
+
+        It "Should fix script to always have 1 space between parameters except when using colon syntax but not by default" {
+            $def = 'foo  -bar   $baz  -ParameterName:  $ParameterValue'
+            Invoke-Formatter -ScriptDefinition $def |
+                Should -BeExactly $def -Because 'CheckParameter configuration is not turned on by default (yet) as the setting is new'
+            Invoke-Formatter -ScriptDefinition $def -Settings $settings |
+                Should -BeExactly 'foo -bar $baz -ParameterName:  $ParameterValue'
+        }
+
+        It "Should fix script when newlines are involved" {
+            $def = {foo  -a  $b `
+-c  d -d  $e  -f  g `
+-h  i |
+bar  -h  i `
+-switch}
+            $expected = {foo -a $b `
+-c d -d $e -f g `
+-h i |
+bar -h i `
+-switch}
+            Invoke-Formatter -ScriptDefinition "$def" -Settings $settings |
+                Should -Be "$expected"
+        }
+    }
 }