Skip to content

UseConsistentWhitespace - Create option to ignore assignment operator inside hash table #1566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 5, 2021
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormatting.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
CheckPipeForRedundantWhitespace = $false
CheckSeparator = $true
CheckParameter = $false
IgnoreAssignmentOperatorInsideHashTable = $true
}

PSAlignAssignmentStatement = @{
Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingAllman.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
CheckPipeForRedundantWhitespace = $false
CheckSeparator = $true
CheckParameter = $false
IgnoreAssignmentOperatorInsideHashTable = $true
}

PSAlignAssignmentStatement = @{
Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingOTBS.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
CheckPipeForRedundantWhitespace = $false
CheckSeparator = $true
CheckParameter = $false
IgnoreAssignmentOperatorInsideHashTable = $true
}

PSAlignAssignmentStatement = @{
Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingStroustrup.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
CheckPipeForRedundantWhitespace = $false
CheckSeparator = $true
CheckParameter = $false
IgnoreAssignmentOperatorInsideHashTable = $true
}

PSAlignAssignmentStatement = @{
Expand Down
80 changes: 80 additions & 0 deletions Engine/TokenOperations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -232,5 +232,85 @@ private bool OnSameLine(Token token1, Token token2)
{
return token1.Extent.StartLineNumber == token2.Extent.EndLineNumber;
}

/// <summary>
/// Finds the position of a given token in the AST.
/// </summary>
/// <param name="token">The <see cref="Token"/> to search for.</param>
/// <returns>The Ast node directly containing the provided <see cref="Token"/>.</returns>
public Ast GetAstPosition(Token token)
{
FindAstPostitionVisitor findAstVisitor = new FindAstPostitionVisitor(token.Extent.StartScriptPosition);
ast.Visit(findAstVisitor);
return findAstVisitor.AstPosition;
}

}

/// <summary>
/// Provides an efficient way to find the position in the AST corresponding to a given script position.
/// </summary>
public class FindAstPostitionVisitor : AstVisitor2
{
private IScriptPosition searchPosition;

/// <summary>
/// Contains the position in the AST corresponding to the provided <see cref="IScriptPosition"/> upon completion of the <see cref="Ast.Visit(AstVisitor)"/> method.
/// </summary>
public Ast AstPosition { get; private set; }

/// <summary>
/// Initializes a new instance of the <see cref="FindAstPostitionVisitor"/> class with the postition to search for.
/// </summary>
/// <param name="position">The script position to search for.</param>
public FindAstPostitionVisitor(IScriptPosition position)
{
this.searchPosition = position;
}

/// <summary>
/// Traverses the AST based on offests to find the leaf node which contains the provided <see cref="IScriptPosition"/>.
/// This method implements the entire functionality of this visitor. All <see cref="AstVisitor2"/> methods are overridden to simply invoke this one.
/// </summary>
/// <param name="ast">Current AST node to process.</param>
/// <returns>An <see cref="AstVisitAction"/> indicating whether to visit children of the current node.</returns>
private AstVisitAction Visit(Ast ast)
{
if (ast.Extent.StartOffset > searchPosition.Offset || ast.Extent.EndOffset < searchPosition.Offset)
{
return AstVisitAction.SkipChildren;
}
else
{
AstPosition = ast;
return AstVisitAction.Continue;
}
}

public override AstVisitAction VisitScriptBlock(ScriptBlockAst scriptBlockAst)
{
return Visit(scriptBlockAst);
}

public override AstVisitAction VisitNamedBlock(NamedBlockAst namedBlockAst)
{
return Visit(namedBlockAst);
}

public override AstVisitAction VisitAssignmentStatement(AssignmentStatementAst assignmentStatementAst)
{
return Visit(assignmentStatementAst);
}

public override AstVisitAction VisitCommandExpression(CommandExpressionAst commandExpressionAst)
{
return Visit(commandExpressionAst);
}

public override AstVisitAction VisitHashtable(HashtableAst hashtableAst)
{
return Visit(hashtableAst);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given its general name, this visitor should probably have overrides for all AST types so that it will reliably work for all position visits. The alternative is to implement this class as a private nested class for the relevant rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully intend to implement all overrides. Just didn't want to produce that much code before I knew I was barking up the right tree. It flummoxed me a bit when I first decided to work on this that there is no good way to search a PowerShell AST based on script position without writing 200+ lines of code. That is why I had tried the simpler solution first (that I agree with you was not ideal by a long shot).

Copy link
Contributor

@rjmholt rjmholt Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well there is another way, which is to use FindAll() instead:

IEnumerable<Ast> astsContainingToken = scriptAst.FindAll(foundAst => ContainsTokenExtent(token, foundAst), /* search through nested ASTs -- I can't remember the correct parameter name */ true);

// Find the containing AST with the smallest extent
Ast smallestContainingAst = astsContainingToken.First();
foreach (Ast containingAst in astsContainingToken)
{
    if (HasSmallerExtent(smallestContainingAst, containingAst))
    {
        smallestContainingAst = containingAst;
    }
}

Naturally you'd need to write the methods I've used there, but they basically contain logic you've already written

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try something like that but ran into a problem because apparently Ast's can contain another of exactly the same size so there isn't always a smallest. At that point I figured as many issues as I have run into so far trying to use FindAll(), it would be better just to go the visitor route.


}
}
5 changes: 5 additions & 0 deletions RuleDocumentation/UseConsistentWhitespace.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
CheckPipeForRedundantWhitespace = $false
CheckSeparator = $true
CheckParameter = $false
IgnoreAssignmentOperatorInsideHashTable = $false
}
}
```
Expand Down Expand Up @@ -64,3 +65,7 @@ Checks if a pipe is surrounded by redundant whitespace (i.e. more than 1 whitesp

Checks if there is more than one space between parameters and values. E.g. `foo -bar $baz -bat` instead of `foo -bar $baz -bat`. This eliminates redundant whitespace that was probably added unintentionally.
The rule does not check for whitespace between parameter and value when the colon syntax `-ParameterName:$ParameterValue` is used as some users prefer either 0 or 1 whitespace in this case.

#### IgnoreAssignmentOperatorInsideHashTable: bool (Default value is `$false`)

When `CheckOperator` is set, ignore whitespace around assignment operators within multi-line hash tables. Set this option in order to use the `AlignAssignmentStatement` rule but still check whitespace around operators everywhere else.
13 changes: 13 additions & 0 deletions Rules/UseConsistentWhitespace.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ private List<Func<TokenOperations, IEnumerable<DiagnosticRecord>>> violationFind
[ConfigurableRuleProperty(defaultValue: false)]
public bool CheckParameter { get; protected set; }

[ConfigurableRuleProperty(defaultValue: false)]
public bool IgnoreAssignmentOperatorInsideHashTable { get; protected set; }

public override void ConfigureRule(IDictionary<string, object> paramValueMap)
{
base.ConfigureRule(paramValueMap);
Expand Down Expand Up @@ -530,6 +533,16 @@ private IEnumerable<DiagnosticRecord> FindOperatorViolations(TokenOperations tok
continue;
}

// exclude assignment operator inside of multi-line hash tables if requested
if (IgnoreAssignmentOperatorInsideHashTable && tokenNode.Value.Kind == TokenKind.Equals)
{
Ast containingAst = tokenOperations.GetAstPosition(tokenNode.Value);
if (containingAst is HashtableAst && containingAst.Extent.EndLineNumber != containingAst.Extent.StartLineNumber)
{
continue;
}
}

var hasWhitespaceBefore = IsPreviousTokenOnSameLineAndApartByWhitespace(tokenNode);
var hasWhitespaceAfter = tokenNode.Next.Value.Kind == TokenKind.NewLine
|| IsPreviousTokenOnSameLineAndApartByWhitespace(tokenNode.Next);
Expand Down
21 changes: 21 additions & 0 deletions Tests/Engine/TokenOperations.tests.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.

Describe "TokenOperations" {
It "Should return correct AST position for assignment operator in hash table" {
$scriptText = @'
$h = @{
a = 72
b = @{ z = "hi" }
}
'@
$tokens = $null
$parseErrors = $null
$scriptAst = [System.Management.Automation.Language.Parser]::ParseInput($scriptText, [ref] $tokens, [ref] $parseErrors)
$tokenOperations = New-Object Microsoft.Windows.PowerShell.ScriptAnalyzer.TokenOperations -ArgumentList @($tokens, $scriptAst)
$operatorToken = $tokens | Where-Object { $_.Extent.StartOffset -eq 32 }
$hashTableAst = $tokenOperations.GetAstPosition($operatorToken)
$hashTableAst | Should -BeOfType [System.Management.Automation.Language.HashTableAst]
$hashTableAst.Extent.Text | Should -Be '@{ z = "hi" }'
}
}
54 changes: 54 additions & 0 deletions Tests/Rules/UseConsistentWhitespace.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ function foo($param) {
$ruleConfiguration.CheckOperator = $true
$ruleConfiguration.CheckPipe = $false
$ruleConfiguration.CheckSeparator = $false
$ruleConfiguration.IgnoreAssignmentOperatorInsideHashTable = $false
}

It "Should find a violation if no whitespace around an assignment operator" {
Expand Down Expand Up @@ -180,6 +181,59 @@ $x = $true -and
Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Should -Be $null
}

It "Should find violation if not asked to ignore assignment operator in hash table" {
$def = @'
$ht = @{
variable = 3
other = 4
}
'@
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
Test-CorrectionExtentFromContent $def $violations 1 ' ' ' '
}
}

Context "When asked to ignore assignment operator inside hash table" {
BeforeAll {
$ruleConfiguration.CheckInnerBrace = $false
$ruleConfiguration.CheckOpenParen = $false
$ruleConfiguration.CheckOpenBrace = $false
$ruleConfiguration.CheckOperator = $true
$ruleConfiguration.CheckPipe = $false
$ruleConfiguration.CheckSeparator = $false
$ruleConfiguration.IgnoreAssignmentOperatorInsideHashTable = $true
}
It "Should not find violation if assignment operator is in multi-line hash table" {
$def = @'
$ht = @{
variable = 3
other = 4
}
'@
Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Should -Be $null
}

It "Should find violation if assignment operator has extra space in single-line hash table" {
$def = @'
$h = @{
ht = @{a = 3; b = 4}
eb = 33
}
'@
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
Test-CorrectionExtentFromContent $def $violations 1 ' ' ' '
}

It "Should find violation for extra space around non-assignment operator inside hash table" {
$def = @'
$ht = @{
variable = 3
other = 4 + 7
}
'@
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
Test-CorrectionExtentFromContent $def $violations 1 ' ' ' '
}
}

Context "When a comma is not followed by a space" {
Expand Down