Restore caching in Helper.GetCommandInfo

[SeeminglyScience]

PR Summary

This change restores caching of CommandInfo objects in rules were it was previously disabled. My understanding is that it was disabled due to the fact that retrieving from the cache ignored the specified CommandTypes parameter, so the key for the cache is now a struct that takes both the command name, and the specified command types into account.

Performance is significantly improved in situations where either the script being analyzed is quite large, or has many unresolvable commands. For example, this build script previously took about 10 seconds to complete with just PSAvoidUsingCmdletAliases enabled. Now it is ~2 seconds for the first invocation and ~30-60ms for every subsequent invocation.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• NA - User facing documentation needed
• Change is not breaking
• NA - Make sure you've added a new test if existing tests do not effectively test the code changed
• 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

[rjmholt]

LGTM!

[rjmholt]

On this part, worth reading through PowerShell/PowerShell#7134 and the comment it starts with. We discussed the hashcode stuff a bit, but not sure (1) how far we want to take the bikeshedding on hashcodes or (2) if there's a convenient function to do this in both .NET Core and .NET Framework

[bergmeister]

PSSA already has many conditional compilation options for different version of powershell, therefore using a special, optimised version for .net core would be acceptable (the only restriction is that it should be part of .net standard 2.0 at the moment)

[rjmholt]

Another option might be new Tuple(Name.ToUpperInvariant(), CommandTypes).GetHashCode(). I don't think older framework versions have System.ValueTuple, so this would be forced to allocate :(

[bergmeister]

There is a NuGet package for System.ValueTuple that supports full .net and .net core

[bergmeister]

Thanks for the performance improvement. After this PR has been merged, we could consider applying your new struct also to GetCommandInfoLegacy. One nit before I'd be happy merging the PR is to put the code of CommandLookupKey into its own file as the Helper class is already way too big.

[SeeminglyScience]

@bergmeister

After this PR has been merged, we could consider applying your new struct also to GetCommandInfoLegacy.

The diff view on this one makes this really hard to see, but it already is applied to legacy. They both already pulled from the same cache, the new one just didn't add to it. Looks like the tests ran clean, but if there's any additional scenarios you'd like me to test to ensure the same issue didn't creep up let me know.

put the code of CommandLookupKey into its own file as the Helper class is already way too big.

Yeah I had the same thought. I kept it in the same file because it's only used by Helper so I was able to make it private nested. If I move it out I have to make it internal. Not a big deal obviously and I definitely don't mind doing that, just want to confirm you're aware and still want it moved.

[bergmeister]

@SeeminglyScience Ok, thanks, it did not see before it was also in GetCommandInfoLegacy until I expanded the diff view. Yes, please put the struct into a separate file please, making it internal is fine.

[bergmeister]

LGTM, I will leave this PR open for 1-2 days before I press the merge button in case @JamesWTruher or others wants to have a quick look at it as well or at least see/know about it.

[JamesWTruher]

looks great - I'll merge it