Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSSignatureContext.cs may include incorrect use of Math.Abs #84996

Closed
tfenise opened this issue Apr 18, 2023 · 4 comments · Fixed by #110539 or #110606
Closed

JSSignatureContext.cs may include incorrect use of Math.Abs #84996

tfenise opened this issue Apr 18, 2023 · 4 comments · Fixed by #110539 or #110606
Assignees
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript in-pr There is an active PR which will close this issue when it is merged os-browser Browser variant of arch-wasm
Milestone

Comments

@tfenise
Copy link
Contributor

tfenise commented Apr 18, 2023

// there could be multiple method signatures with the same name, get unique signature name
uint hash = 17;
unchecked
{
foreach (var param in sigContext.ElementTypeInformation)
{
hash = hash * 31 + (uint)param.ManagedType.FullTypeName.GetHashCode();
}
};
int typesHash = Math.Abs((int)hash);

The last line does not seem correct. If (int)hash == int.MinValue, Math.Abs would throw, which I suppose is not the intended behaviour for a hash function. I would suggest simply int typesHash = (int)hash & int.MaxValue;, or if the hash code is required to be persistent across versions, int typesHash = (int)hash == int.MinValue ? int.MaxValue : Math.Abs((int)hash).

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 18, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 18, 2023
@EgorBo
Copy link
Member

EgorBo commented Apr 18, 2023

It should've used HashCode.Combine but I'm not familiar with what this API does and if the changed hash code impl is a breaking change or not

@EgorBo EgorBo added area-System.Runtime.InteropServices.JavaScript and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 18, 2023
@marek-safar marek-safar removed the untriaged New issue has not been triaged by the area owner label Jul 24, 2023
@marek-safar marek-safar added this to the 8.0.0 milestone Jul 24, 2023
@pavelsavara pavelsavara modified the milestones: 8.0.0, 10.0.0 Dec 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Dec 9, 2024
@pavelsavara
Copy link
Member

It doesn't have to be consistent across versions. This just prevents conflict in the names on the same assembly, type and method name.

@pavelsavara pavelsavara self-assigned this Dec 9, 2024
@pavelsavara pavelsavara added arch-wasm WebAssembly architecture os-browser Browser variant of arch-wasm labels Dec 9, 2024
Copy link
Contributor

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky
Copy link
Member

The fix was reverted in #110599

@github-actions github-actions bot locked and limited conversation to collaborators Jan 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript in-pr There is an active PR which will close this issue when it is merged os-browser Browser variant of arch-wasm
Projects
None yet
5 participants