Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix PSMethodInvocationConstraints.GetHashCode method#24965

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
iSazonov merged 1 commit intoPowerShell:masterfromcrazyjncsu:fix/24964
Feb 13, 2025

Conversation

crazyjncsu
Copy link
Contributor

@crazyjncsucrazyjncsu commentedFeb 6, 2025•
edited by iSazonov
Loading

PR Summary

Fix#24964

PR Context

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

PR Checklist

0xfeeddeadbeef reacted with thumbs up emoji
@crazyjncsu

This comment was marked as duplicate.

@iSazonoviSazonov added the CL-GeneralIndicates that a PR should be marked as a general cmdlet change in the Change Log labelFeb 10, 2025
@iSazonoviSazonov changed the titlefixed gethashcode calculationFix PSMethodInvocationConstraints.GetHashCode methodFeb 10, 2025
@daxian-dbw
Copy link
Member

daxian-dbw commentedFeb 11, 2025•
edited
Loading

@crazyjncsu Thank you for reporting this issue!
I think we should either revert the change back to theoriginal hash code calculation with the addition ofParameterTypes.SequenceGetHashCode(), or useHashCode.Add to include all elements ofParameterTypes andGenericTypeParameters to the hash code calculation.

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

@daxian-dbw
Copy link
Member

/azp run

@azure-pipelinesAzure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@crazyjncsu
Copy link
ContributorAuthor

Looks great for my purposes ;)

@iSazonov
Copy link
Collaborator

iSazonov commentedFeb 12, 2025•
edited
Loading

@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.)

Comment on lines 2013 to 2023
// 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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
{
intresult=61;
result=result*397+(MethodTargetType!=null?MethodTargetType.GetHashCode():0);
result=result*397+ParameterTypes.SequenceGetHashCode();
result=result*397+GenericTypeParameters.SequenceGetHashCode();
returnresult;
}
HashCodehashCode=new();
hashCode.Add(MethodTargetType);
if(ParameterTypesis notnull)
{
foreach(TypeparameterTypeinParameterTypes)
{
hashCode.Add(parameterType);
}
}
else
{
hashCode.Add(0);
}
if(GenericTypeParametersisnull)
{
hashCode.Add(0);
returnhashCode.ToHashCode();
}
foreach(TypegenericParameterinGenericTypeParameters)
{
hashCode.Add(genericParameter);
}
returnhashCode.ToHashCode();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

HashCode.Combine callsGetHashCode on the passed-in values (seeits source code here), butInt32.GetHashCode() simply returns the integer itself, soHashCode.Combine(MethodTargetType, ParameterTypes.SequenceGetHashCode(), GenericTypeParameters.SequenceGetHashCode()) should work as expected.

SeeminglyScience reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

but Int32.GetHashCode() simply returns the integer itself

Also given this, I guess we now can replace the utility methodUtils.CombineHashCodes in PowerShell withHashCode.Combine.

Copy link
Collaborator

@iSazonoviSazonovFeb 13, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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:

  1. Less collisions.
  2. Random seed per process (protects against cache attacks).

@daxian-dbw
Copy link
Member

daxian-dbw commentedFeb 12, 2025•
edited
Loading

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

I have removed my commit.

iSazonov reacted with eyes emoji

@iSazonov

This comment was marked as outdated.

@azure-pipelinesAzure Pipelines

This comment was marked as outdated.

@iSazonoviSazonov self-assigned thisFeb 13, 2025
@iSazonoviSazonov merged commit14fb6f2 intoPowerShell:masterFeb 13, 2025
68 of 70 checks passed
@microsoft-github-policy-serviceMicrosoft GitHub Policy Service
Copy link
Contributor

microsoft-github-policy-servicebot commentedFeb 13, 2025•
edited by unfurl-linksbot
Loading

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

🔗https://aka.ms/PSRepoFeedback

@iSazonov
Copy link
Collaborator

iSazonov commentedFeb 13, 2025•
edited
Loading

@crazyjncsu Thanks for you contribution!

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

pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull requestApr 10, 2025
There is a difference between GetHashCode for array object itself and cumulative GetHashCode based on hashes of the array members.
@TravisEz13
Copy link
Member

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

pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull requestMay 14, 2025
There is a difference between GetHashCode for array object itself and cumulative GetHashCode based on hashes of the array members.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@iSazonoviSazonoviSazonov left review comments

@SeeminglyScienceSeeminglyScienceSeeminglyScience left review comments

@daxian-dbwdaxian-dbwdaxian-dbw approved these changes

Assignees

@iSazonoviSazonov

Labels
Backport-7.5.x-MigratedCL-GeneralIndicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bad hash code calculation inPSMethodInvocationConstraints results in explosion of static cache
5 participants
@crazyjncsu@daxian-dbw@iSazonov@TravisEz13@SeeminglyScience
[8]ページ先頭
©2009-2025 Movatter.jp