From 7755e787f98f78832e6e0a981cd358cb83087661 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 21:11:15 -0800 Subject: [PATCH 1/9] Add methods to get one line if-else braces --- Engine/TokenOperations.cs | 80 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 567fe4146..aea9de639 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -110,6 +110,86 @@ private IEnumerable GetBraceInCommandElement(TokenKind tokenKind) } } + public IEnumerable GetCloseBraceInOneLineIfStatement() + { + return GetBraceInOneLineIfStatment(TokenKind.RCurly); + } + + public IEnumerable GetOpenBraceInOneLineIfStatement() + { + return GetBraceInOneLineIfStatment(TokenKind.LCurly); + } + + // TODO Fix code duplication in the following method and GetBraceInCommandElement + private IEnumerable 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(); + 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 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> GetTokenNodes(TokenKind kind) { return GetTokenNodes((token) => token.Kind == kind); From 1329e309db888f25e271e1cd598c94d632ebb635 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 21:12:01 -0800 Subject: [PATCH 2/9] Ignore open brace in a one line if else statement --- Rules/PlaceOpenBrace.cs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/Rules/PlaceOpenBrace.cs b/Rules/PlaceOpenBrace.cs index 8d70aaf4c..e946dcbde 100644 --- a/Rules/PlaceOpenBrace.cs +++ b/Rules/PlaceOpenBrace.cs @@ -45,6 +45,9 @@ public class PlaceOpenBrace : ConfigurableRule [ConfigurableRuleProperty(defaultValue: true)] public bool NewLineAfter { get; protected set; } + [ConfigurableRuleProperty(defaultValue: true)] + public bool IgnoreOneLineIf { get; protected set; } + private List>> violationFinders = new List>>(); @@ -96,12 +99,21 @@ public override IEnumerable AnalyzeScript(Ast ast, string file var tokens = Helper.Instance.Tokens; // Ignore open braces that are part of arguments to a command - // * E.g. get-process | % { "blah } + // * E.g. get-process | % { "blah }f // 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( - new TokenOperations(tokens, ast).GetOpenBracesInCommandElements()); + var tokenOps = new TokenOperations(tokens, ast); + tokensToIgnore = new HashSet(tokenOps.GetOpenBracesInCommandElements()); + + if (IgnoreOneLineIf) + { + foreach (var openBraceToken in tokenOps.GetOpenBraceInOneLineIfStatement()) + { + tokensToIgnore.Add(openBraceToken); + } + } + foreach (var violationFinder in violationFinders) { diagnosticRecords.AddRange(violationFinder(tokens, ast, fileName)); From 2d2f7162ea8274f8cec332fb1aa9845cd4184023 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 21:13:00 -0800 Subject: [PATCH 3/9] Test open brace in a one line if-else statement --- Tests/Rules/PlaceOpenBrace.tests.ps1 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Tests/Rules/PlaceOpenBrace.tests.ps1 b/Tests/Rules/PlaceOpenBrace.tests.ps1 index 200a8777e..20f1cc3c3 100644 --- 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 = @{ @@ -42,7 +43,7 @@ function foo ($param1) { } '@ $ruleConfiguration.'OnSameLine' = $false - $ruleConfiguration.'NewLineAfter' = $true + $ruleConfiguration.'NewLineAfter' = $true $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings $defShouldIgnore = @' Get-Process | % { "blah" } @@ -61,6 +62,15 @@ 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.'IgnoreOneLineIf' = $true + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should Be 0 + } } Context "When a new line should follow an open brace" { From 8210765fa14286ec950a42516da9e4de48388920 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 21:13:38 -0800 Subject: [PATCH 4/9] Ignore close brace in a one line if else statement --- Rules/PlaceCloseBrace.cs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index b17d44261..542934831 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -36,6 +36,9 @@ public class PlaceCloseBrace : ConfigurableRule [ConfigurableRuleProperty(defaultValue:false)] public bool NoEmptyLineBefore { get; protected set; } + [ConfigurableRuleProperty(defaultValue: true)] + public bool IgnoreOneLineIf { get; protected set; } + private HashSet tokensToIgnore; /// @@ -64,8 +67,18 @@ public override IEnumerable AnalyzeScript(Ast ast, string file var tokens = Helper.Instance.Tokens; var diagnosticRecords = new List(); var curlyStack = new Stack> (); - tokensToIgnore = new HashSet ( - new TokenOperations(tokens, ast).GetCloseBracesInCommandElements()); + + // TODO move part common with PlaceOpenBrace to one place + // TODO use a general switch to ignore blocks on one line + var tokenOps = new TokenOperations(tokens, ast); + tokensToIgnore = new HashSet (tokenOps.GetCloseBracesInCommandElements()); + if (IgnoreOneLineIf) + { + foreach (var closeBraceToken in tokenOps.GetCloseBraceInOneLineIfStatement()) + { + tokensToIgnore.Add(closeBraceToken); + } + } for (int k = 0; k < tokens.Length; k++) { From a42fa1a1cccd89b7add3f8fd50df783868343364 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 21:13:59 -0800 Subject: [PATCH 5/9] Test close brace in a one line if-else statement --- Tests/Rules/PlaceCloseBrace.tests.ps1 | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index 983bb590d..841951a9f 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -6,6 +6,7 @@ $settings = @{ PSPlaceCloseBrace = @{ Enable = $true NoEmptyLineBefore = $true + IgnoreOneLineIf = $true } } } @@ -89,5 +90,13 @@ Get-Process * | % { "blah" } 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" } +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should Be 0 + } } -} \ No newline at end of file +} From 1b4ac85c6b62f9402475f68df4b200af7f64f46a Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 21:50:18 -0800 Subject: [PATCH 6/9] Add methods to return brace pairs --- Engine/TokenOperations.cs | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index aea9de639..35d4b6828 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -61,7 +61,6 @@ public IEnumerable GetOpenBracesInCommandElements() return GetBraceInCommandElement(TokenKind.LCurly); } - /// /// Return tokens of kind RCurly that end a scriptblock expression in an command element. /// @@ -74,6 +73,44 @@ public IEnumerable GetCloseBracesInCommandElements() return GetBraceInCommandElement(TokenKind.RCurly); } + /// + /// Returns pairs of associatd braces. + /// + /// Tuples of tokens such that item1 is LCurly token and item2 is RCurly token. + public IEnumerable> GetBracePairs() + { + var openBraceStack = new Stack(); + 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(openBraceStack.Pop(), token); + } + } + } + + /// + /// Returns brace pairs that are on the same line. + /// + /// Tuples of tokens such that item1 is LCurly token and item2 is RCurly token. + public IEnumerable> GetBracePairsOnSameLine() + { + foreach (var bracePair in GetBracePairs()) + { + if (bracePair.Item1.Extent.StartLineNumber == bracePair.Item2.Extent.StartLineNumber) + { + yield return bracePair; + } + } + } + private IEnumerable GetBraceInCommandElement(TokenKind tokenKind) { var cmdElemAsts = ast.FindAll(x => x is CommandElementAst && x is ScriptBlockExpressionAst, true); From 613c9a02748360d45a4b6e2054749216652a35ee Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 21:51:55 -0800 Subject: [PATCH 7/9] Add option to ignore open brace in a one line block --- Rules/PlaceOpenBrace.cs | 12 +++++++----- Tests/Rules/PlaceOpenBrace.tests.ps1 | 12 +++++++----- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/Rules/PlaceOpenBrace.cs b/Rules/PlaceOpenBrace.cs index e946dcbde..b79243518 100644 --- a/Rules/PlaceOpenBrace.cs +++ b/Rules/PlaceOpenBrace.cs @@ -46,7 +46,7 @@ public class PlaceOpenBrace : ConfigurableRule public bool NewLineAfter { get; protected set; } [ConfigurableRuleProperty(defaultValue: true)] - public bool IgnoreOneLineIf { get; protected set; } + public bool IgnoreOneLineBlock { get; protected set; } private List>> violationFinders = new List>>(); @@ -99,18 +99,20 @@ public override IEnumerable AnalyzeScript(Ast ast, string file var tokens = Helper.Instance.Tokens; // Ignore open braces that are part of arguments to a command - // * E.g. get-process | % { "blah }f + // * E.g. get-process | % { "blah } // 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 var tokenOps = new TokenOperations(tokens, ast); tokensToIgnore = new HashSet(tokenOps.GetOpenBracesInCommandElements()); - if (IgnoreOneLineIf) + // 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 openBraceToken in tokenOps.GetOpenBraceInOneLineIfStatement()) + foreach (var pair in tokenOps.GetBracePairsOnSameLine()) { - tokensToIgnore.Add(openBraceToken); + tokensToIgnore.Add(pair.Item1); } } diff --git a/Tests/Rules/PlaceOpenBrace.tests.ps1 b/Tests/Rules/PlaceOpenBrace.tests.ps1 index 20f1cc3c3..bbfab27de 100644 --- a/Tests/Rules/PlaceOpenBrace.tests.ps1 +++ b/Tests/Rules/PlaceOpenBrace.tests.ps1 @@ -15,7 +15,7 @@ $settings = @{ Describe "PlaceOpenBrace" { Context "When an open brace must be on the same line" { - BeforeAll{ + BeforeAll { $def = @' function foo ($param1) { @@ -36,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" } @@ -67,19 +68,20 @@ Get-Process | % { "blah" } $def = @' $x = if ($true) { "blah" } else { "blah blah" } '@ - $ruleConfiguration.'IgnoreOneLineIf' = $true + $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 } From 9e3c1d65eeffcf627aeaeb8991051be284251103 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 22:02:23 -0800 Subject: [PATCH 8/9] Add option to ignore close brace in a one line block --- Rules/PlaceCloseBrace.cs | 14 +++++++------- Tests/Rules/PlaceCloseBrace.tests.ps1 | 16 +++++++++------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 542934831..4d1be5b1c 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -37,7 +37,7 @@ public class PlaceCloseBrace : ConfigurableRule public bool NoEmptyLineBefore { get; protected set; } [ConfigurableRuleProperty(defaultValue: true)] - public bool IgnoreOneLineIf { get; protected set; } + public bool IgnoreOneLineBlock { get; protected set; } private HashSet tokensToIgnore; @@ -61,22 +61,22 @@ public override IEnumerable 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(); var curlyStack = new Stack> (); // TODO move part common with PlaceOpenBrace to one place - // TODO use a general switch to ignore blocks on one line var tokenOps = new TokenOperations(tokens, ast); tokensToIgnore = new HashSet (tokenOps.GetCloseBracesInCommandElements()); - if (IgnoreOneLineIf) + + // 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 closeBraceToken in tokenOps.GetCloseBraceInOneLineIfStatement()) + foreach (var pair in tokenOps.GetBracePairsOnSameLine()) { - tokensToIgnore.Add(closeBraceToken); + tokensToIgnore.Add(pair.Item2); } } diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index 841951a9f..8fd9a4835 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -1,17 +1,17 @@ Import-Module PSScriptAnalyzer -$ruleName = "PSPlaceCloseBrace" +$ruleConfiguration = @{ + Enable = $true + NoEmptyLineBefore = $true + IgnoreOneLineBlock = $true +} + $settings = @{ IncludeRules = @("PSPlaceCloseBrace") Rules = @{ - PSPlaceCloseBrace = @{ - Enable = $true - NoEmptyLineBefore = $true - IgnoreOneLineIf = $true - } + PSPlaceCloseBrace = $ruleConfiguration } } - Describe "PlaceCloseBrace" { Context "When a close brace is not on a new line" { BeforeAll { @@ -84,6 +84,7 @@ $hashtable = @{ $def = @' Get-Process * | % { "blah" } '@ + $ruleConfiguration.'IgnoreOneLineBlock' = $false $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings } @@ -95,6 +96,7 @@ Get-Process * | % { "blah" } $def = @' $x = if ($true) { "blah" } else { "blah blah" } '@ + $ruleConfiguration.'IgnoreOneLineBlock' = $true $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings $violations.Count | Should Be 0 } From 7b7d7ddcf38b465773df9c3befc64aa5eaec069a Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 7 Feb 2017 22:07:50 -0800 Subject: [PATCH 9/9] Add documentation for IgnoreOneLineBlock switch --- RuleDocumentation/PlaceCloseBrace.md | 6 ++++++ RuleDocumentation/PlaceOpenBrace.md | 6 ++++++ Rules/PlaceCloseBrace.cs | 10 +++++++++- Rules/PlaceOpenBrace.cs | 8 ++++++++ 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/RuleDocumentation/PlaceCloseBrace.md b/RuleDocumentation/PlaceCloseBrace.md index 417528333..12b891dc0 100644 --- 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. \ No newline at end of file diff --git a/RuleDocumentation/PlaceOpenBrace.md b/RuleDocumentation/PlaceOpenBrace.md index b4cd7f1cd..c5a1f7b45 100644 --- 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. \ No newline at end of file diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 4d1be5b1c..671f496f3 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -36,6 +36,14 @@ public class PlaceCloseBrace : ConfigurableRule [ConfigurableRuleProperty(defaultValue:false)] public bool NoEmptyLineBefore { get; protected set; } + /// + /// 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. + /// [ConfigurableRuleProperty(defaultValue: true)] public bool IgnoreOneLineBlock { get; protected set; } @@ -70,7 +78,7 @@ public override IEnumerable AnalyzeScript(Ast ast, string file var tokenOps = new TokenOperations(tokens, ast); tokensToIgnore = new HashSet (tokenOps.GetCloseBracesInCommandElements()); - // Ignore open braces that are part of a one line if-else statement + // Ignore close braces that are part of a one line if-else statement // E.g. $x = if ($true) { "blah" } else { "blah blah" } if (IgnoreOneLineBlock) { diff --git a/Rules/PlaceOpenBrace.cs b/Rules/PlaceOpenBrace.cs index b79243518..50df9f6c0 100644 --- a/Rules/PlaceOpenBrace.cs +++ b/Rules/PlaceOpenBrace.cs @@ -45,6 +45,14 @@ public class PlaceOpenBrace : ConfigurableRule [ConfigurableRuleProperty(defaultValue: true)] public bool NewLineAfter { get; protected set; } + /// + /// 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. + /// [ConfigurableRuleProperty(defaultValue: true)] public bool IgnoreOneLineBlock { get; protected set; }