PSUseConsistentIndentation: Indentation wrong if pipeline is wrapped in parentheses

[felixfbecker]

Before submitting a bug report:

• Make sure you are able to repro it on the latest released version
• Perform a quick search for existing issues to check if this bug has already been reported

Steps to reproduce

Format one of these files:

https://github.com/pcgeek86/PSGitHub/blob/5fa152568f3218843bbec97c5604d6b16b03066f/Functions/Private/ConvertTo-ColoredPatch.ps1
https://github.com/pcgeek86/PSGitHub/blob/5fa152568f3218843bbec97c5604d6b16b03066f/Functions/Public/Merge-GitHubPullRequest.ps1
https://github.com/pcgeek86/PSGitHub/blob/5fa152568f3218843bbec97c5604d6b16b03066f/Functions/Public/New-GitHubReleaseAsset.ps1
https://github.com/pcgeek86/PSGitHub/blob/5fa152568f3218843bbec97c5604d6b16b03066f/Functions/Public/New-GitHubReview.ps1
https://github.com/pcgeek86/PSGitHub/blob/5fa152568f3218843bbec97c5604d6b16b03066f/Functions/Public/Set-GitHubToken.ps1

with

@{
    Severity = @('Error', 'Warning', 'Information')
    ExcludeRules = @(
        'PSUseShouldProcessForStateChangingFunctions' # TODO: implement ShouldProcess
    )
    Rules = @{
        PSPlaceOpenBrace = @{
            Enable = $true
            OnSameLine = $true
            NewLineAfter = $true
            IgnoreOneLineBlock = $true
        }

        PSPlaceCloseBrace = @{
            Enable = $true
            NewLineAfter = $false
            IgnoreOneLineBlock = $true
            NoEmptyLineBefore = $true
        }

        PSUseConsistentIndentation = @{
            Enable = $true
            Kind = 'space'
            IndentationSize = 4
            PipelineIndentation = 'IncreaseIndentationForFirstPipeline'
        }

        PSUseConsistentWhitespace = @{
            Enable = $true
            CheckOpenBrace = $true
            CheckOpenParen = $true
            CheckOperator = $true
            CheckSeparator = $true
        }

        PSAlignAssignmentStatement = @{
            Enable = $false
        }

        PSUseCorrectCasing = @{
            Enable = $true
        }
    }
}

Expected behavior

Should stay the same

Actual behavior

Following indentation is too much, one for every pair of () it seems, e.g.:

        if ($UploadUrl -match '\{\?(.+)\}') {
            $vars = @{ name = $name; label = $Label }
            $allowedVars = $Matches[1] -split ','
            $query = '?' + (($allowedVars | ForEach-Object {
                        if ($vars.ContainsKey($_)) {
                            $value = [System.Web.HttpUtility]::UrlEncode($vars[$_])
                            "$_=$value"
                        }
                    }) -join '&')
            $UploadUrl = $UploadUrl -replace '\{\?.+\}', $query
        }

Environment data

Name                           Value
----                           -----
PSVersion                      6.2.4
PSEdition                      Core
GitCommitId                    6.2.4
OS                             Darwin 19.2.0 Darwin Kernel Version 19.2.0: Sat Nov  9 03:47:04 PST 2019; root:xnu-6153…
Platform                       Unix
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

1.18.3

[bergmeister]

Thanks for raising it and the level of detail. Very interesting and important. Next week is busy for me, but it's definitely on my tracker to investigate more. In the meantime I verified that the behaviour repros in master as well.
At first I thought this could be due to code of PipelineIndentation but I also see it happens with any available setting of that option, therefore I am thinking there is also a chance it might be something else.

[bergmeister]

A more minimalistic repro is:

(foo | bar {
    baz
})

The parenthesis are 'causing' it to happen. I will investigate now what the root cause is in terms of code

[bergmeister]

Update: The indentation being one level too much on line 2 and 3 of the above minimal repro is being 'caused' by the left parenthesis and left brace both causing the indentation to increase by one level, which is one level too much here due to them both applying in this scenario. The code that does this is here:

PSScriptAnalyzer/Rules/UseConsistentIndentation.cs

Lines 145 to 152 in f389aa4

	switch (token.Kind)
	{
	case TokenKind.AtCurly:
	case TokenKind.AtParen:
	case TokenKind.LParen:
	case TokenKind.LCurly:
	case TokenKind.DollarParen:
	AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);

I didn't write this code but I think the idea is to provide indentation for the following scenarios that both apply in this special case

# Indentation after brace
if ($foo) {
    bar
}
# Indendation of multi-line condition
if ($foo -and
    $bar) { ... }

Do you agree with my reasoning of isolating this special case or do you see more cases?

This behaviour is actually not new and happens also with older version such as e.g. 1.17.1 and 1.18.1, therefore is not a regression or related to the new PipelineIndentation setting.
Unfortunately the formatter rules are token based, therefore writing or modifying logic is very hard as logical decisions are generic and often lack context of the surrounding syntax. I will have a think but I cannot guarantee I can think of a good solution to this without breaking something else....

@SydneyhSmith Since this is not a regression, I consider it to have lower priority, is it worthwhile having a label of its own?

[bergmeister]

@felixfbecker Thinking about this again, it feels like the best fix for this would be to tweak the logic for ( parenthesis to only increase indentation on the next line if it is being preceded by certain language keywords that have an opening parenthesis afterwards, which are: if, elseif, else, for, foreach, while or switch.
What do you think @rjmholt ?

[felixfbecker]

You probably still want to allow something like this though:

$result = (
    Get-Something | Get-SomethingElse | Get-Another
).Property

(it would look bad if line 2 was not indented)

[bergmeister]

@felixfbecker Good thinking. Thinking harder about the different use cases, it feels like ( should not increase indentation when the ( opening parenthesis is preceded by a Token with a Kind property of TokenKind.NewLine unless it is followed by a Token with a Kind property of TokenKind.NewLine . PowerShell's tokenizer skips whitespace btw. What do you think? I opened a PR with that below.

[rjmholt]

We should identify some cases where we think parens should and shouldn't increase indentation before proceeding I think, so that we don't end up oscillating between solutions that go too far in either direction.

I don't often use parens like this myself, so I don't have as good a feel for it as you might.

My thinking is:

• This rule should care about ASTs, not tokens, since if ($x -and $y) is different to ($x -and $y) in my mind in terms of how I indent them. The more specific we try to get to a syntactic structure, the more work we must do building the context for tokens, which is what ASTs are designed to be.
• Hanging clauses in a condition seems to be something that demands a newline and indentation in some cases
• Indentation in other paren expressions should probably only be increased for scriptblocks or something that isn't intrinsic to the parens; the parens should be invisible to the formatter
• The example in PSUseConsistentIndentation: Indentation wrong if pipeline is wrapped in parentheses  #1407 (comment) is fairly pathological I think, since paren expressions only allow one expression, so there's no benefit to putting things on new lines like that. I'm not sure what to do about that scenario particularly, but it seems to be its own scenario separate from paren indentation