From 2ed036f285cd8a8ce107cec4dcf5142856eba21d Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Sun, 11 Jun 2017 20:18:02 -0700 Subject: [PATCH 01/11] Fix context description in PlaceCloseBrace rule test --- Tests/Rules/PlaceCloseBrace.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index 44b00e206..ccb0f784e 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -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 From 32f15e9e6dcd7f9207cabba309b0bfffe1d6f56e Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Sun, 11 Jun 2017 20:26:13 -0700 Subject: [PATCH 02/11] Create violation for close brace in branch statements Whenever `NewLineAfter = $false`, the rule creates a violation if a branching statement is on a new line after the preceding close brace. We consider the following braching statements: - else - elseif - catch - finally --- Rules/PlaceCloseBrace.cs | 51 +++++++++++++++++++++++++++++++++++++++- Rules/Strings.resx | 3 +++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 3ecaea842..38d4a0af1 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -138,6 +138,12 @@ public override IEnumerable AnalyzeScript(Ast ast, string file GetViolationForBraceShouldHaveNewLineAfter(tokens, k, openBracePos, fileName), ref diagnosticRecords); } + else + { + AddToDiagnosticRecords( + GetViolationsForUncuddledBranches(tokens, k, openBracePos, fileName), + ref diagnosticRecords); + } } else { @@ -326,12 +332,41 @@ private DiagnosticRecord GetViolationForBraceShouldHaveNewLineAfter( fileName, null, GetCorrectionsForBraceShouldHaveNewLineAfter(tokens, closeBracePos, openBracePos, fileName)); - } + } } return null; } + private DiagnosticRecord GetViolationsForUncuddledBranches( + Token[] tokens, + int closeBracePos, + int openBracePos, + string fileName) + { + var expectedNewLinePos = closeBracePos + 1; + + if (tokens.Length > 1 && tokens.Length > expectedNewLinePos) + { + var closeBraceToken = tokens[closeBracePos]; + if (tokens[closeBracePos + 1].Kind == TokenKind.NewLine && + IsBranchingStatementToken(tokens[closeBracePos + 2])) + { + return new DiagnosticRecord( + GetError(Strings.PlaceCloseBraceErrorShouldCuddleBranchStatement), + closeBraceToken.Extent, + GetName(), + GetDiagnosticSeverity(), + fileName, + null, + GetCorrectionsForBraceShouldHaveNewLineAfter(tokens, closeBracePos, openBracePos, fileName)); + } + } + + return null; + } + + private DiagnosticRecord GetViolationForBraceShouldBeOnNewLine( Token[] tokens, int closeBracePos, @@ -376,6 +411,20 @@ private List 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 diagnosticRecords) diff --git a/Rules/Strings.resx b/Rules/Strings.resx index f954248dc..559d0ee13 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -906,6 +906,9 @@ Close brace does not follow a new line. + + Close brace follows a new line. + UseConsistentIndentation From 7e85ccce269f60abb59f24a9a6a69449a1033c3e Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Sun, 11 Jun 2017 21:00:28 -0700 Subject: [PATCH 03/11] Add corrections for uncuddled branch statements --- Rules/PlaceCloseBrace.cs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 38d4a0af1..0a136385c 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -346,11 +346,15 @@ private DiagnosticRecord GetViolationsForUncuddledBranches( { var expectedNewLinePos = closeBracePos + 1; + // this will not work if there is a comment in between any tokens. + // find violation only if the open brace is on the same line as the branching statement. + // todo handle types in catch statement if (tokens.Length > 1 && tokens.Length > expectedNewLinePos) { var closeBraceToken = tokens[closeBracePos]; if (tokens[closeBracePos + 1].Kind == TokenKind.NewLine && - IsBranchingStatementToken(tokens[closeBracePos + 2])) + IsBranchingStatementToken(tokens[closeBracePos + 2]) && + tokens[closeBracePos + 3].Kind == TokenKind.LCurly) { return new DiagnosticRecord( GetError(Strings.PlaceCloseBraceErrorShouldCuddleBranchStatement), @@ -359,13 +363,32 @@ private DiagnosticRecord GetViolationsForUncuddledBranches( GetDiagnosticSeverity(), fileName, null, - GetCorrectionsForBraceShouldHaveNewLineAfter(tokens, closeBracePos, openBracePos, fileName)); + GetCorrectionsForUncuddledBranches(tokens, closeBracePos, closeBracePos + 2, fileName)); } } return null; } + private List GetCorrectionsForUncuddledBranches( + Token[] tokens, + int closeBracePos, + int branchStatementPos, + string fileName) + { + var corrections = new List(); + 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, From 895420420c08c7562d8a7b2f3dc74b6a544edc55 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Sun, 11 Jun 2017 21:01:49 -0700 Subject: [PATCH 04/11] Add tests for uncuddled branch statements --- Tests/Rules/PlaceCloseBrace.tests.ps1 | 55 +++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index ccb0f784e..a708e70b7 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -199,4 +199,59 @@ Some-Command -Param1 @{ $violations.Count | Should Be 0 } } + + Context "When a close brace should 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 not find a violation if a branching statement is on a new line if open brance is not on same line" { + $def = @' +if ($true) +{ + $true +} +else +{ + $false +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should Be 0 + } + + 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 + } + } } From 16f01b1bef953b46380b1b7c9560d4c4783a2124 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Sun, 11 Jun 2017 21:02:25 -0700 Subject: [PATCH 05/11] Fix code formatting in PlaceCloseBrace.cs --- Rules/PlaceCloseBrace.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 0a136385c..5ecb9b89a 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -33,7 +33,7 @@ public class PlaceCloseBrace : ConfigurableRule /// /// Default value is false. /// - [ConfigurableRuleProperty(defaultValue:false)] + [ConfigurableRuleProperty(defaultValue: false)] public bool NoEmptyLineBefore { get; protected set; } /// @@ -82,11 +82,11 @@ public override IEnumerable AnalyzeScript(Ast ast, string file var tokens = Helper.Instance.Tokens; var diagnosticRecords = new List(); - var curlyStack = new Stack> (); + var curlyStack = new Stack>(); // TODO move part common with PlaceOpenBrace to one place var tokenOps = new TokenOperations(tokens, ast); - tokensToIgnore = new HashSet (tokenOps.GetCloseBracesInCommandElements()); + tokensToIgnore = new HashSet(tokenOps.GetCloseBracesInCommandElements()); // Ignore close braces that are part of a one line if-else statement // E.g. $x = if ($true) { "blah" } else { "blah blah" } @@ -323,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, From 826fb5486d58e10954b358e4a19f4d4acdeb6d97 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Sun, 11 Jun 2017 21:02:54 -0700 Subject: [PATCH 06/11] Fix code formatting in PlaceCloseBrace.tests.ps1 --- Tests/Rules/PlaceCloseBrace.tests.ps1 | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index a708e70b7..018d73b94 100644 --- 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 } } @@ -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 } From c6cfac2e3a8620a9faf0b2e5a1202b2ba1ffc623 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 14 Jun 2017 13:11:11 -0700 Subject: [PATCH 07/11] Update logic for checking newline after close brace --- Rules/PlaceCloseBrace.cs | 50 ++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 5ecb9b89a..0acc46f8e 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -344,30 +344,40 @@ private DiagnosticRecord GetViolationsForUncuddledBranches( int openBracePos, string fileName) { - var expectedNewLinePos = closeBracePos + 1; - // this will not work if there is a comment in between any tokens. - // find violation only if the open brace is on the same line as the branching statement. - // todo handle types in catch statement - if (tokens.Length > 1 && tokens.Length > expectedNewLinePos) + var closeBraceToken = tokens[closeBracePos]; + if (tokens.Length <= closeBracePos + 2 || + tokensToIgnore.Contains(closeBraceToken)) { - var closeBraceToken = tokens[closeBracePos]; - if (tokens[closeBracePos + 1].Kind == TokenKind.NewLine && - IsBranchingStatementToken(tokens[closeBracePos + 2]) && - tokens[closeBracePos + 3].Kind == TokenKind.LCurly) - { - return new DiagnosticRecord( - GetError(Strings.PlaceCloseBraceErrorShouldCuddleBranchStatement), - closeBraceToken.Extent, - GetName(), - GetDiagnosticSeverity(), - fileName, - null, - GetCorrectionsForUncuddledBranches(tokens, closeBracePos, closeBracePos + 2, fileName)); - } + return null; } - 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 && + e1.EndColumnNumber - e2.StartColumnNumber == 1; } private List GetCorrectionsForUncuddledBranches( From 2f5f97fa42a73a5fff04ed5bcf38dd2baed9f5ab Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 14 Jun 2017 13:12:10 -0700 Subject: [PATCH 08/11] Add tests to validate NewLineAfter=$false behavior --- Tests/Rules/PlaceCloseBrace.tests.ps1 | 93 +++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 11 deletions(-) diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index 018d73b94..a6d7f2a78 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -200,7 +200,7 @@ Some-Command -Param1 @{ } } - Context "When a close brace should be followed by a new line" { + Context "When a close brace should not be followed by a new line" { BeforeAll { $ruleConfiguration.'NoEmptyLineBefore' = $false $ruleConfiguration.'IgnoreOneLineBlock' = $false @@ -220,27 +220,48 @@ else { $violations.Count | Should Be 1 } - It "Should not find a violation if a branching statement is on a new line if open brance is not on same line" { + It "Should correct violation by cuddling the else branch statement" { $def = @' -if ($true) -{ +if ($true) { $true } -else -{ +else { $false } '@ - $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings - $violations.Count | Should Be 0 + $expected = @' +if ($true) { + $true +} else { + $false +} +'@ + Invoke-Formatter -ScriptDefinition $def -Settings $settings | Should Be $expected } - It "Should correct violation by cuddling the else branch statement" { - $def = @' + It "Should correct violation if the close brace and following keyword are apart by less than a space" { +$def = @' if ($true) { $true +}else { + $false } -else { +'@ + $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 } '@ @@ -250,6 +271,56 @@ if ($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 } From 52d3d2e36cbaf7ef11eefce1801298378255555b Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 14 Jun 2017 13:51:33 -0700 Subject: [PATCH 09/11] Fix logic for checking newline after close brace --- Rules/PlaceCloseBrace.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 0acc46f8e..8c6e70dab 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -356,7 +356,7 @@ private DiagnosticRecord GetViolationsForUncuddledBranches( var token2 = tokens[closeBracePos + 2]; var branchTokenPos = IsBranchingStatementToken(token1) && !ApartByWhitespace(closeBraceToken, token1) ? closeBracePos + 1 : - token1.Kind == TokenKind.NewLine || IsBranchingStatementToken(token2) ? + token1.Kind == TokenKind.NewLine && IsBranchingStatementToken(token2) ? closeBracePos + 2 : -1; @@ -377,7 +377,7 @@ private static bool ApartByWhitespace(Token token1, Token token2) var e1 = token1.Extent; var e2 = token2.Extent; return e1.StartLineNumber == e2.StartLineNumber && - e1.EndColumnNumber - e2.StartColumnNumber == 1; + e2.StartColumnNumber - e1.EndColumnNumber == 1; } private List GetCorrectionsForUncuddledBranches( From 83927b3db602a477db220250036df751723f2e2d Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 14 Jun 2017 13:52:18 -0700 Subject: [PATCH 10/11] Update error string --- Rules/Strings.resx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 559d0ee13..66884bff7 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -907,7 +907,7 @@ Close brace does not follow a new line. - Close brace follows a new line. + Close brace before a branch statement is followed by a new line. UseConsistentIndentation From c67e81e760a8fcd66260b5813340c5228398df8a Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 14 Jun 2017 13:52:55 -0700 Subject: [PATCH 11/11] Add test for successive close braces --- Tests/Rules/PlaceCloseBrace.tests.ps1 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index a6d7f2a78..7b6f6203d 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -324,5 +324,19 @@ try { '@ 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 + } } }