From 5e6c4fb97ffa0ef42303e93be994f861a4dd8472 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 8 Jul 2019 21:33:13 +0100 Subject: [PATCH 1/7] Ignore Hashtables for PlaceCloseBrace rule --- Engine/TokenOperations.cs | 12 ++++++++++++ Rules/PlaceCloseBrace.cs | 12 ++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 07b8a2621..8e79501c9 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -104,6 +104,18 @@ public IEnumerable> GetBracePairsOnSameLine() } } + public bool CloseBraceBelongsToHashTable(Token token) + { + var hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); // todo: cache + if (hashtableAsts.Any(oneAst => + oneAst.Extent.EndLineNumber == token.Extent.EndLineNumber && + oneAst.Extent.EndColumnNumber == token.Extent.EndColumnNumber)) + { + return true; + } + return false; + } + private IEnumerable GetBraceInCommandElement(TokenKind tokenKind) { var cmdElemAsts = ast.FindAll(x => x is CommandElementAst && x is ScriptBlockExpressionAst, true); diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 652197afd..980c18cc6 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -25,6 +25,8 @@ public class PlaceCloseBrace : ConfigurableRule private List> violationFinders = new List>(); + private TokenOperations _tokenOperations; + /// /// Indicates if there should or should not be an empty line before a close brace. /// @@ -103,14 +105,14 @@ public override IEnumerable AnalyzeScript(Ast ast, string file var curlyStack = new Stack>(); // TODO move part common with PlaceOpenBrace to one place - var tokenOps = new TokenOperations(tokens, ast); - tokensToIgnore = new HashSet(tokenOps.GetCloseBracesInCommandElements()); + _tokenOperations = new TokenOperations(tokens, ast); + tokensToIgnore = new HashSet(_tokenOperations.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()) + foreach (var pair in _tokenOperations.GetBracePairsOnSameLine()) { tokensToIgnore.Add(pair.Item2); } @@ -409,7 +411,9 @@ private DiagnosticRecord GetViolationForBraceShouldBeOnNewLine( { var closeBraceToken = tokens[closeBracePos]; if (tokens[closeBracePos - 1].Kind != TokenKind.NewLine - && !tokensToIgnore.Contains(closeBraceToken)) + && !tokensToIgnore.Contains(closeBraceToken) + && _tokenOperations.CloseBraceBelongsToHashTable(closeBraceToken) + ) { return new DiagnosticRecord( GetError(Strings.PlaceCloseBraceErrorShouldBeOnNewLine), From 6a824b8b2204473e4ffe77180467a5d38855a3d3 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 15 Aug 2019 22:16:26 +0100 Subject: [PATCH 2/7] suppress only single line hash tables --- Engine/TokenOperations.cs | 20 ++++++++++++++------ Rules/PlaceCloseBrace.cs | 2 +- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 8e79501c9..b6908bca6 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -104,16 +104,24 @@ public IEnumerable> GetBracePairsOnSameLine() } } - public bool CloseBraceBelongsToHashTable(Token token) + public bool CloseBraceBelongsToSingleLineHashTable(Token token) { var hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); // todo: cache - if (hashtableAsts.Any(oneAst => - oneAst.Extent.EndLineNumber == token.Extent.EndLineNumber && - oneAst.Extent.EndColumnNumber == token.Extent.EndColumnNumber)) + + var closeBraceBelongsToSingleLineHashTable = hashtableAsts.Any(oneAst => { - return true; + bool closeBraceBelongsToHashTable = oneAst.Extent.EndLineNumber == token.Extent.EndLineNumber + && oneAst.Extent.EndColumnNumber == token.Extent.EndColumnNumber; + if (!closeBraceBelongsToHashTable) + { + return false; + } + bool isSingleLineHashtable = oneAst.Extent.StartLineNumber == oneAst.Extent.EndLineNumber; + return isSingleLineHashtable; } - return false; + ); + + return closeBraceBelongsToSingleLineHashTable; } private IEnumerable GetBraceInCommandElement(TokenKind tokenKind) diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 980c18cc6..4c0212a95 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -412,7 +412,7 @@ private DiagnosticRecord GetViolationForBraceShouldBeOnNewLine( var closeBraceToken = tokens[closeBracePos]; if (tokens[closeBracePos - 1].Kind != TokenKind.NewLine && !tokensToIgnore.Contains(closeBraceToken) - && _tokenOperations.CloseBraceBelongsToHashTable(closeBraceToken) + && !_tokenOperations.CloseBraceBelongsToSingleLineHashTable(closeBraceToken) ) { return new DiagnosticRecord( From 7b4b473bfe00174b0570ac909c45e33fa2db6343 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Aug 2019 12:02:14 +0100 Subject: [PATCH 3/7] Add test and make fix in proper place where RCurly of hashtable gets wrongly associated to the LCurly of the one line if statement --- Engine/TokenOperations.cs | 42 ++++++++++++++------------- Rules/PlaceCloseBrace.cs | 4 +-- Tests/Rules/PlaceCloseBrace.tests.ps1 | 14 +++++++++ 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index b6908bca6..77e3c9e3d 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -17,6 +17,18 @@ public class TokenOperations private readonly Token[] tokens; private LinkedList tokensLL; private readonly Ast ast; + private IEnumerable hashtableAsts + { + get + { + if (_hashtableAsts == null) + { + _hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); + } + return _hashtableAsts; + } + } + private IEnumerable _hashtableAsts; public Ast Ast { get { return ast; } } @@ -73,6 +85,7 @@ public IEnumerable GetCloseBracesInCommandElements() public IEnumerable> GetBracePairs() { var openBraceStack = new Stack(); + var hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); // todo: cache foreach (var token in tokens) { if (token.Kind == TokenKind.LCurly) @@ -84,7 +97,15 @@ public IEnumerable> GetBracePairs() if (token.Kind == TokenKind.RCurly && openBraceStack.Count > 0) { - yield return new Tuple(openBraceStack.Pop(), token); + var closeBraceBelongsToHashTable = hashtableAsts.Any(hashtableAst => + { + return hashtableAst.Extent.EndLineNumber == token.Extent.EndLineNumber + && hashtableAst.Extent.EndColumnNumber == token.Extent.EndColumnNumber; + }); + if (!closeBraceBelongsToHashTable) + { + yield return new Tuple(openBraceStack.Pop(), token); + } } } } @@ -104,25 +125,6 @@ public IEnumerable> GetBracePairsOnSameLine() } } - public bool CloseBraceBelongsToSingleLineHashTable(Token token) - { - var hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); // todo: cache - - var closeBraceBelongsToSingleLineHashTable = hashtableAsts.Any(oneAst => - { - bool closeBraceBelongsToHashTable = oneAst.Extent.EndLineNumber == token.Extent.EndLineNumber - && oneAst.Extent.EndColumnNumber == token.Extent.EndColumnNumber; - if (!closeBraceBelongsToHashTable) - { - return false; - } - bool isSingleLineHashtable = oneAst.Extent.StartLineNumber == oneAst.Extent.EndLineNumber; - return isSingleLineHashtable; - } - ); - - return closeBraceBelongsToSingleLineHashTable; - } private IEnumerable GetBraceInCommandElement(TokenKind tokenKind) { diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 4c0212a95..2a3afd7f3 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -411,9 +411,7 @@ private DiagnosticRecord GetViolationForBraceShouldBeOnNewLine( { var closeBraceToken = tokens[closeBracePos]; if (tokens[closeBracePos - 1].Kind != TokenKind.NewLine - && !tokensToIgnore.Contains(closeBraceToken) - && !_tokenOperations.CloseBraceBelongsToSingleLineHashTable(closeBraceToken) - ) + && !tokensToIgnore.Contains(closeBraceToken)) { return new DiagnosticRecord( GetError(Strings.PlaceCloseBraceErrorShouldBeOnNewLine), diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index 22d4785f6..f7eda98a8 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -78,6 +78,20 @@ $hashtable = @{a = 1; b = 2} } } + Context "When there is a one line hashtable inside a one line statement" { + BeforeAll { + $def = 'if ($true) { $test = @{ } }' + $ruleConfiguration.'NoEmptyLineBefore' = $false + $ruleConfiguration.'IgnoreOneLineBlock' = $true + $ruleConfiguration.'NewLineAfter' = $false + } + + It "Should not find a violation" { + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should -Be 0 + } + } + Context "When there is a multi-line hashtable" { BeforeAll { From b6239b9c1bcd837f372bb794633f2535efe39d37 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Aug 2019 12:03:54 +0100 Subject: [PATCH 4/7] cleanup code --- Engine/TokenOperations.cs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 77e3c9e3d..1630597ca 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -17,18 +17,6 @@ public class TokenOperations private readonly Token[] tokens; private LinkedList tokensLL; private readonly Ast ast; - private IEnumerable hashtableAsts - { - get - { - if (_hashtableAsts == null) - { - _hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); - } - return _hashtableAsts; - } - } - private IEnumerable _hashtableAsts; public Ast Ast { get { return ast; } } From d28b4efc05cb1747c6dba38ad3556e8c13f084cb Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Aug 2019 16:31:59 +0100 Subject: [PATCH 5/7] revert change to minimise diff --- Engine/TokenOperations.cs | 1 - Rules/PlaceCloseBrace.cs | 8 +++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 1630597ca..77060355f 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -113,7 +113,6 @@ public IEnumerable> GetBracePairsOnSameLine() } } - private IEnumerable GetBraceInCommandElement(TokenKind tokenKind) { var cmdElemAsts = ast.FindAll(x => x is CommandElementAst && x is ScriptBlockExpressionAst, true); diff --git a/Rules/PlaceCloseBrace.cs b/Rules/PlaceCloseBrace.cs index 2a3afd7f3..652197afd 100644 --- a/Rules/PlaceCloseBrace.cs +++ b/Rules/PlaceCloseBrace.cs @@ -25,8 +25,6 @@ public class PlaceCloseBrace : ConfigurableRule private List> violationFinders = new List>(); - private TokenOperations _tokenOperations; - /// /// Indicates if there should or should not be an empty line before a close brace. /// @@ -105,14 +103,14 @@ public override IEnumerable AnalyzeScript(Ast ast, string file var curlyStack = new Stack>(); // TODO move part common with PlaceOpenBrace to one place - _tokenOperations = new TokenOperations(tokens, ast); - tokensToIgnore = new HashSet(_tokenOperations.GetCloseBracesInCommandElements()); + var tokenOps = new TokenOperations(tokens, ast); + 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" } if (IgnoreOneLineBlock) { - foreach (var pair in _tokenOperations.GetBracePairsOnSameLine()) + foreach (var pair in tokenOps.GetBracePairsOnSameLine()) { tokensToIgnore.Add(pair.Item2); } From 51fe11c675af09207df19849cae30d0a8eaa8945 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Aug 2019 16:42:12 +0100 Subject: [PATCH 6/7] final cleanup --- Engine/TokenOperations.cs | 2 +- Tests/Rules/PlaceCloseBrace.tests.ps1 | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 77060355f..707e95f8f 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -73,7 +73,7 @@ public IEnumerable GetCloseBracesInCommandElements() public IEnumerable> GetBracePairs() { var openBraceStack = new Stack(); - var hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); // todo: cache + var hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); foreach (var token in tokens) { if (token.Kind == TokenKind.LCurly) diff --git a/Tests/Rules/PlaceCloseBrace.tests.ps1 b/Tests/Rules/PlaceCloseBrace.tests.ps1 index f7eda98a8..df1be8812 100644 --- a/Tests/Rules/PlaceCloseBrace.tests.ps1 +++ b/Tests/Rules/PlaceCloseBrace.tests.ps1 @@ -80,14 +80,12 @@ $hashtable = @{a = 1; b = 2} Context "When there is a one line hashtable inside a one line statement" { BeforeAll { - $def = 'if ($true) { $test = @{ } }' - $ruleConfiguration.'NoEmptyLineBefore' = $false + $scriptDefinition = 'if ($true) { $test = @{ } }' $ruleConfiguration.'IgnoreOneLineBlock' = $true - $ruleConfiguration.'NewLineAfter' = $false } It "Should not find a violation" { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings $violations.Count | Should -Be 0 } } From 0384e949ee2239f25495ee5c273c0b8e2322258a Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister [MVP]" Date: Mon, 19 Aug 2019 21:04:25 +0100 Subject: [PATCH 7/7] Apply suggestions from code review Co-Authored-By: Robert Holt --- Engine/TokenOperations.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 707e95f8f..d7df01fc2 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -73,7 +73,7 @@ public IEnumerable GetCloseBracesInCommandElements() public IEnumerable> GetBracePairs() { var openBraceStack = new Stack(); - var hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); + IEnumerable hashtableAsts = ast.FindAll(oneAst => oneAst is HashtableAst, searchNestedScriptBlocks: true); foreach (var token in tokens) { if (token.Kind == TokenKind.LCurly) @@ -85,11 +85,12 @@ public IEnumerable> GetBracePairs() if (token.Kind == TokenKind.RCurly && openBraceStack.Count > 0) { - var closeBraceBelongsToHashTable = hashtableAsts.Any(hashtableAst => + bool closeBraceBelongsToHashTable = hashtableAsts.Any(hashtableAst => { return hashtableAst.Extent.EndLineNumber == token.Extent.EndLineNumber && hashtableAst.Extent.EndColumnNumber == token.Extent.EndColumnNumber; }); + if (!closeBraceBelongsToHashTable) { yield return new Tuple(openBraceStack.Pop(), token);