Repository navigation
Fix PSMethodInvocationConstraints.GetHashCode method - #24965
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Jake Morgan (@crazyjncsu) Thank you for reporting this issue! [UPDATE] I reverted the code back to the original hash code calculation with the addition of |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Looks great for my purposes ;) |
|
Dongbo Wang (@daxian-dbw) Could you please explain why we cannot use HashCode.Combine() with SequenceGetHashCode() as Jake Morgan (@crazyjncsu) proposed? I think old custom algorithm is not best choice. Or are you thinking about performance? |
There was a problem hiding this comment.
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)
| // 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
My bad. I missed the fact that I have removed my commit. |
673eefe to
0df9d13
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
📣 Hey Jake Morgan (@crazyjncsu), how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
|
Jake Morgan (@crazyjncsu) Thanks for you contribution! Armaan Mcleod (@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.
|
@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.
There is a difference between GetHashCode for array object itself and cumulative GetHashCode based on hashes of the array members.
…owerShell#24965) (PowerShell#26953) Co-authored-by: crazyjncsu <j@thewolfweb.com>
There is a difference between GetHashCode for array object itself and cumulative GetHashCode based on hashes of the array members.
PR Summary
Fix #24964
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
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).