-
-
Notifications
You must be signed in to change notification settings - Fork 32
Description
I ran into what looks like a real bug in AttributedScopeStack.GetHashCode() that makes the type effectively un-hashable in normal usage.
What’s happening
Current implementation:
public override int GetHashCode()
{
return Parent.GetHashCode() +
ScopePath.GetHashCode() +
TokenAttributes.GetHashCode();
}
Two problems:
- Root nodes always throw: root stacks have Parent == null, so
Parent.GetHashCode()throwsNullReferenceException. - Non-root nodes also throw: even if Parent != null for the leaf node, calling
Parent.GetHashCode()eventually walks up to the root where Parent == null, so the whole chain still throws.
There’s also a second edge case: ScopePath can be null (constructor allows it, and Equals works with nulls), but ScopePath.GetHashCode() will also throw if it’s null.
Why this is a problem
- Any
AttributedScopeStackinstance can’t be used as a key in a Dictionary/HashSet without blowing up. - It’s especially surprising because the type overrides Equals, and Equals already treats
ScopePath == nullas a valid comparable value. - This alsd violates a Microsoft guideline for implementing
GetHashCode:The GetHashCode() method should not throw exceptions.
Repro
var root = new AttributedScopeStack(null, "root", 1);
_ = root.GetHashCode(); // throws
var child = new AttributedScopeStack(root, "child", 2);
_ = child.GetHashCode(); // also throws (hits root.Parent == null)Suggested fix
Make GetHashCode total (never throw) and compute it iteratively instead of delegating to Parent.GetHashCode().
Something along these lines (high level):
- If
Parentis null, treat parent hash contribution as 0 (or a seed). - If
ScopePathis null, treat its hash contribution as 0. - Walk the parent chain in a loop and combine hashes.
Optional: cache the hash since the object is effectively immutable after construction.
Breaking change note
If anyone currently relies on the NullReferenceException behavior (unlikely, but technically possible), this would change observable behavior. I’d still consider it the correct fix for a public API as no public API should ever throw an NRE imo :).
If you’d like, I can also put together a small PR with the fix + unit tests.