Skip to content

Throw ArgumentNullException on StringFnV1AHasher.Hash(null) - #72

Merged
marius-bughiu merged 2 commits into
mainfrom
fix/string-fnv1a-null-key
May 11, 2026
Merged

Throw ArgumentNullException on StringFnV1AHasher.Hash(null)#72
marius-bughiu merged 2 commits into
mainfrom
fix/string-fnv1a-null-key

Conversation

@marius-bughiu

Copy link
Copy Markdown
Owner

Summary

Closes #71. Replaces the NullReferenceException that StringFnV1AHasher.Hash(null) currently throws with an explicit ArgumentNullException (parameter name "key"), and updates the test that previously pinned the NRE behaviour.

NullReferenceException from a public API is a code-quality wart per .NET framework design guidelines — it conflates a programmer error (unchecked dereference) with what is really a contract violation. The previous test even openly noted the wart: "If you'd prefer an explicit ArgumentNullException, modify the struct." This PR makes that change.

Surface area

The Celerity dictionaries route the out-of-band null / default(TKey) key entry around the hasher and never call Hash(null), so existing dictionary callers see no behaviour change. The new ArgumentNullException only surfaces when:

  • callers use StringFnV1AHasher directly (e.g. as a building block in their own collection or a hash-quality test harness), or
  • callers plug StringFnV1AHasher into a custom IHashProvider<string> consumer that does not handle the null-key slot itself.

In both cases the new exception is the correct one — and the regression test pins it down.

Files

  • src/Celerity/Hashing/StringFnV1AHasher.csArgumentNullException.ThrowIfNull(key) at the top of Hash, plus a full XML doc on the method declaring the exception.
  • src/Celerity.Tests/Hashing/StringFnV1AHasherTests.csHash_NullString_ThrowsNullReferenceException is renamed to Hash_NullString_ThrowsArgumentNullException and asserts both the exception type and ParamName == "key".
  • CHANGELOG.md[Unreleased] Fixed entry.

Test plan

  • dotnet build clean (0 errors; 55 pre-existing xUnit2013 warnings in unrelated test files)
  • CI runs StringFnV1AHasherTests on Linux / Windows / macOS — local runtime is .NET 10 only and the projects target net8.0, so the test execution gate is CI
  • Coverage in StringFnV1AHasherTests.Hash_NullString_ThrowsArgumentNullException:
    • Hash(null!) throws ArgumentNullException
    • The thrown exception's ParamName is "key"
  • No change to existing dictionary code paths — the out-of-band null/default-key handling already short-circuits the hasher call.

Public APIs should signal a null argument as a contract violation, not
as an unchecked dereference. Adds ArgumentNullException.ThrowIfNull at
the top of Hash, declares the exception in the XML doc, and updates the
test that previously pinned the NullReferenceException wart.

In practice the dictionaries route the out-of-band default-key entry
around the hasher, so this only surfaces for direct hasher use or for
custom IHashProvider<string> consumers that don't handle null keys.

Closes #71.
@marius-bughiu

Copy link
Copy Markdown
Owner Author

@copilot resolve the merge conflicts in this pull request

Co-authored-by: marius-bughiu <11870708+marius-bughiu@users.noreply.github.com>

Copilot AI commented May 11, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Merged main into the branch and resolved the CHANGELOG.md conflict — the [Unreleased] ### Fixed entry is preserved above the new [1.2.0] section. Build is clean (0 errors). Commit: 004d36a.

@marius-bughiu
marius-bughiu merged commit 1d6534a into main May 11, 2026
@marius-bughiu
marius-bughiu deleted the fix/string-fnv1a-null-key branch July 24, 2026 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

StringFnV1AHasher.Hash(null) throws NullReferenceException instead of ArgumentNullException

2 participants