Ensure equality when hashing default args and no args in actions

[carlos-zamora]

Summary of the Pull Request

#10297 found a bug in ActionMap where the ToggleCommandPalette key chord could not be found using GetKeyBindingForAction.

This was caused by the following:

• AddAction: when adding the action, the ActionAndArgs is basically added as {ToggleCommandPalette, ToggleCommandLineArgs{}} (Note the default ctor used for the action args)
• GetKeyBindingForAction: we're searching for an ActionAndArgs structured as {ToggleCommandPalette, nullptr}
• Since these are technically two different actions, we are unable to find it.

This issue was fixed by making the Hash(ActionAndArgs) function smarter! If the ActionAndArgs has no args, but the ShortcutAction supports args, generate the args using the default ctor.

By making Hash() smarter, everybody benefits from this logic! We can basically now enforce that ActionAndArgs{ <X>, nullptr } == ActionAndArgs{ <X>, <default_ctor> }.

Validation Steps Performed

• Added a test.
• Tested this on Replace feedback button with command palette button in dropdown #10297's branch and this does fix the bug

[DHowett]

This can't be serviced to Stable as that code doesn't exist in Stable. Be careful with those servicing tags...

[lhecker]

Woops I meant to comment and not approve.
I'll request changes, since I can't undo an approval.

[KalleOlaviNiemitalo]

It seems static InternalActionID Hash(const Model::ActionAndArgs& actionAndArgs) might have some hash collisions for non-equal inputs but each of its callers assumes that InternalActionID being equal means the ActionAndArgs is equal. That's not caused by this PR, though.

[carlos-zamora]

This can't be serviced to Stable as that code doesn't exist in Stable. Be careful with those servicing tags...

My bad. Wasn't sure if ActionMap was in stable or not. Removing the tag.

[carlos-zamora]

It seems static InternalActionID Hash(const Model::ActionAndArgs& actionAndArgs) might have some hash collisions for non-equal inputs but each of its callers assumes that InternalActionID being equal means the ActionAndArgs is equal. That's not caused by this PR, though.

Yeah. This is where I wish I took a course on cryptography and hashing in college haha. I don't think that theoretically there could be no collisions. That said, in practice, we haven't seen any so we're probably fine for now.

[lhecker]

Long term a more robust equality-solution would be nice, but that's for another day.
Until then: LGTM.

[DHowett]

I low-key hate that this requires constructing an entire object filled with default values to calculate a hash; it could be wildly expensive. Can you profile to make sure we're not making a bunch of objects just to destroy them moments later? Both CPU and memory-wise.

[miniksa]

I feel like we could get around this by maintaining a static .Empty singleton of each of these if this is what we need to Hash... or cache the hash of the empty one as a singleton...

[lhecker]

To be fair though none of the ActionArgs constructors are complex at the moment.

As such I "propose" the following solution:

• add constexpr to the default constructors of all ActionArgs classes
• add constexpr to all their Hash() member functions
• add constexpr to the getters of the ACTION_ARG and WINRT_PROPERTY macros
• write:

  case ShortcutAction::action:
      hashedArgs = implementation::action##Args{}.Hash();
      break;

This will probably compile to constants in release mode and in debug mode it won't allocate anything on the heap.

[DHowett]

That will be illegal because we're not using winrt::make for something that's known to be a runtimeclass.

[carlos-zamora]

Added EmptyHash() which lazy loads and caches the hash value of a default constructed action args.

Took the average of 5 perf traces in VS:

	Allocations (delta)	Heap Size (delta)
main	70511.2	13431.52
d83a764	71268	13472.34

With this change, we see an average increase of 756.8 Allocations and 70.82 Heap Size.

Scenario:

• First snapshot taken on first ActionMap::AddAction
• Second snapshot taken after Windows Terminal loads and the Command Palette is opened

These hashes are always first used in AddAction, but then they are used a few times in ActionMap (i.e. adding more actions, action comparison, action queries, etc...).

[ghost]

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.