Skip to content

C#: Improve the query cs/gethashcode-is-not-defined. #19497

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

Merged
merged 5 commits into from
May 16, 2025

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented May 15, 2025

Even though there is a general compiler warning CS0659 for all types overriding Equals but not GetHashCode - this query only targets expressions of types, where it is more likely to have an impact that GetHashCode() is not overridden.

@michaelnebel
Copy link
Contributor Author

DCA looks good; We found a couple of extra results, but the query also performs slightly worse.

@michaelnebel michaelnebel marked this pull request as ready for review May 16, 2025 06:07
@Copilot Copilot AI review requested due to automatic review settings May 16, 2025 06:07
@michaelnebel michaelnebel requested a review from a team as a code owner May 16, 2025 06:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Enhance the cs/gethashcode-is-not-defined query to catch more hashing usages across both method calls and indexers, backed by new framework definitions and expanded tests.

  • Extend the main QL predicate (usesHashing) to include more methods and indexer accesses.
  • Add support for IReadOnlyDictionary and HashSet in the framework library.
  • Update test suite and change notes to reflect the broader detection.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
csharp/ql/src/Likely Bugs/HashedButNoHash.ql Updated imports and predicates to cover additional scenarios, renamed and refactored predicates (usesHashing, dictionary, hashSet, hashStructure).
csharp/ql/lib/semmle/code/csharp/frameworks/system/collections/Generic.qll Added IReadOnlyDictionary and HashSet class definitions.
csharp/ql/src/change-notes/2025-05-15-gethashcode-is-not-defined.md Added change note documenting the precision improvements.
csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.qlref Adjusted test metadata to use query: and postprocess: directives.
csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.expected Expanded expected alerts to cover the new hashing calls.
csharp/ql/test/query-tests/Likely Bugs/HashedButNoHash/HashedButNoHash.cs Added more hashing-related calls (Contains, Remove, TryAdd, TryGetValue) to the test case.
Comments suppressed due to low confidence (3)

csharp/ql/src/Likely Bugs/HashedButNoHash.ql:29

  • The Javadoc comment refers to parameter c but the predicate declares parameter t; consider updating the comment to match the actual parameter name for clarity.
/** Holds if `c` is a hashset type. */

csharp/ql/src/Likely Bugs/HashedButNoHash.ql:14

  • The import path uses an uppercase 'Collections' segment, which may be inconsistent with the lowercase directory naming used elsewhere; please verify the correct casing for the module path.
import semmle.code.csharp.frameworks.system.Collections

csharp/ql/src/change-notes/2025-05-15-gethashcode-is-not-defined.md:1

  • [nitpick] The frontmatter defines only category; to maintain consistency with other change notes, consider adding a title field or confirming this matches the existing style conventions.
---

@michaelnebel michaelnebel merged commit 5e72b8b into github:main May 16, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants