Remove extra field from Nullable when unnecessary #6253

marler8997 · 2018-03-08T06:02:47Z

Some types like classes/interfaces and most enum types can be "nullified" by changing their value, rather than adding an extra field to hold this data. This change utilizes this "null" value for classes/interfaces and when it applies to enum types as well.

dlang-bot · 2018-03-08T06:02:48Z

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.

wilzbach

I think you do this without NoNullValue

wilzbach · 2018-03-08T06:15:11Z

std/typecons.d

+    {
+        private enum nullValue = NoNullValue._;
+    }
+    private enum hasNullValue = !is(typeof(nullValue) == NoNullValue);


I think you can entirely skip NoNullValue and simply use false or void.

JackStouffer · 2018-03-08T13:21:52Z

std/typecons.d

+    }
+    else static if (is(T == class) || is(T == interface))
+    {
+        private enum nullValue = cast(T)null;


space after cast

Same throughout

JackStouffer · 2018-03-08T13:29:13Z

std/typecons.d

    {
-        return _isNull;
+        static if (hasNullValue)
+            return _value is nullValue;


This doesn't work in the enum case.

It passes the unittests...

JackStouffer · 2018-03-08T13:31:42Z

std/typecons.d

+    }

+    enum Foo2 : int {a = int.min}
+    assert(Nullable!Foo2.sizeof == Foo2.sizeof);


static assert

JackStouffer · 2018-03-08T13:32:33Z

std/typecons.d

+    class FooInterface { }
+    assert(Nullable!FooInterface.sizeof == FooInterface.sizeof);
+}
+@safe unittest


What's the purpose of this test?

Because nullValue == false when the base type is bool, I think he wanted to ensure that the code won't accidently consider non-null false being null, or something like that. I think that's a good rationale.

MetaLang · 2018-03-08T14:30:39Z

This is an awful lot of extra special casing and ugly bloat for something that seems minimally useful to the end user.

JackStouffer · 2018-03-08T14:54:23Z

Well, for the cases covered here, the Nullable does now end up fitting into one register.

MetaLang · 2018-03-08T16:17:20Z

This will also break code:

Nullable!Object no = null;
assert(!no.isNull); //Currently passes

Although the current behaviour is more an accident than anything else.

dukc · 2018-03-08T16:25:25Z

This is an awful lot of extra special casing and ugly bloat for something that seems minimally useful
to the end user.

Since D is a systems programming language, I would hardly consider cutting size of Nullable!SomeEnumType in half being minimally useful. Good work.

dukc · 2018-03-08T16:27:50Z

Nullable!Object no = null;
assert(!no.isNull); //Currently passes

If somebody does that, he fairly much deserves what he's getting here.

JackStouffer · 2018-03-08T16:31:13Z

The entire concept of a Nullable class is very odd to begin with and probably should have been disallowed.

In any case, this needs a changelog entry detailing the breakage.

thewilsonator · 2018-03-10T14:15:01Z

You've covered classes and interfaces, what about pointers?

Some types like classes/interfaces and most enum types can be "nullified" by changing their value, rather than adding an extra field to hold this data. This change utilizes this "null" value when the the type has an applicable "null value".

marler8997 · 2018-03-10T16:32:38Z

std/typecons.d

    Nullable!(int*) npi;
    assert(npi.isNull);

-    //Passes?!


wilzbach

I think this is a useful change and the complication is worth it as fitting in one register will result in quite a speed-up for classes et.al.

wilzbach · 2018-03-11T15:37:44Z

@marler8997 Have a look at the failing diet-ng test. That seems to be a real error (I just restarted Jenkins to test it and it doesn't appear on other PRs).

s-ludwig · 2018-03-11T16:04:19Z

diet-ng is probably hitting the "Passes?!" case with an implicit null array being assigned. I think the new behavior is definitely what it should have been from the start, but I don't think we can introduce such subtle logic changes without warnings or a deprecation phase.

JackStouffer · 2018-03-11T16:30:21Z

I was initially going to require a deprecation, but I realized there's no easy way to have a deprecation for the new behavior without creating a different isNull equivalent function.

Any ideas on how to make the transition smoother?

marler8997 · 2018-03-12T16:10:52Z

I'm at a loss. I'm not sure what we could deprecate or warn about. Both assigning a Nullable struct to null and calling isNull are still fine, it's just that doing both will result it in returning true instead of false now. The only thing that makes sense to me is to warn/deprecate the "expectation" of this use case returning false...but how do you warn/deprecate a program's expectations?

Maybe someone can come up with a brilliant idea to do this, I have been surprised before when it comes to what D can do. But if no one can come up with a solution, do we just leave Nullable as is?

JackStouffer · 2018-03-12T16:14:08Z

The only other option I can think of is having a version block with the old Nullable in it marked as deprecated. The else clause would have the new Nullable. That way you give people a chance to get the old behavior explicitly at compile time if they need it.

marler8997 · 2018-03-12T16:21:37Z

A version would be simple to add...what would you call it?

JackStouffer · 2018-03-12T16:31:14Z

Transition_Nullable

JackStouffer · 2018-03-12T16:38:28Z

We need a way to emphasize breaking changes in the changelog.

For now, can you manually add $(RED Breaking Change:) to the start of the first sentence in your entry?

marler8997 · 2018-03-12T17:42:03Z

In looking at the implementation a bit more I think a better way to implement this would be to make Nullable an alias to one of 2 implementations, one where an extra bool field is used and one where a nullValue is used. I've made a separate PR for this change #6270

jmdavis · 2018-03-15T21:31:32Z

Honestly, I think that the assumption that it's not desirable and valid to have a Nullable!(T*) where isNull is false and the value is null is a bad assumption. It's assuming that T* and Nullable!(T*) should be treated as semantically equivalent. But null is a perfectly valid value for a pointer, and sometimes it can be useful to treat the absence of a value as being different from a specific value can be useful and important.

For instance, I've sometimes found it annoying how getopt works. It sets associated variable if the flag is present, but there's no way to know whether the flag was present or not if the value provided matched the default value for that type. To an extent, that can be gotten around by doing stuff like setting an int to -1 when negative values aren't acceptable but 0 is, but nothing like that works in the general case. If getopt were set up to use Nullable, then it becomes trivial to figure out whether the flag was present or whether it was given the default value for a type.

jmdavis · 2018-03-15T21:33:45Z

Whoops. I accidentally submitted too soon.

Anyway, while pointers and classes don't apply to getopt, the principle is the same. There are cases where it's valid and valuable to differentiate between a default value - even null - and the lack of a value. So, not only is tweaking Nullable as is done here something that potentially breaks code, but it makes Nullable less useful and makes it behave inconsistently for nullable types in comparison to other types.

marler8997 · 2018-03-15T21:34:41Z

@jmdavis Agreed. You've basically restated my point. However, it is confusing that the return value of isNull is not the same as a "null value". That's why the proposal is to use Optional and hasValue instead.

jmdavis · 2018-03-15T21:43:14Z

@jmdavis Agreed. You've basically restated my point. However, it is confusing that the return value of isNull is not the same as a "null value". That's why the proposal is to use Optional and hasValue instead.

I grant you that that could be confusing, and maybe hasValue would have been a better name, but I don't think that renaming things like that are worth the code breakage at this point, and Andrei almost always vetoes them for precisely that reason.

marler8997 · 2018-03-15T21:47:05Z

Adding Optional doesn't break any code.

jmdavis · 2018-03-15T22:02:47Z

Adding Optional doesn't break any code.

No, but changing Nullable.isNull would, and Optional is just another version of Nullable, which would increase the confusion around Nullable without providing much additional value. I have a very hard time believing that you would ever convince Andrei to either accept changing Nullable.isNull to Nullable.hasValue or allow something like Optional to be added. Nullable already solves the problem. If anyone doesn't like the fact that a nullable type takes up extra space when put in a Nullable, then they can just use the nullable type directly. Adding another type to the standard library which is almost the same as Nullable just for that case is overkill, and it would result everyone who dealt with them having to spend time figuring out what the subtle difference was between them.

Also, Nullable / Optional / whatever cannot behave well in generic code if has different semantics for nullable and non-nullable types. So, it's arguably a bad plan to have a type like that treat null as special in the first place. It would work in some cases, but any time that null was involved, it would result in subtle bugs. To fix those bugs, either the current version of Nullable would have to be used instead, or the code would have to be special-cased for types that can be nullable on their own, making that portion of the code non-generic, and at that point, you might as well just use the nullable type directly.

quickfur · 2018-03-15T22:23:44Z

This spectre keeps coming up, but, all things considered, it's unlikely Nullable can be significantly improved without undesirable consequences. See my previous attempt at doing essentially the same thing that eventually was abandoned because it would break too much existing code.

What we need is a new design that is properly thought-out, to be a replacement for Nullable, like an Optional type that's closer in behaviour to the analogous construct in, say, Haskell. It's just too hard to work with the existing limitations of Nullable in a way that won't introduce gratuitous breakage in hard-to-detect ways, that will almost certainly provoke users' anger.

marler8997 · 2018-03-15T22:27:15Z

@quickfur You may be right, but the only breakage that my change was proposing was to have Nullable.isNull return true when it has a "null value". I was anticipating this to break very little code. And then introduce the Optional type to replace the current semantics of Nullable which is to always add a new state to the value to represent "no value". Would your change have resulted in more breakage or the same amount?

marler8997 · 2018-03-15T22:28:16Z

#6038 (comment)

Maybe it's time to write a proper Optional or Maybe type that addresses these issues.

Wow...deja vu...

jmdavis · 2018-03-15T22:32:14Z

What we need is a new design that is properly thought-out, to be a replacement for Nullable, like an Optional type that's closer in behaviour to the analogous construct in, say, Haskell. It's just too hard to work with the existing limitations of Nullable in a way that won't introduce gratuitous breakage in hard-to-detect ways, that will almost certainly provoke users' anger.

What is actually wrong with Nullable as-is? All I see being complained about at the moment is that when you have a nullable type in a Nullable, Nullable.isNull is badly named, and it's annoying that Nullable has an extra bool in it when the type is already nullable. Maybe isNull isn't the best name in retrospect, but we're not going to break code over that, and having Nullable treat null as special rather than having a separate bool does not behave well in generic code and would result in subtle bugs. If anyone really doesn't want the extra bool, the type is nullable. So, it can be used on its own. So, I'd argue that an Optional / Nullable / whatever type that treats nullable types as special would be a big mistake. Aside from someone disagreeing with me on that and disliking isNull, what is it that is wrong with Nullable? As far as I can tell, it works just fine as-is.

marler8997 · 2018-03-15T22:34:46Z

Aside from someone disagreeing with me on that and disliking isNull, what is it that is wrong with Nullable? As far as I can tell, it works just fine as-is.

isNull is easily conflated with null values
It always adds a bool field when it doesn't always need to

Let's stop repeating the same arguments and stop asking the same questions. Repetition is tiring when it comes to constructive conversation.

jmdavis · 2018-03-15T22:36:53Z

Well, based on those points, I'm in complete disagreement that a replacement for Nullable is needed or desirable. Yes, it's less than ideal that isNull can be false when Nullable!(T*) holds a null value, but it's not worth changing or replacing Nullable, and not always have a bool in Nullable would be a huge mistake IMHO.

marler8997 · 2018-03-15T22:38:12Z

Well, based on those points, I'm in complete disagreement that a replacement for Nullable is needed or desirable. Yes, it's less than ideal that isNull can be false when Nullable!(T*) holds a null value, but it's not worth changing or replacing Nullable, and not always have a bool in Nullable would be a huge mistake IMHO.

Yes you've repeated this multiple times. You have made your opinion heard. I don't strongly agree or disagree with it, but repeating your opinion over and over doesn't help anyone.

marler8997 · 2018-03-15T22:40:48Z

BTW, if we went forward with Optional, I think the "compatible" course would be to introduce Optional first without modifying Nullable to give people a chance to migrate to Optional if that's really the semantics they are looking for. Then when enough time has passed, Nullable could be modified to be an alias to Nullable(T, T nullValue) when it makes sense (i.e. classes, pointers, arrays, some enums, etc).

quickfur · 2018-03-15T23:10:24Z

The point of contention here is really an ideological one, and centers on the question of whether you consider a reference type like a class as having null as part of its set of defined values, or whether you consider null as a separate thing indicating that the class does not have a value.

Poor naming aside, it seems reasonable in some situations to consider that null is a part of the set of valid values for a class variable; for example, if you are writing a D interpreter (as a hypothetical example), and you need to tell the difference between a particular class variable being defined to be null, vs. said variable not being defined at all (isNull).

In other situations, of course, such as when you want to make explicit in your API when a function can return null (e.g., write Nullable!MyClass func to make it clear that the caller should be prepared to handle a null return, vs. MyClass func where it's not clear whether or not the function guarantees that the returned reference will not be null), then it doesn't make sense to distinguish between null and isNull, and it would be preferable to merge the two into the same concept.

The current implementation makes me think that the original author likely did not consider instantiating it with a reference type (the code comments seem to point to the expectation that it would only be used with a value type -- and the lack of sig constraint does not help in this regard), so the above distinction likely was never considered, leading us to the unfortunate situation today where one could argue either way, with no clear answer as to what's the "right thing" to do. i.e., reasonable people will disagree, and no matter which way you choose, the other group of people will be unhappy.

marler8997 · 2018-03-15T23:15:50Z

@quickfur I agree with your analysis. My proposal was to have Optional represent the one use case and have Nullable represent the other. Allow an application to choose what semantics they want.

JackStouffer · 2018-03-16T13:46:59Z

Taking the above conversation in mind, here's how we can move forward on this PR.

Remove the special cases for pointers, classes, and interfaces.
Keep the change for enums.
Remove the changelog entry.

JackStouffer · 2018-04-09T14:53:49Z

@marler8997 Ping.

marler8997 · 2018-04-09T15:17:42Z

I'm waiting for people to chime in on whether we should introduce the Optional type (see more discussion here #6270). So far the only one to offer an opinion on it is @jmdavis who doesn't think it is justified. If we don't introduce the Optional type, then I'm thinking that Nullable should remain how it is so that developers will still be able to have the semantics available to keep "null values" and the "nullfied" state as "separate states".

In other words, I don't think I want to move forward making Nullable collapse the "nullfied" state with "null values" unless there is an alternative that apps can use that doesn't do this.

marler8997 · 2018-04-12T13:03:46Z

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 requested review from MetaLang and PetarKirov as code owners March 8, 2018 06:02

marler8997 force-pushed the nullable branch 2 times, most recently from 143c83f to d2fc394 Compare March 8, 2018 06:05

wilzbach reviewed Mar 8, 2018

View reviewed changes

marler8997 force-pushed the nullable branch from d2fc394 to 704e11c Compare March 8, 2018 06:24

JackStouffer suggested changes Mar 8, 2018

View reviewed changes

marler8997 force-pushed the nullable branch 4 times, most recently from 7273774 to 8ce241e Compare March 9, 2018 21:14

marler8997 force-pushed the nullable branch from 8ce241e to 9ed807a Compare March 10, 2018 16:32

marler8997 commented Mar 10, 2018

View reviewed changes

std/typecons.d

Nullable!(int*) npi;

assert(npi.isNull);

//Passes?!

Copy link

Contributor Author

marler8997 Mar 10, 2018

Choose a reason for hiding this comment

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

lol :)

wilzbach reacted with laugh emoji

JackStouffer approved these changes Mar 10, 2018

View reviewed changes

wilzbach approved these changes Mar 11, 2018

View reviewed changes

marler8997 mentioned this pull request Mar 12, 2018

Make Nullable a template #6270

Closed

JackStouffer added Review:Needs Work Severity:Optimization labels Mar 23, 2018

marler8997 closed this Apr 12, 2018

Uh oh!

Remove extra field from Nullable when unnecessary #6253

Remove extra field from Nullable when unnecessary #6253

Uh oh!

Conversation

marler8997 commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Mar 8, 2018

Bugzilla references

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

JackStouffer Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

JackStouffer Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

marler8997 Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

JackStouffer Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

JackStouffer Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

dukc Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetaLang commented Mar 8, 2018

Uh oh!

JackStouffer commented Mar 8, 2018

Uh oh!

MetaLang commented Mar 8, 2018

Uh oh!

dukc commented Mar 8, 2018

Uh oh!

dukc commented Mar 8, 2018

Uh oh!

JackStouffer commented Mar 8, 2018

Uh oh!

thewilsonator commented Mar 10, 2018

Uh oh!

marler8997 Mar 10, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 11, 2018

Uh oh!

s-ludwig commented Mar 11, 2018

Uh oh!

JackStouffer commented Mar 11, 2018

Uh oh!

marler8997 commented Mar 12, 2018

Uh oh!

JackStouffer commented Mar 12, 2018

Uh oh!

marler8997 commented Mar 12, 2018

Uh oh!

JackStouffer commented Mar 12, 2018

Uh oh!

JackStouffer commented Mar 12, 2018

Uh oh!

marler8997 commented Mar 12, 2018

Uh oh!

jmdavis commented Mar 15, 2018

Uh oh!

jmdavis commented Mar 15, 2018

Uh oh!

marler8997 commented Mar 15, 2018

Uh oh!

marler8997 commented Mar 8, 2018 •

edited

Loading

dukc Mar 8, 2018 •

edited

Loading

marler8997 commented Mar 15, 2018 •

edited

Loading

marler8997 commented Mar 15, 2018 •

edited

Loading