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

GetHashCode fails in certain cases when Backing Type is String #49

Open
Miljankg opened this issue Feb 15, 2022 · 3 comments
Open

GetHashCode fails in certain cases when Backing Type is String #49

Miljankg opened this issue Feb 15, 2022 · 3 comments

Comments

@Miljankg
Copy link

Miljankg commented Feb 15, 2022

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 :)

@SteveDunn
Copy link
Contributor

SteveDunn commented Feb 21, 2022

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/

@Miljankg
Copy link
Author

@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.

@SteveDunn
Copy link
Contributor

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

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

No branches or pull requests

2 participants