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

Incorrect implementation of the RuleIdEqualityComparer.Equals method #3182

Closed
4 tasks done
VasilievSerg opened this issue Jun 1, 2023 · 2 comments · Fixed by #3205
Closed
4 tasks done

Incorrect implementation of the RuleIdEqualityComparer.Equals method #3182

VasilievSerg opened this issue Jun 1, 2023 · 2 comments · Fixed by #3205
Assignees
Milestone

Comments

@VasilievSerg
Copy link

Checklist

  • I have verified this is the correct repository for opening this issue.
  • I have verified no other issues exist related to my problem.
  • I have verified this is not an issue for a specific package.
  • I have verified this issue is not security related.

What You Are Seeing?

It looks like there are 2 errors in the RuleIdEqualityComparer.Equals method implementation.

Link to the sources.

Code fragment:

public bool Equals(ImmutableRule x, ImmutableRule y)
{
    return ReferenceEquals(x, y) || x.Id.IsEqualTo(x.Id);
}

Issues

  1. The result of the ReferenceEquals call will always be false: the type of the x and y variables is ImmutableRule (readonly struct, value type). So, values will be boxed and references to newly created objects will differ.

  2. In the x.Id.IsEqualTo(x.Id) invocation the x.Id expression is compared with itself. It looks like the correct invocation is x.Id.Equals(y.Id).

What is Expected?

No described flaws in the method implementations.

How Did You Get This To Happen?

I've checked the latest sources via the PVS-Studio analyzer.

System Details

Installed Packages

-

Output Log

-

Additional Context

No response

@AdmiringWorm
Copy link
Member

Thank you for reporting this issue. This is indeed an oversight that we need to fix.

It is a minor issue at this moment as the only place that makes use of this Equals call is not meant to be used yet, but rather is a placeholder for future development that is intended.

@AdmiringWorm AdmiringWorm self-assigned this Jun 6, 2023
@AdmiringWorm AdmiringWorm added this to the 2.1.0 milestone Jun 7, 2023
AdmiringWorm added a commit to AdmiringWorm/choco that referenced this issue Jun 8, 2023
When finding the distinct rules that are available in Chocolatey CLI
and its extensions, the equals and GetHashCode was incorrectly
implemented which in some cases causes the incorrect rules to be
returned, or not returned.

This commit updates these two methods to handle the check for
getting the hash code, and comparing immutable rules in the way
that we expect them to behave.
corbob added a commit that referenced this issue Jun 8, 2023
@corbob corbob added 4 - Done and removed 3 - Review labels Jun 8, 2023
@choco-bot
Copy link

🎉 This issue has been resolved in version 2.1.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants