Fix PSMethodInvocationConstraints.GetHashCode method #24965

crazyjncsu · 2025-02-06T21:30:15Z

PR Summary

PR Context

The issue comes from #12412. The bottom line is that we need to calculate the hash for all array members, not for the array object itself.

PR Checklist

src/System.Management.Automation/engine/MshMemberInfo.cs

daxian-dbw · 2025-02-11T18:32:55Z

@crazyjncsu Thank you for reporting this issue!
I think we should either revert the change back to the original hash code calculation with the addition of ParameterTypes.SequenceGetHashCode(), or use HashCode.Add to include all elements of ParameterTypes and GenericTypeParameters to the hash code calculation.

[UPDATE] I reverted the code back to the original hash code calculation with the addition of GenericTypeParameters.
@iSazonov @SeeminglyScience @crazyjncsu Can you please take a look?

daxian-dbw · 2025-02-11T23:23:56Z

/azp run

azure-pipelines · 2025-02-11T23:24:10Z

Azure Pipelines successfully started running 2 pipeline(s).

crazyjncsu · 2025-02-12T03:35:25Z

Looks great for my purposes ;)

iSazonov · 2025-02-12T04:08:25Z

@daxian-dbw Could you please explain why we cannot use HashCode.Combine() with SequenceGetHashCode() as @crazyjncsu proposed? I think old custom algorithm is not best choice. Or are you thinking about performance?
(I don't mind going back to the source code, which no doubt underwent a full audit a long time ago.)

SeeminglyScience · 2025-02-12T18:08:13Z

src/System.Management.Automation/engine/MshMemberInfo.cs

I would go with this since it seems to be the preferred API now. But I also don't know enough about the specifics of hashcodes to know why one is or isn't better (partially why I'd personally stick to the API that does it "properly" for you)

Suggested change

// algorithm based on https://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode

unchecked

{

int result = 61;

result = result * 397 + (MethodTargetType != null ? MethodTargetType.GetHashCode() : 0);

result = result * 397 + ParameterTypes.SequenceGetHashCode();

result = result * 397 + GenericTypeParameters.SequenceGetHashCode();

return result;

}

HashCode hashCode = new();

hashCode.Add(MethodTargetType);

if (ParameterTypes is not null)

{

foreach (Type parameterType in ParameterTypes)

{

hashCode.Add(parameterType);

}

}

else

{

hashCode.Add(0);

}

if (GenericTypeParameters is null)

{

hashCode.Add(0);

return hashCode.ToHashCode();

}

foreach (Type genericParameter in GenericTypeParameters)

{

hashCode.Add(genericParameter);

}

return hashCode.ToHashCode();

HashCode.Combine calls GetHashCode on the passed-in values (see its source code here), but Int32.GetHashCode() simply returns the integer itself, so HashCode.Combine(MethodTargetType, ParameterTypes.SequenceGetHashCode(), GenericTypeParameters.SequenceGetHashCode()) should work as expected.

but Int32.GetHashCode() simply returns the integer itself

Also given this, I guess we now can replace the utility method Utils.CombineHashCodes in PowerShell with HashCode.Combine.

But I also don't know enough about the specifics of hashcodes to know why one is or isn't better (partially why I'd personally stick to the API that does it "properly" for you)

Better:

Less collisions.

Random seed per process (protects against cache attacks).

daxian-dbw · 2025-02-12T18:26:28Z

My bad. I missed the fact that GetHashCode() on an integer will simply return the integer value itself.
So HashCode.Combine(MethodTargetType, ParameterTypes.SequenceGetHashCode(), GenericTypeParameters.SequenceGetHashCode()) should work as expected.

I have removed my commit.

microsoft-github-policy-service · 2025-02-13T04:31:26Z

📣 Hey @crazyjncsu, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms

iSazonov · 2025-02-13T04:33:25Z

@crazyjncsu Thanks for you contribution!

@ArmaanMcleod Have you an interest to migrate the SequenceGetHashCode() method to new HashCode.Add() API?

There is a difference between GetHashCode for array object itself and cumulative GetHashCode based on hashes of the array members.

TravisEz13 · 2025-04-28T17:38:19Z

@PowerShell/powershell-maintainers triage decision - wait until more data from a 7.5 release

There is a difference between GetHashCode for array object itself and cumulative GetHashCode based on hashes of the array members.

fixed gethashcode calculation

0df9d13

This comment was marked as duplicate.

Sign in to view

iSazonov requested review from SeeminglyScience and daxian-dbw February 7, 2025 03:48

iSazonov reviewed Feb 10, 2025

View reviewed changes

src/System.Management.Automation/engine/MshMemberInfo.cs Show resolved Hide resolved

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 10, 2025

iSazonov changed the title ~~fixed gethashcode calculation~~ Fix PSMethodInvocationConstraints.GetHashCode method Feb 10, 2025

iSazonov added BackPort-7.4.x-Consider BackPort-7.5.x-Consider labels Feb 10, 2025

iSazonov mentioned this pull request Feb 10, 2025

GetReflectionType only returns open generic types once per session #24982

Closed

5 tasks

SeeminglyScience reviewed Feb 12, 2025

View reviewed changes

daxian-dbw force-pushed the fix/24964 branch from 673eefe to 0df9d13 Compare February 12, 2025 18:30

daxian-dbw approved these changes Feb 12, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

iSazonov self-assigned this Feb 13, 2025

iSazonov merged commit 14fb6f2 into PowerShell:master Feb 13, 2025
68 of 70 checks passed

pwshBot mentioned this pull request Apr 10, 2025

[release/v7.5] Fix PSMethodInvocationConstraints.GetHashCode method #25306

Merged

9 tasks

TravisEz13 added Backport-7.5.x-Migrated and removed BackPort-7.5.x-Consider labels Apr 11, 2025

pwshBot mentioned this pull request May 14, 2025

[release/v7.4] Fix PSMethodInvocationConstraints.GetHashCode method #25567

Closed

9 tasks

TravisEz13 removed the BackPort-7.4.x-Consider label Jul 9, 2025

-            // algorithm based on https://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode
-            unchecked
-            {
-                int result = 61;
-                result = result * 397 + (MethodTargetType != null ? MethodTargetType.GetHashCode() : 0);
-                result = result * 397 + ParameterTypes.SequenceGetHashCode();
-                result = result * 397 + GenericTypeParameters.SequenceGetHashCode();
-                return result;
-            }
+            HashCode hashCode = new();
+            hashCode.Add(MethodTargetType);
+            if (ParameterTypes is not null)
+            {
+                foreach (Type parameterType in ParameterTypes)
+                {
+                    hashCode.Add(parameterType);
+                }
+            }
+            else
+            {
+                hashCode.Add(0);
+            }
+            if (GenericTypeParameters is null)
+            {
+                hashCode.Add(0);
+                return hashCode.ToHashCode();
+            }
+            foreach (Type genericParameter in GenericTypeParameters)
+            {
+                hashCode.Add(genericParameter);
+            }
+            return hashCode.ToHashCode();

Fix PSMethodInvocationConstraints.GetHashCode method #24965

Fix PSMethodInvocationConstraints.GetHashCode method #24965

Conversation

crazyjncsu commented Feb 6, 2025 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

This comment was marked as duplicate.

Uh oh!

daxian-dbw commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Feb 11, 2025

Uh oh!

azure-pipelines bot commented Feb 11, 2025

Uh oh!

crazyjncsu commented Feb 12, 2025

Uh oh!

iSazonov commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeeminglyScience Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

iSazonov Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

microsoft-github-policy-service bot commented Feb 13, 2025 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

crazyjncsu commented Feb 6, 2025 •

edited by iSazonov

Loading

daxian-dbw commented Feb 11, 2025 •

edited

Loading

iSazonov commented Feb 12, 2025 •

edited

Loading

iSazonov Feb 13, 2025 •

edited

Loading

daxian-dbw commented Feb 12, 2025 •

edited

Loading

microsoft-github-policy-service bot commented Feb 13, 2025 •

edited by unfurl-links bot

Loading

iSazonov commented Feb 13, 2025 •

edited

Loading