PSUseConsistentIndentation: Check indentation of lines where first token is a LParen not followed by comment or new line

[liamjpeters]

PR Summary

Currently if an LParen ( is the first token on a line and the next token is not a comment or new line, then the line's indentation is not checked.

This is due to this if-check:

PSScriptAnalyzer/Rules/UseConsistentIndentation.cs

Lines 165 to 177 in a754b95

	case TokenKind.LParen:
	// When a line starts with a parenthesis and it is not the last non-comment token of that line,
	// then indentation does not need to be increased.
	if ((tokenIndex == 0 || tokens[tokenIndex - 1].Kind == TokenKind.NewLine) &&
	NextTokenIgnoringComments(tokens, tokenIndex)?.Kind != TokenKind.NewLine)
	{
	onNewLine = false;
	lParenSkippedIndentation.Push(true);
	break;
	}
	lParenSkippedIndentation.Push(false);
	AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
	break;

AddViolation(), which subsequently checks the lines indentation against the expected indentation, is not called if the conditional evaluates to true.

This PR changes the logic to always call AddViolation(), so the indentation is checked, but to only increase the indentation level when the conditional evaluates to false.

Fixes #1994

I've run this rule recursively over some large code-bases, including the PSScriptAnalyzer repo.

• In the current 1.22.0 we find 2,927 violations.
• With this PR applied, we find 2,931 violations.
• I believe the settings I used (below) means the rule defaults to spaces and many of the files were using tabs - hence the large number of violations.

  $Settings = @{
      IncludeRules = @('PSUseConsistentIndentation')
      Rules        = @{
          PSUseConsistentIndentation = @{
              Enable = $true
          }
      }
  }

• All previously found violations were still found.
• There were 4 newly found violations:

  • \Tests\Engine\ModuleHelp.Tests.ps1\ModuleHelp.Tests.ps1 line 48
    

    PSScriptAnalyzer/Tests/Engine/ModuleHelp.Tests.ps1

    Lines 47 to 54 in a754b95

    	if ((-not ((($commandModuleName = $CommandInfo.Module.Name) -and ($commandVersion = $CommandInfo.Module.Version)) -or
    	(($commandModuleName = $CommandInfo.PSSnapin) -and ($commandVersion = $CommandInfo.PSSnapin.Version))))) {
    	Write-Error "For $($CommandInfo.Name) : Can't find PSSnapin/module name and version"
    	}
    	else {
    	# "For $commandName : Module is $commandModuleName. Version is $commandVersion"
    	[PSCustomObject]@{ CommandName = $CommandInfo.Name; ModuleName = $commandModuleName; Version = $commandVersion }
    	}

    This becomes:

    

    Which is a little dubious 🤔

    Edit: This is because the previous line has 6 opening LParens, and 3 closing RParens. So the indentation level is 3 greater than the previous line

  • \Tests\Engine\ModuleHelp.Tests.ps1\ModuleHelp.Tests.ps1 line 116
    

    PSScriptAnalyzer/Tests/Engine/ModuleHelp.Tests.ps1

    Lines 115 to 117 in a754b95

    	It "gets example code from <CommandName>" -TestCases $testCases {
    	($Help.Examples.Example | Select-Object -First 1).Code | Should -Not -BeNullOrEmpty
    	}

    This line wasn't previously being checked. It's using tabs, and as mentioned above the settings is defaulting to spaces.

  • \Tests\Engine\CommunityAnalyzerRules\CommunityAnalyzerRules.psm1\CommunityAnalyzerRules.psm1 line 518
    

    PSScriptAnalyzer/Tests/Engine/CommunityAnalyzerRules/CommunityAnalyzerRules.psm1

    Lines 515 to 521 in a754b95

    	{
    	# Checks nesting structures
    	$nestingASTs = $asts | Where-Object {($PSItem.Extent.StartLineNumber -gt $ast.Extent.StartLineNumber) -and
    	($PSItem.Extent.EndLineNumber -lt $ast.Extent.EndLineNumber)}
    	# If one AST have end-of-line comments, we should skip it.
    	[bool]$needComment = $ast.Extent.EndScriptPosition.Line.Trim().EndsWith("}")

    This is changed to:

    

  • \Tests\Rules\UseToExportFieldsInManifest.tests.ps1\UseToExportFieldsInManifest.tests.ps1 line 73

    This is a line that randomly uses tabs

    

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

[bergmeister]

Thorough testing of formatting rules is always tricky although we've kept adding tests over the years. Have you done any manual smoke test with a build from this? If not, not an issue, we could always do a bit of that before release time

[liamjpeters]

I've used it across several large code bases at work. I'm happy with the corrections it's suggested on those.

Alongside the test run I did on the PSScriptAnalyzer module, I've tested it on a public module I've used a bit recently, IntuneManagement. It has a fairly large PowerShell codebase.

Without this PR applied, using settings to limit analysis to just the rule in question, PSUseConsistentIndentation, I get 3,304 violations.

With this PR applied I get 3,305.

Just like the PSScriptAnalyzer module anyalysis, all previously found violations were still found. So there was 1 additional violation found.

It's here on line 1,394.

New PR would fix it to be:

This is because Two Left-Parens are on line 1,393, with just the one Right-Paren. So the indentation on the following line is increased by 1.

To me it reads slightly clearer as I scan down the code-block: if statement, indentation, left curly brace for the body of the statement.

I'd personally probably format the code-block like:

if (
    ($obj.PSObject.Properties | Where-Object Name -EQ 'securityRequireSafetyNetAttestationBasicIntegrity') -and 
    ($obj.PSObject.Properties | Where-Object Name -EQ 'securityRequireSafetyNetAttestationCertifiedDevice')
) {

But that's personal preference.

Where's no sort of definitive style guide that covers this scenario (that I'm aware of anyway), it's not clear cut what should be done here.

Ultimately if this PR moves things in a 'more correct' direction (whatever 'correct' is), then I'd say I'm happy that this doesn't break anything. But if you think this change is moving the other way - I won't be offended with it being rejected or some changes being suggested; it's all good 😀.

[andyleejordan]

This looks better to me too. Thanks for the thorough field testing.