diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 3ecaea842..8c6e70dab 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" } @@ -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 { @@ -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 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, int closeBracePos, @@ -376,6 +444,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..66884bff7 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -906,6 +906,9 @@ Close brace does not follow a new line. + + Close brace before a branch statement is followed by a new line. + UseConsistentIndentation diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index 44b00e206..7b6f6203d 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 } } @@ -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