AlignAssignmentStatement overhaul to fix issues and include handing of Enums. by liamjpeters · Pull Request #2132 · PowerShell/PSScriptAnalyzer

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.

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.

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.

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 - I hadn't considered the upstream VSCode extension settings.

I don't think the default needs to change, in the extension/ES, for CheckHashtable - or expose the Enable value as a setting. It defaults already to Enabled = true (hardcoded in ES) and CheckHashtable = true. So it was enabling this rule and hashtable checking by default already.

If anyone has changed the hashtable setting to false in their settings file, the rule will continue to skip checking hashtables - regardless of Enabled = true.

The rule does introduce Enum Member Value Alignment (enabled by default), so perhaps we need to introduce that setting as a minimum?

If both CheckHashtable and CheckEnum are false, the rule effectively does nothing.

Something like the below could be a reasonable starting point.

I can raise the EditorServices and extension PRs? - at least it'll start a conversation and gather some more opinions 🙂

What do you think?

---

As an aside, something like the below in settings would allow full control of the rule. A quick Google, however, tells me the "nested" settings aren't really a thing 😔.

• powershell > Code Formatting > Align Property Value Pairs

  Align assignment statements in a hashtable or a DSC Configuration

  • Include inline comments in Hashtable alignment

    Include Pairs with comments between the property and equals sign in alignment

• powershell > Code Formatting > Align Enum Member Values

  Align member value assignments in an Enum Type Definition

  • Include inline comments in Enum alignment

    Include assignments with comments between the member and equals sign in alignment

  • Include valueless members in Enum alignment

    Include members without a value in the alignment, as if they had a value

@bergmeister - I hadn't considered the upstream VSCode extension settings.

I don't think the default needs to change, in the extension/ES, for CheckHashtable - or expose the Enable value as a setting. It defaults already to Enabled = true (hardcoded in ES) and CheckHashtable = true. So it was enabling this rule and hashtable checking by default already.

If anyone has changed the hashtable setting to false in their settings file, the rule will continue to skip checking hashtables - regardless of Enabled = true.

The rule does introduce Enum Member Value Alignment (enabled by default), so perhaps we need to introduce that setting as a minimum?

If both CheckHashtable and CheckEnum are false, the rule effectively does nothing.

Something like the below could be a reasonable starting point.

I can raise the EditorServices and extension PRs? - at least it'll start a conversation and gather some more opinions 🙂

What do you think?

As an aside, something like the below in settings would allow full control of the rule. A quick Google, however, tells me the "nested" settings aren't really a thing 😔.

• powershell > Code Formatting > Align Property Value Pairs

  Align assignment statements in a hashtable or a DSC Configuration

  • Include inline comments in Hashtable alignment

    Include Pairs with comments between the property and equals sign in alignment

• powershell > Code Formatting > Align Enum Member Values

  Align member value assignments in an Enum Type Definition

  • Include inline comments in Enum alignment

    Include assignments with comments between the member and equals sign in alignment

  • Include valueless members in Enum alignment

    Include members without a value in the alignment, as if they had a value

The issue is more that before the user could turn rule on and off with user setting alignPropertyValuePairs but by changing the on/off switch to Enabled, it means it runs the new code for new options by default and cannot be turned off as there are currently no user settings for Enabled or the new options. That's why I think at minimum we need to give user ability to turn rule off if there is a bug in new code (experience showed, new formatting options often required iterations). But since this is quite an important rule that people rely on, I'd suggest adding the new option(s) as user settings so that a) users can take advantage of new features and also turn them off if needed (and we can as well by patching vs-code extension with different defaults). But like I said this is something we can do post the next PSSA release because the extension pins the PSSA version so we are in full control when to bump it and introduce new extension user setting to accommodate new release.

@liamjpeters Can you please resolve the merge conflicts (think due to merging your PR #2150)

I made that far harder for myself than it needed to be 😅. I think that's done. Thank goodness for squash commits, so nobody will ever see my little git mess🙂

Tests all pass in fork's CI (thanks for setting that up @bergmeister 👍).