StronglyTypedId icon indicating copy to clipboard operation
StronglyTypedId copied to clipboard

GetHashCode fails in certain cases when Backing Type is String

Open Miljankg opened this issue 3 years ago • 3 comments

Hi!

Thanks for creating this library, I really like it. As I was using it within my project I had several ids with a backing type of String. On certain occasions, these ids were created using a default, parameterless constructor. When GetHashCode was invoked on those ids the NullReferenceException was thrown, and that what a bit against what I would expect.

I have attached a .CS file containing tests you can run to reproduce the issue (I had some issues with dotnetfiddle).

Tests.txt

Some of my thoughts on this:

  • I found out that in the current beta version, there is a NullableString as a backing type. This is ok and GetHashCode works without issues here in all scenarios, but I would expect that using String as a backing type is reliable. By this, I mean that it is ok that creating such a struct with the default constructor ends up with the Value property being null, but I would not expect GetHashCode to throw.

  • Creating structs with backing type String using default constructor (or default keyword) could potentially be undesirable. I could imagine some use cases where having such "id" would be ok, but Nullable String could be a better fit as a backing type. Could we have an analyzer that would raise a warning about creating structs with reference types like string, using the default constructor? This can prevent both intentional, but also accidental creation of such instances.

In the end, if you opt for addressing this, I would be happy to implement the solution and contribute to your project :)

Miljankg avatar Feb 15 '22 23:02 Miljankg

It's incredibly difficult to fully find and warn of instances where value types contain uninitialised reference types.

I have a similar library to Andrew's, but they concentrate on different things. Mine (Vogen) concentrates on validity of 'Value Objects' (domain concepts). It has analyzers that help you stop creating invalid instances: https://twitter.com/SteveDunn/status/1495894535850844167

I wrote about the issues here: https://dunnhq.com/posts/2022/non-defaultable-value-types/

SteveDunn avatar Feb 21 '22 22:02 SteveDunn

@SteveDunn thanks for the reply and the article. Actually, I was talking about the same thing (analyzers) you have implemented in your library.

As for the GetHashCode throwing NullReferenceException, I think that can be easily avoided in the GetHashCode implementation.

Miljankg avatar Feb 23 '22 21:02 Miljankg

I hope that the analyzers and runtime checks wouldn't even get as far as GetHashCode

SteveDunn avatar Feb 23 '22 22:02 SteveDunn