AlignAssignmentStatement overhaul to fix issues and include handing of Enums.

[liamjpeters]

PR Summary

Overhauls the AlignAssignmentStatement rule to include handling of enums as well as fix several issues with Hashtable alignment.

I am very open to feedback and suggestions 😀.

Setting defaults have changed. Previously the rule could be enabled, but not do anything, as CheckHashtable (previously the only setting and gating all functionality) defaulted to $false. I've changed this (as well as the new settings) to default to $true. If you enable the rule - it will do something now. You can then optionally disable any parts you don't want. The rule is opt-in and there are already breaking changes in this PR (see the baby and the bath water below). Docs update shows and explains new settings.

Hashtable alignment issues resolved include:

• Erroring when the key of a key-value pair is an expression containing an equals sign

  @{
      ($Key1 = 5) = "Value1"
      Key2 = "Value2"
  }

  Invoke-ScriptAnalyzer: Start position cannot be before End position.

  This is because of the way the current implementation finds Equals tokens. It find the equals token within the expression.

  It results in a correction extent that has an end column which is before the start column.

• Erasing inline comments when they appear between the key of the key-value pair and the Equals token.

  @{
      Key1 <#Sneakycomment#> = "Value1"
      Key2 = "Value2"
  }

  results in

  @{
      Key1 = "Value1"
      Key2 = "Value2"
  }

• Performance scaling poorly. This is due to how Equals tokens of Key-Value pairs are located. For each Key-Value pair the list of tokens is scanned (from the beginning) until the relevant Equals token is found. This means that, as a worst case, performance scales quadratically - proportional to half the number of Key-Value pairs in the file * the number of tokens.

  We can improve this by scanning the tokens once and keeping track of all the Equals tokens and what line they appear on. This reduces complexity to essentially linear time with negligible memory use. Goes from O(H*T) -> O(H+T).

  I've put together some stress-test files (one saved as a gist here and script to generate them here) and ran only the AlignAssignmentStatement rule on them. Results below from my machine, averaged over 5 runs (with the first cold run discarded):

  	Violations	Pre-PR	Post-PR
  100 Hashtables with 100 KVP	8,301	3.627s	0.221s
  500 Hashtables with 100 KVP	41,124	121.902s	1.533s
  1000 Hashtables with 100 KVP	82,147	457.873s	2.165s

• The baby is thrown out with the bath water. If a single Key-Value pair is not in the expected format (Key and Equals-sign on same line, no two kvp on same line etc) the whole hashtable is skipped. I don't think this should be the case. The rule should consider and align the keys which are in the expected format and ignore the others. The rule is opt-in and so people using it want alignment - we should do our best to provide it.

I've done my best to expand the test coverage.

Fixes #1739
Fixes #1860

Would address downstream VSCode Extension issue: PowerShell/vscode-powershell#5138

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.

[TMA-2]

This is great! I won't pretend to know what you did regarding performance (I'm merely a PowerShell coder, not a C# programmer), but was hoping enums would be added to the PSAlignAssignmentStatement rule, so kudos! I just wrote a custom rule to handle this very issue, but there's no way it performs as well and strips comments.

[liamjpeters]

This is great! I won't pretend to know what you did regarding performance (I'm merely a PowerShell coder, not a C# programmer) ...

👋 @TMA-2

I'm also merely a PowerShell coder, not a C# programmer day-to-day 😀 - I'm sure it shows to those that are - but I'm interested and am giving it a go.

There's a lot to this PR, some desirable - some perhaps not 😅. I imagine it will be a while yet before anyone has the time to review it, and it may need to change quite a bit once it is.

[TMA-2]

This is great! I won't pretend to know what you did regarding performance (I'm merely a PowerShell coder, not a C# programmer) ...

👋 @TMA-2

I'm also merely a PowerShell coder, not a C# programmer day-to-day 😀 - I'm sure it shows to those that are - but I'm interested and am giving it a go.

There's a lot to this PR, some desirable - some perhaps not 😅. I imagine it will be a while yet before anyone has the time to review it, and it may need to change quite a bit once it is.

Certainly don't mean to downplay PowerShell -- I can't get enough, personally, despite all the gaps in the ecosystem that don't seem to exist with other languages. But nonetheless, lots of ingenious things come from the community that shouldn't seem possible (PODE alone is amazing!).

Here's hoping this gets a review quite soon. There's still an open PR for fixing unary operator formatting from June 17 with no comments, so the maintainers may be busy with other projects.

I could still attempt to take a look and see what I see, for what it's worth?

[bergmeister]

Thanks for all the effort, I can see this was no easy feat.
It makes sense to move the gating to Enabled property (which should've been like that before) but we need to be careful, if there is any knock on effect on Vs-Code extension, which calls into rule here:
https://github.com/PowerShell/PowerShellEditorServices/blob/7b40f14d27f19b00069a10c8c2da02e3b9f59308/src/PowerShellEditorServices/Services/Workspace/LanguageServerSettings.cs#L298-L301
The value of AlignPropertyValuePairs that is used for CheckHashtable comes from here, with a default of true
https://github.com/PowerShell/vscode-powershell/blob/f3ee7362f11f70c2daf43a621cdc301cbde3f989/src/settings.ts#L94
Therefore we need to consider a PR there to either change default there as well and add new setting to let user control Enabled property OR adding settings for all the properties. As a start I'd say let's do just the former for simplicity. So far we've only mapped single user settings to individual PSSA settings as VS-Code didn't support rich objects as settings but I think that has changed now (?) but we don't need to boil the ocean for this but could be a candidate for the rule with object based vs code setting config. Don't get too frightened by this, we don't need to do this now, just when it comes to release time.