Merge pull request #784 from PowerShell/kapilmb/fix-close-brace-rule

Update PlaceCloseBrace rule behavior for NewLineAfter switch

Author: Kapil Borle

Rules/PlaceCloseBrace.cs

@@ -28,6 +28,10 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 #endif
     public class PlaceCloseBrace : ConfigurableRule
     {
+        private HashSet<Token> tokensToIgnore;
+        private List<Func<Token[], int, int, string, DiagnosticRecord>> violationFinders
+            = new List<Func<Token[], int, int, string, DiagnosticRecord>>();
+
         /// <summary>
         /// Indicates if there should or should not be an empty line before a close brace.
         ///
@@ -57,7 +61,28 @@ public class PlaceCloseBrace : ConfigurableRule
         [ConfigurableRuleProperty(defaultValue: true)]
         public bool NewLineAfter { get; protected set; }
 
-        private HashSet<Token> tokensToIgnore;
+        /// <summary>
+        /// Sets the configurable properties of this rule.
+        /// </summary>
+        /// <param name="paramValueMap">A dictionary that maps parameter name to it value. Must be non-null</param>
+        public override void ConfigureRule(IDictionary<string, object> paramValueMap)
+        {
+            base.ConfigureRule(paramValueMap);
+            violationFinders.Add(GetViolationForBraceShouldBeOnNewLine);
+            if (NoEmptyLineBefore)
+            {
+                violationFinders.Add(GetViolationForBraceShouldNotFollowEmptyLine);
+            }
+
+            if (NewLineAfter)
+            {
+                violationFinders.Add(GetViolationForBraceShouldHaveNewLineAfter);
+            }
+            else
+            {
+                violationFinders.Add(GetViolationsForUncuddledBranches);
+            }
+        }
 
         /// <summary>
         /// Analyzes the given ast to find violations.
@@ -121,27 +146,10 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
                             continue;
                         }
 
-                        AddToDiagnosticRecords(
-                            GetViolationForBraceShouldBeOnNewLine(tokens, k, openBracePos, fileName),
-                            ref diagnosticRecords);
-
-                        if (NoEmptyLineBefore)
-                        {
-                            AddToDiagnosticRecords(
-                                GetViolationForBraceShouldNotFollowEmptyLine(tokens, k, openBracePos, fileName),
-                                ref diagnosticRecords);
-                        }
-
-                        if (NewLineAfter)
-                        {
-                            AddToDiagnosticRecords(
-                                GetViolationForBraceShouldHaveNewLineAfter(tokens, k, openBracePos, fileName),
-                                ref diagnosticRecords);
-                        }
-                        else
+                        foreach (var violationFinder in violationFinders)
                         {
                             AddToDiagnosticRecords(
-                                GetViolationsForUncuddledBranches(tokens, k, openBracePos, fileName),
+                                violationFinder(tokens, k, openBracePos, fileName),
                                 ref diagnosticRecords);
                         }
                     }
@@ -320,9 +328,7 @@ private DiagnosticRecord GetViolationForBraceShouldHaveNewLineAfter(
             if (tokens.Length > 1 && tokens.Length > expectedNewLinePos)
             {
                 var closeBraceToken = tokens[closeBracePos];
-                if ((tokens[expectedNewLinePos].Kind == TokenKind.Else
-                    || tokens[expectedNewLinePos].Kind == TokenKind.ElseIf)
-                    && !tokensToIgnore.Contains(closeBraceToken))
+                if (!tokensToIgnore.Contains(closeBraceToken) && IsBranchingStatementToken(tokens[expectedNewLinePos]))
                 {
                     return new DiagnosticRecord(
                         GetError(Strings.PlaceCloseBraceErrorShouldFollowNewLine),

Tests/Rules/PlaceCloseBrace.tests.ps1

@@ -177,6 +177,27 @@ if (Test-Path "blah") {
             Test-CorrectionExtentFromContent @params
         }
 
+        It "Should find a violation for a close brace followed by a catch statement" {
+             $def = @'
+try {
+    "try"
+} catch {
+    "catch"
+}
+
+'@
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+            $violations.Count | Should Be 1
+            $params = @{
+                RawContent       = $def
+                DiagnosticRecord = $violations[0]
+                CorrectionsCount = 1
+                ViolationText    = '}'
+                CorrectionText   = '}' + [System.Environment]::NewLine
+            }
+            Test-CorrectionExtentFromContent @params
+
+        }
         It "Should not find a violation for a close brace followed by a comma in an array expression" {
             $def = @'
 Some-Command -Param1 @{