Merge pull request #706 from PowerShell/kapilmb/brace-rule-exceptions

Kapil Borle · web-flow · commit 9ef1119713cf · 2017-02-08T16:08:59.000-08:00
Add ability to ignore one line constructs in brace rules
diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs
@@ -61,7 +61,6 @@ public IEnumerable<Token> GetOpenBracesInCommandElements()
             return GetBraceInCommandElement(TokenKind.LCurly);
         }
 
-
         /// <summary>
         /// Return tokens of kind RCurly that end a scriptblock expression in an command element.
         ///
@@ -74,6 +73,44 @@ public IEnumerable<Token> GetCloseBracesInCommandElements()
             return GetBraceInCommandElement(TokenKind.RCurly);
         }
 
+        /// <summary>
+        /// Returns pairs of associatd braces.
+        /// </summary>
+        /// <returns>Tuples of tokens such that item1 is LCurly token and item2 is RCurly token.</returns>
+        public IEnumerable<Tuple<Token, Token>> GetBracePairs()
+        {
+            var openBraceStack = new Stack<Token>();
+            foreach (var token in tokens)
+            {
+                if (token.Kind == TokenKind.LCurly)
+                {
+                    openBraceStack.Push(token);
+                    continue;
+                }
+
+                if (token.Kind == TokenKind.RCurly
+                    && openBraceStack.Count > 0)
+                {
+                    yield return new Tuple<Token, Token>(openBraceStack.Pop(), token);
+                }
+            }
+        }
+
+        /// <summary>
+        /// Returns brace pairs that are on the same line.
+        /// </summary>
+        /// <returns>Tuples of tokens such that item1 is LCurly token and item2 is RCurly token.</returns>
+        public IEnumerable<Tuple<Token, Token>> GetBracePairsOnSameLine()
+        {
+            foreach (var bracePair in GetBracePairs())
+            {
+                if (bracePair.Item1.Extent.StartLineNumber == bracePair.Item2.Extent.StartLineNumber)
+                {
+                    yield return bracePair;
+                }
+            }
+        }
+
         private IEnumerable<Token> GetBraceInCommandElement(TokenKind tokenKind)
         {
             var cmdElemAsts = ast.FindAll(x => x is CommandElementAst && x is ScriptBlockExpressionAst, true);
@@ -110,6 +147,86 @@ private IEnumerable<Token> GetBraceInCommandElement(TokenKind tokenKind)
             }
         }
 
+        public IEnumerable<Token> GetCloseBraceInOneLineIfStatement()
+        {
+            return GetBraceInOneLineIfStatment(TokenKind.RCurly);
+        }
+
+        public IEnumerable<Token> GetOpenBraceInOneLineIfStatement()
+        {
+            return GetBraceInOneLineIfStatment(TokenKind.LCurly);
+        }
+
+        // TODO Fix code duplication in the following method and GetBraceInCommandElement
+        private IEnumerable<Token> GetBraceInOneLineIfStatment(TokenKind tokenKind)
+        {
+            var ifStatementAsts = ast.FindAll(ast =>
+            {
+                var ifAst = ast as IfStatementAst;
+                if (ifAst == null)
+                {
+                    return false;
+                }
+
+                return ifAst.Extent.StartLineNumber == ifAst.Extent.EndLineNumber;
+            },
+            true);
+
+            if (ifStatementAsts == null)
+            {
+                yield break;
+            }
+
+            var braceTokens = new List<Token>();
+            foreach (var ast in ifStatementAsts)
+            {
+                var ifStatementAst = ast as IfStatementAst;
+                foreach (var clause in ifStatementAst.Clauses)
+                {
+                    var tokenIf
+                        = tokenKind == TokenKind.LCurly
+                            ? GetTokens(clause.Item2).FirstOrDefault()
+                            : GetTokens(clause.Item2).LastOrDefault();
+                    if (tokenIf != null)
+                    {
+                        yield return tokenIf;
+                    }
+                }
+
+                if (ifStatementAst.ElseClause == null)
+                {
+                    continue;
+                }
+
+                var tokenElse
+                    = tokenKind == TokenKind.LCurly
+                            ? GetTokens(ifStatementAst.ElseClause).FirstOrDefault()
+                            : GetTokens(ifStatementAst.ElseClause).LastOrDefault();
+                if (tokenElse != null)
+                {
+                    yield return tokenElse;
+                }
+            }
+        }
+
+        private IEnumerable<Token> GetTokens(Ast ast)
+        {
+            int k = 0;
+            while (k < tokens.Length && tokens[k].Extent.EndOffset <= ast.Extent.StartOffset)
+            {
+                k++;
+            }
+
+            while (k < tokens.Length && tokens[k].Extent.EndOffset <= ast.Extent.EndOffset)
+            {
+                var token = tokens[k++];
+                if (token.Extent.StartOffset >= ast.Extent.StartOffset)
+                {
+                    yield return token;
+                }
+            }
+        }
+
         public IEnumerable<LinkedListNode<Token>> GetTokenNodes(TokenKind kind)
         {
             return GetTokenNodes((token) => token.Kind == kind);
diff --git a/RuleDocumentation/PlaceCloseBrace.md b/RuleDocumentation/PlaceCloseBrace.md
@@ -12,6 +12,7 @@ Close brace placement should follow a consistent style. It should be on a new li
         PSPlaceCloseBrace = @{
             Enable = $true
             NoEmptyLineBefore = $false
+            IgnoreOneLineBlock = $true
         }
     }
 ```
@@ -23,3 +24,8 @@ Enable or disable the rule during ScriptAnalyzer invocation.
 
 #### NoEmptyLineBefore: bool (Default value is `$false`)
 Create violation if there is an empty line before a close brace.
+
+#### IgnoreOneLineBlock: bool (Default value is `$true`)
+Indicates if close braces in a one line block should be ignored or not.
+E.g. $x = if ($true) { "blah" } else { "blah blah" }
+In the above example, if the property is set to true then the rule will not fire a violation.
diff --git a/RuleDocumentation/PlaceOpenBrace.md b/RuleDocumentation/PlaceOpenBrace.md
@@ -13,6 +13,7 @@ Open brace placement should follow a consistent style. It can either follow KR s
             Enable = $true
             OnSameLine = $true
             NewLineAfter = $true
+            IgnoreOneLineBlock = $true
         }
     }
 ```
@@ -27,3 +28,8 @@ Provide an option to enforce the open brace to be on the same line as preceding
 
 #### NewLineAfter: bool (Default value is `$true`)
 Enforce a new line character after an open brace. The default value is true.
+
+#### IgnoreOneLineBlock: bool (Default value is `$true`)
+Indicates if open braces in a one line block should be ignored or not.
+E.g. $x = if ($true) { "blah" } else { "blah blah" }
+In the above example, if the property is set to true then the rule will not fire a violation.
diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs
@@ -36,6 +36,17 @@ public class PlaceCloseBrace : ConfigurableRule
         [ConfigurableRuleProperty(defaultValue:false)]
         public bool NoEmptyLineBefore { get; protected set; }
 
+        /// <summary>
+        /// Indicates if close braces in a one line block should be ignored or not.
+        /// E.g. $x = if ($true) { "blah" } else { "blah blah" }
+        /// In the above example, if the property is set to true then the rule will
+        /// not fire a violation.
+        ///
+        /// Default value if true.
+        /// </summary>
+        [ConfigurableRuleProperty(defaultValue: true)]
+        public bool IgnoreOneLineBlock { get; protected set; }
+
         private HashSet<Token> tokensToIgnore;
 
         /// <summary>
@@ -58,14 +69,24 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
 
             // TODO Should have the following options
             // * no-empty-lines-before
-            // * align (if close brance and open brace on new lines align with open brace,
-            //   if close brace is on new line but open brace is not align with the first keyword on open brace line)
 
             var tokens = Helper.Instance.Tokens;
             var diagnosticRecords = new List<DiagnosticRecord>();
             var curlyStack = new Stack<Tuple<Token, int>> ();
-            tokensToIgnore = new HashSet<Token> (
-                new TokenOperations(tokens, ast).GetCloseBracesInCommandElements());
+
+            // TODO move part common with PlaceOpenBrace to one place
+            var tokenOps = new TokenOperations(tokens, ast);
+            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" }
+            if (IgnoreOneLineBlock)
+            {
+                foreach (var pair in tokenOps.GetBracePairsOnSameLine())
+                {
+                    tokensToIgnore.Add(pair.Item2);
+                }
+            }
 
             for (int k = 0; k < tokens.Length; k++)
             {
diff --git a/Rules/PlaceOpenBrace.cs b/Rules/PlaceOpenBrace.cs
@@ -45,6 +45,17 @@ public class PlaceOpenBrace : ConfigurableRule
         [ConfigurableRuleProperty(defaultValue: true)]
         public bool NewLineAfter { get; protected set; }
 
+        /// <summary>
+        /// Indicates if open braces in a one line block should be ignored or not.
+        /// E.g. $x = if ($true) { "blah" } else { "blah blah" }
+        /// In the above example, if the property is set to true then the rule will
+        /// not fire a violation.
+        ///
+        /// Default value if true.
+        /// </summary>
+        [ConfigurableRuleProperty(defaultValue: true)]
+        public bool IgnoreOneLineBlock { get; protected set; }
+
         private List<Func<Token[], Ast, string, IEnumerable<DiagnosticRecord>>> violationFinders
             = new List<Func<Token[], Ast, string, IEnumerable<DiagnosticRecord>>>();
 
@@ -100,8 +111,19 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
             // In the above case even if OnSameLine == false, we should not
             // flag the open brace as it would move the brace to the next line
             // and will invalidate the command
-            tokensToIgnore = new HashSet<Token>(
-                new TokenOperations(tokens, ast).GetOpenBracesInCommandElements());
+            var tokenOps = new TokenOperations(tokens, ast);
+            tokensToIgnore = new HashSet<Token>(tokenOps.GetOpenBracesInCommandElements());
+
+            // Ignore open braces that are part of a one line if-else statement
+            // E.g. $x = if ($true) { "blah" } else { "blah blah" }
+            if (IgnoreOneLineBlock)
+            {
+                foreach (var pair in tokenOps.GetBracePairsOnSameLine())
+                {
+                    tokensToIgnore.Add(pair.Item1);
+                }
+            }
+
             foreach (var violationFinder in violationFinders)
             {
                 diagnosticRecords.AddRange(violationFinder(tokens, ast, fileName));
diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1
@@ -1,16 +1,17 @@
 ﻿Import-Module PSScriptAnalyzer
-$ruleName = "PSPlaceCloseBrace"
+$ruleConfiguration = @{
+    Enable = $true
+    NoEmptyLineBefore = $true
+    IgnoreOneLineBlock = $true
+}
+
 $settings = @{
     IncludeRules = @("PSPlaceCloseBrace")
     Rules = @{
-        PSPlaceCloseBrace = @{
-            Enable = $true
-            NoEmptyLineBefore = $true
-        }
+        PSPlaceCloseBrace = $ruleConfiguration
     }
 }
 
-
 Describe "PlaceCloseBrace" {
     Context "When a close brace is not on a new line" {
         BeforeAll {
@@ -83,11 +84,21 @@ $hashtable = @{
             $def = @'
 Get-Process * | % { "blah" }
 '@
+            $ruleConfiguration.'IgnoreOneLineBlock' = $false
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
         }
 
         It "Should not find a violation" {
             $violations.Count | Should Be 0
         }
+
+        It "Should ignore violations for one line if statement" {
+            $def = @'
+$x = if ($true) { "blah" } else { "blah blah" }
+'@
+            $ruleConfiguration.'IgnoreOneLineBlock' = $true
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+            $violations.Count | Should Be 0
+        }
     }
-}
+}
diff --git a/Tests/Rules/PlaceOpenBrace.tests.ps1 b/Tests/Rules/PlaceOpenBrace.tests.ps1
@@ -3,6 +3,7 @@ $ruleConfiguration = @{
     Enable = $true
     OnSameLine = $true
     NewLineAfter = $true
+    IgnoreOneLineIf = $true
 }
 
 $settings = @{
@@ -14,7 +15,7 @@ $settings = @{
 
 Describe "PlaceOpenBrace" {
     Context "When an open brace must be on the same line" {
-        BeforeAll{
+        BeforeAll {
             $def = @'
 function foo ($param1)
 {
@@ -35,14 +36,15 @@ function foo ($param1)
     }
 
     Context "When an open brace must be on a new line" {
-        BeforeAll{
+        BeforeAll {
             $def = @'
 function foo ($param1) {
 
 }
 '@
             $ruleConfiguration.'OnSameLine' = $false
-	    $ruleConfiguration.'NewLineAfter' = $true
+            $ruleConfiguration.'NewLineAfter' = $true
+            $ruleConfiguration.'IgnoreOneLineBlock' = $false
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
             $defShouldIgnore = @'
 Get-Process | % { "blah" }
@@ -61,15 +63,25 @@ Get-Process | % { "blah" }
         It "Should ignore violations for a command element" {
             $violationsShouldIgnore.Count | Should Be 0
         }
+
+        It "Should ignore violations for one line if statement" {
+            $def = @'
+$x = if ($true) { "blah" } else { "blah blah" }
+'@
+            $ruleConfiguration.'IgnoreOneLineBlock' = $true
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+            $violations.Count | Should Be 0
+        }
     }
 
     Context "When a new line should follow an open brace" {
-        BeforeAll{
+        BeforeAll {
             $def = @'
 function foo { }
 '@
             $ruleConfiguration.'OnSameLine' = $true
             $ruleConfiguration.'NewLineAfter' = $true
+            $ruleConfiguration.'IgnoreOneLineBlock' = $false
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
         }
 
