Merge pull request #781 from PowerShell/kapilmb/fix-place-close-brace

Kapil Borle · web-flow · commit 50595fdd4008 · 2017-06-15T09:53:51.000-07:00
Fix PlaceCloseBrace rule behavior for NewLineAfter switch
diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs
@@ -33,7 +33,7 @@ public class PlaceCloseBrace : ConfigurableRule
         ///
         /// Default value is false.
         /// </summary>
-        [ConfigurableRuleProperty(defaultValue:false)]
+        [ConfigurableRuleProperty(defaultValue: false)]
         public bool NoEmptyLineBefore { get; protected set; }
 
         /// <summary>
@@ -82,11 +82,11 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
 
             var tokens = Helper.Instance.Tokens;
             var diagnosticRecords = new List<DiagnosticRecord>();
-            var curlyStack = new Stack<Tuple<Token, int>> ();
+            var curlyStack = new Stack<Tuple<Token, int>>();
 
             // TODO move part common with PlaceOpenBrace to one place
             var tokenOps = new TokenOperations(tokens, ast);
-            tokensToIgnore = new HashSet<Token> (tokenOps.GetCloseBracesInCommandElements());
+            tokensToIgnore = new HashSet<Token>(tokenOps.GetCloseBracesInCommandElements());
 
             // Ignore close braces that are part of a one line if-else statement
             // E.g. $x = if ($true) { "blah" } else { "blah blah" }
@@ -138,6 +138,12 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
                                 GetViolationForBraceShouldHaveNewLineAfter(tokens, k, openBracePos, fileName),
                                 ref diagnosticRecords);
                         }
+                        else
+                        {
+                            AddToDiagnosticRecords(
+                                GetViolationsForUncuddledBranches(tokens, k, openBracePos, fileName),
+                                ref diagnosticRecords);
+                        }
                     }
                     else
                     {
@@ -317,7 +323,7 @@ private DiagnosticRecord GetViolationForBraceShouldHaveNewLineAfter(
                 if ((tokens[expectedNewLinePos].Kind == TokenKind.Else
                     || tokens[expectedNewLinePos].Kind == TokenKind.ElseIf)
                     && !tokensToIgnore.Contains(closeBraceToken))
-                    {
+                {
                     return new DiagnosticRecord(
                         GetError(Strings.PlaceCloseBraceErrorShouldFollowNewLine),
                         closeBraceToken.Extent,
@@ -326,12 +332,74 @@ private DiagnosticRecord GetViolationForBraceShouldHaveNewLineAfter(
                         fileName,
                         null,
                         GetCorrectionsForBraceShouldHaveNewLineAfter(tokens, closeBracePos, openBracePos, fileName));
-                    }
+                }
             }
 
             return null;
         }
 
+        private DiagnosticRecord GetViolationsForUncuddledBranches(
+            Token[] tokens,
+            int closeBracePos,
+            int openBracePos,
+            string fileName)
+        {
+            // this will not work if there is a comment in between any tokens.
+            var closeBraceToken = tokens[closeBracePos];
+            if (tokens.Length <= closeBracePos + 2 ||
+                tokensToIgnore.Contains(closeBraceToken))
+            {
+                return null;
+            }
+
+            var token1 = tokens[closeBracePos + 1];
+            var token2 = tokens[closeBracePos + 2];
+            var branchTokenPos = IsBranchingStatementToken(token1) && !ApartByWhitespace(closeBraceToken, token1) ?
+                             closeBracePos + 1 :
+                             token1.Kind == TokenKind.NewLine && IsBranchingStatementToken(token2) ?
+                                closeBracePos + 2 :
+                                -1;
+
+            return branchTokenPos == -1 ?
+                null :
+                new DiagnosticRecord(
+                 GetError(Strings.PlaceCloseBraceErrorShouldCuddleBranchStatement),
+                 closeBraceToken.Extent,
+                 GetName(),
+                 GetDiagnosticSeverity(),
+                 fileName,
+                 null,
+                 GetCorrectionsForUncuddledBranches(tokens, closeBracePos, branchTokenPos, fileName));
+        }
+
+        private static bool ApartByWhitespace(Token token1, Token token2)
+        {
+            var e1 = token1.Extent;
+            var e2 = token2.Extent;
+            return e1.StartLineNumber == e2.StartLineNumber &&
+                e2.StartColumnNumber - e1.EndColumnNumber == 1;
+        }
+
+        private List<CorrectionExtent> GetCorrectionsForUncuddledBranches(
+            Token[] tokens,
+            int closeBracePos,
+            int branchStatementPos,
+            string fileName)
+        {
+            var corrections = new List<CorrectionExtent>();
+            var closeBraceToken = tokens[closeBracePos];
+            var branchStatementToken = tokens[branchStatementPos];
+            corrections.Add(new CorrectionExtent(
+                closeBraceToken.Extent.StartLineNumber,
+                branchStatementToken.Extent.EndLineNumber,
+                closeBraceToken.Extent.StartColumnNumber,
+                branchStatementToken.Extent.EndColumnNumber,
+                closeBraceToken.Extent.Text + ' ' + branchStatementToken.Extent.Text,
+                fileName));
+            return corrections;
+
+        }
+
         private DiagnosticRecord GetViolationForBraceShouldBeOnNewLine(
             Token[] tokens,
             int closeBracePos,
@@ -376,6 +444,20 @@ private List<CorrectionExtent> GetCorrectionsForBraceShouldBeOnNewLine(
             return corrections;
         }
 
+        private bool IsBranchingStatementToken(Token token)
+        {
+            switch (token.Kind)
+            {
+                case TokenKind.Else:
+                case TokenKind.ElseIf:
+                case TokenKind.Catch:
+                case TokenKind.Finally:
+                    return true;
+
+                default:
+                    return false;
+            }
+        }
         private void AddToDiagnosticRecords(
             DiagnosticRecord diagnosticRecord,
             ref List<DiagnosticRecord> diagnosticRecords)
diff --git a/Rules/Strings.resx b/Rules/Strings.resx
@@ -906,6 +906,9 @@
   <data name="PlaceCloseBraceErrorShouldFollowNewLine" xml:space="preserve">
     <value>Close brace does not follow a new line.</value>
   </data>
+  <data name="PlaceCloseBraceErrorShouldCuddleBranchStatement" xml:space="preserve">
+    <value>Close brace before a branch statement is followed by a new line.</value>
+  </data>
   <data name="UseConsistentIndentationName" xml:space="preserve">
     <value>UseConsistentIndentation</value>
   </data>
diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1
@@ -5,15 +5,15 @@ Import-Module PSScriptAnalyzer
 Import-Module (Join-Path $testRootDirectory "PSScriptAnalyzerTestHelper.psm1")
 
 $ruleConfiguration = @{
-    Enable = $true
-    NoEmptyLineBefore = $true
+    Enable             = $true
+    NoEmptyLineBefore  = $true
     IgnoreOneLineBlock = $true
-    NewLineAfter = $true
+    NewLineAfter       = $true
 }
 
 $settings = @{
     IncludeRules = @("PSPlaceCloseBrace")
-    Rules = @{
+    Rules        = @{
         PSPlaceCloseBrace = $ruleConfiguration
     }
 }
@@ -130,7 +130,7 @@ $x = if ($true) { "blah" } else { "blah blah" }
         }
     }
 
-    Context "When a close brace should be follow a new line" {
+    Context "When a close brace should be followed by a new line" {
         BeforeAll {
             $ruleConfiguration.'NoEmptyLineBefore' = $false
             $ruleConfiguration.'IgnoreOneLineBlock' = $false
@@ -148,11 +148,11 @@ if (Test-Path "blah") {
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
             $violations.Count | Should Be 1
             $params = @{
-                RawContent = $def
+                RawContent       = $def
                 DiagnosticRecord = $violations[0]
                 CorrectionsCount = 1
-                ViolationText = '}'
-                CorrectionText = '}' + [System.Environment]::NewLine
+                ViolationText    = '}'
+                CorrectionText   = '}' + [System.Environment]::NewLine
             }
             Test-CorrectionExtentFromContent @params
         }
@@ -168,11 +168,11 @@ if (Test-Path "blah") {
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
             $violations.Count | Should Be 1
             $params = @{
-                RawContent = $def
+                RawContent       = $def
                 DiagnosticRecord = $violations[0]
                 CorrectionsCount = 1
-                ViolationText = '}'
-                CorrectionText = '}' + [System.Environment]::NewLine
+                ViolationText    = '}'
+                CorrectionText   = '}' + [System.Environment]::NewLine
             }
             Test-CorrectionExtentFromContent @params
         }
@@ -194,6 +194,146 @@ Some-Command -Param1 @{
 Some-Command -Param1 @{
     key="value"
 } -Param2
+'@
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+            $violations.Count | Should Be 0
+        }
+    }
+
+    Context "When a close brace should not be followed by a new line" {
+        BeforeAll {
+            $ruleConfiguration.'NoEmptyLineBefore' = $false
+            $ruleConfiguration.'IgnoreOneLineBlock' = $false
+            $ruleConfiguration.'NewLineAfter' = $false
+        }
+
+        It "Should find a violation if a branching statement is on a new line if open brance is on same line" {
+            $def = @'
+if ($true) {
+    $true
+}
+else {
+    $false
+}
+'@
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+            $violations.Count | Should Be 1
+        }
+
+        It "Should correct violation by cuddling the else branch statement" {
+            $def = @'
+if ($true) {
+    $true
+}
+else {
+    $false
+}
+'@
+             $expected = @'
+if ($true) {
+    $true
+} else {
+    $false
+}
+'@
+            Invoke-Formatter -ScriptDefinition $def -Settings $settings | Should Be $expected
+        }
+
+        It "Should correct violation if the close brace and following keyword are apart by less than a space" {
+$def = @'
+if ($true) {
+    $true
+}else {
+    $false
+}
+'@
+             $expected = @'
+if ($true) {
+    $true
+} else {
+    $false
+}
+'@
+            Invoke-Formatter -ScriptDefinition $def -Settings $settings | Should Be $expected
+        }
+
+        It "Should correct violation if the close brace and following keyword are apart by more than a space" {
+$def = @'
+if ($true) {
+    $true
+}     else {
+    $false
+}
+'@
+             $expected = @'
+if ($true) {
+    $true
+} else {
+    $false
+}
+'@
+            Invoke-Formatter -ScriptDefinition $def -Settings $settings | Should Be $expected
+        }
+
+        It "Should correct violations in an if-else-elseif block" {
+            $def = @'
+$x = 1
+if ($x -eq 1) {
+    "1"
+}
+elseif ($x -eq 2) {
+    "2"
+}
+else {
+    "3"
+}
+'@
+             $expected = @'
+$x = 1
+if ($x -eq 1) {
+    "1"
+} elseif ($x -eq 2) {
+    "2"
+} else {
+    "3"
+}
+'@
+            Invoke-Formatter -ScriptDefinition $def -Settings $settings | Should Be $expected
+        }
+
+        It "Should correct violations in a try-catch-finally block" {
+            $def = @'
+try {
+    "try"
+}
+catch {
+    "catch"
+}
+finally {
+    "finally"
+}
+'@
+            $expected = @'
+try {
+    "try"
+} catch {
+    "catch"
+} finally {
+    "finally"
+}
+'@
+            Invoke-Formatter -ScriptDefinition $def -Settings $settings | Should Be $expected
+        }
+
+        It "Should not find violations when a script has two close braces in succession" {
+            $def = @'
+function foo {
+if ($true) {
+"true"
+} else {
+"false"
+}
+}
 '@
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
             $violations.Count | Should Be 0
