Skip to content

Conversation

@marler8997
Copy link
Contributor

@marler8997 marler8997 commented Mar 12, 2018

Make Nullable a template so that it can be modified to "select" different implementations. This is in preparation for #6253

The plan is to make Nullable an alias to either NullableWithExtraField!T or Nullable!(T, nullValue) depending on if the type has a "sane" default null value (i.e. classes/interfaces/pointers/dynamic arrays/some enums). One reason for doing this is that Nullable!(T, nullValue) does not add an extra field, allowing values like classes/enums/pointers to only take one register.

Note, I think making NullableWithExtraField a public struct is useful since it allows an application to force a type to have an extra "null" state even if the type has a "null" value. However, maybe someone can come up with a better name?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

For example, if `T` is a class, assigning the value to `null` does not nullify
this struct.
*/
struct NullableWithExtraField(T)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is a new public symbol. Can't we move this into the eponymous template and thus make it private for now (and also save on template instantiations)?

Copy link
Contributor Author

@marler8997 marler8997 Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, I just finished editing the description to explain why I think it should be "public" :) But I don't really like the name...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but what's the use case for this? Is this just to simply the deprecation period?
If we really have to do so, can't we use an additional flag like Nullable!(ExtraField.Yes, T, nullValue)? Adding new public symbols shouldn't be done lightly. Once added, we never get rid of them.

@JackStouffer
Copy link
Contributor

JackStouffer commented Mar 12, 2018

Adding a new name for small changes in behavior ... this smells like PHP.

If we consider Nullable!Object(null).isNull == false as a bug rather than a feature, we would be justified in "fixing" it with a deprecation.

@marler8997
Copy link
Contributor Author

marler8997 commented Mar 12, 2018

If we consider Nullable!Object(null).isNull == false as a bug rather than a feature, we would be justified in "fixing" it with a deprecation.

Agreed. However, I was thinking that this was still a valid use case, though the names are confusing. Currently, Nullable is just adding an extra state to any value, and it happens to be called a "nullified" state, even though it is different from a "null" value. If it was called something like Optional we would have:

Optional!Object(null).hasValue == true
// same as
Nullable!Object(null).isNull == false

Even though these are essentially the same thing, the Nullable version is confusing because the "nullified" state is conflated with the "null" value.

Maybe we should create a new type named Optional instead?

@n8sh
Copy link
Member

n8sh commented Mar 13, 2018

The plan is to make Nullable an alias to either NullableWithExtraField!T or Nullable!(T, nullValue) depending on if the type has a "sane" default null value (i.e. classes/interfaces/pointers/dynamic arrays/some enums).

Is there a reason for them not to be both named Nullable?

@marler8997
Copy link
Contributor Author

marler8997 commented Mar 13, 2018

Let me summarize the proposal so far.

Currently we have two Nullable template structs:

// Wrap a value of type T with an extra "nullified" state by using an extra bool field
// NOTE: this extra state is added regardless of if T can already be set to null
struct Nullable(T) { ... }

// Wrap a value of type T and interpret the state as "nullified" when the value is
// set to `nullValue`
struct Nullable(T, T nullValue) { ... }

The latest idea is to:

  1. Modify Nullable(T) to either be an alias to Nullable(T, T nullValue) if T has a nullValue, or, have it behave like it does today.
  2. Create a new Optional struct that behaves like the current Nullable(T) template. This means it always adds a bool field to represent the "no value" state.
// If T has a null value then this is an alias to Nullable(T, T nullValue), otherwise,
// it is a value/boolean pair where the boolean is used to represent an extra "nullfied"
// state in place of not having a null value.
template Nullable(T)
{
    static if (hasNullValue!T)
    {
        alias Nullable = Nullable!(T, nullValue!T);
    }
    else
    {
        struct Nullable
        {
            // current definition of Nullable ...
            // maybe this would just use an instance of Optional
            // with some wrapper methods that translate isNull to hasValue or something
        }
    }
}

// Wrap a value of type T and interpret the state as "nullified" when the value is
// set to `nullValue`
// NOTE: this is not changed from the current definition
struct Nullable(T, T nullValue) { ... }

// Wrap a value of type T with an extra "no value" state by using an extra bool field
// NOTE: This is basically the same as the current Nullable(T) template struct
struct Optional(T)
{
    private T _value;
    private bool _hasValue;
    bool hasValue() const { return _hasValue; }
    bool setNoValue() { _hasValue = true; }
    // ...
}

@jmdavis
Copy link
Member

jmdavis commented Mar 15, 2018

If we consider Nullable!Object(null).isNull == false as a bug rather than a feature, we would be justified in "fixing" it with a deprecation.

As I just posted in #625, I really don't think that the current behavior is a bug. Rather, distinguishing between a value of null and the lack of a value is sometimes useful and would be a good reason to use Nullable rather than just using a class reference or pointer directly. If you didn't care about that distinction, then really the only reason to use Nullable with a class or pointer would be generic code, and having Nullable act differently for types that can be null on their own makes it behave badly in generic code. So, I think that it would be a big mistake to change the behavior of Nullable to treat nullable types differently from other types.

However, I also don't think that complicating things here by trying to create new versions of Nullable is worth it. If someone really wants a pointer to only take up the space of a pointer, then they can just use a pointer directly rather than Nullable.

@MetaLang
Copy link
Member

MetaLang commented Mar 17, 2018

Let me reiterate; as I said on your other PR, that I don't agree with these changes.

@marler8997
Copy link
Contributor Author

@MetaLang the plan at this point isn't to integrate this change as it exists. The plan would be to introduce a new Optional type without modifying Nullable. It would behave the same as Nullable except you would call hasValue instead of isNull so that people wouldn't confuse "null values" with isNull. i.e.

// this is what exists today that is confusing
assert(Nullable!MyClass(null).isNull == false);

// this is the proposed alternative
assert(Optional!MyClass(null).hasValue == true);

I think everyone agrees that Optional is less confusing, the question is whether or not it's worth adding. If Optional is added, then at some point Nullable could be modified to "collapse" "null values" and "isNull" into the same state, with the hope being that anyone not wanting this would have switched over to using Optional instead, which doesn't collapse the 2 states.

Note that the reason for these changes are

  1. isNull and "null values" being different is confusing
  2. Nullable uses an extra boolean field in many cases where it doesn't need to (prevents it from fitting into a single register)

Adding Optional addresses number 1, and modifying Nullable addresses number 2.

@marler8997
Copy link
Contributor Author

I'm interested in people's opinions of introducing the proposed Optional type as described above. What do people think?

@jmdavis
Copy link
Member

jmdavis commented Apr 6, 2018

I'm interested in people's opinions of introducing the proposed Optional type as described above. What do people think?

Well, I think that I've made it fairly clear that I'm against it. Others will have to comment for themselves.

@marler8997
Copy link
Contributor Author

marler8997 commented Apr 6, 2018

Ok, @jmdavis doesn't think the introduction of Optional is justified. Fair enough (I'm still on the fence myself).

Let me summarize the proposal for anyone else to chime in.

The first reason for introducing Optional is to fix the isNull/"null value" confusion. These are 2 different states but the naming makes it seem like they are the same. With Optional, you would have hasValue and "null value". With these names, it's harder to confuse them for the same state.

This reason alone may or may not justify introducing Optional into the library. The other reason is that by introducing Optional, it makes it more feasible to modify Nullable to collapse null values with isNull. Collapsing these 2 states allows Nullable to be more efficient with how it stores the nullified state. It removes the need to introduce an extra bool field which means most values will still fit inside a single register. Introducing Optional makes this change more feasible because those not wanting to collapse the "nullified" state with null values will have the "Option" to use the Optional type instead (pun intended).

@marler8997
Copy link
Contributor Author

Closing this PR and the matter of adding the Optional type. The only person to weigh in was @jmdavis who is against the addition.

@marler8997 marler8997 closed this Apr 12, 2018
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.

7 participants