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

Remove Maybe type and change Option to be a readonly struct #57

Closed
DavidArno opened this issue Jun 21, 2018 · 5 comments
Closed

Remove Maybe type and change Option to be a readonly struct #57

DavidArno opened this issue Jun 21, 2018 · 5 comments

Comments

@DavidArno
Copy link
Owner

This is a major breaking change:

  1. Any code using Maybe<T> will need to change to use Option<T>.
  2. In line with the conversation in Maybe.Some(null) does not throw and has a different behavior than Maybe.None() #51, the way Option<T> handles null changes:
    i. Option<string>.Some(null) will throw an ArgumentNullException.
    ii. As the type supports implicit casting of T to Option<T>, casting null in this way will result in a None:
Option<string> x = null`
AreEqual(Option<int>.None(), result); // test passes
@DavidArno
Copy link
Owner Author

Have abandoned this idea as read only structs are just weird.

@SugoiDev
Copy link

Do you have any insight on the weirdness you found?
I was planning some changes on a codebase using readonly structs soon.

@DavidArno DavidArno reopened this Aug 27, 2019
@DavidArno
Copy link
Owner Author

Well yes, I do. And having gone full circle, I'm re-opening this.

The reason I claimed that readonly structs are weird is due to the distinction between "read only" and "immutable". In C#, readonly denotes that the value of that item cannot change. This means that I might have the class,

class C
{
    readonly List<int> _x = new List<int>();

    void ReadonlyNotImmutable() => _x.Add(1);
}

The "value" of _x can't change, ie it is fixed to referring to the same List<int> object. But the contents can change, ie it's readonly not immutable.

Readonly structs apply this same rule, but it creates odd results. So for example, the following code:

class FakeTuple
{
    public int a;
    public int b;
}

readonly struct S
{
    readonly (int a, int b) _x;
    readonly FakeTuple _y;
    
    public S(int n)
    {
        _x = (n, n);
        _y = new FakeTuple { a = n, b = n };
    }
    
    int X { set { _x.a = value; } }           
    int Y { set { _y.a = value; }}
}

gives an error on _x.a = value; but not on _y.a = value;. Further, despite marking it as readonly, I can still have setters. So I really struggled with the idea that it's readonly as to me, that ought to rule out setters.

But what I was missing is the simple idea that, just like with a readonly field, a readonly struct guarantees its value cannot change. And the value of a struct is the value of its fields. So the tuple, being a struct itself, cannot change the value of its fields as that would change its own value and thus the value of the struct, S. Likewise the reference for the FakeTuple field can't change, but items it contains can, as they aren't part of the struct's value. And setters are allowed, as long as they don't change the value of the struct.

So having had that lightbulb moment, I've gone back and re-introduced that readonly struct for Option. 😄

@DavidArno
Copy link
Owner Author

DavidArno commented Sep 6, 2019

Adding a checklist here for all the types that can become readonly structs:

  • Any
  • Either
  • EitherTuple<T1, T2> (internal type)
  • EitherTuple<T1, T2, T3> (internal type)
  • EitherTuple<T1, T2, T3, T4> (internal type)
  • None
  • Option<T>
  • Success<T>
  • ValueOrError
  • ValueOrError<TV, TE>
  • Union<T1, T2>
  • Union<T1, T2, T3>
  • Union<T1, T2, T3, T4>
  • Unit

DavidArno added a commit that referenced this issue Sep 9, 2019
… classes in SuccincT are now either sealed or static.
@DavidArno
Copy link
Owner Author

Maybe<T> is gone and Option<T> is now a read-only struct.

Closing as implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants