[Breaking] Deprecate alias get this in Nullable.

[FeepingCreature]

This PR deprecates alias get this in Nullable.

Nullable effectively creates a "pointer type" out of a regular type, with the addition of a null state that throws an error when trying to access the contained value of an empty Nullable with Nullable.get.

However, this conversion may happen implicitly.

Effect

In the short term, all code that takes advantage of the implicit conversion of Nullable!T to T will print a deprecation warning, or fail with -de. Needless to say this is a breaking change.

Motivation

For better or worse, Nullable has become the de-facto type for making a value optional.

However, it also includes functionality to implicitly call its getter, throwing an error on failure. It is thus to my knowledge the only implicit conversion in the language that can error at runtime.

This is like if int* implicitly converted to int by dereferencing. The typesystem is supposed to protect us from things like this, by reminding us to check values for validity before attempting to dereference, decode or otherwise process them. In essence, Nullable's alias get this independently reinvents the null pointer crash, then allows it to occur implicitly.

Making object references nullable by default has been one of D's decisions that has received the most criticism over the years. Nullable does the same, then adds the further problem of implicit conversion to types that aren't usually associated with null pointer crashes. If null pointers are bad, Nullable recreates them in a worse form.

Any existing code that takes advantage of the implicit conversion can be trivially updated by adding a .get, if intentional, or an .isNull check followed by a .get, if unintentional.

Does this really matter?

We have literally spent days of debugging effort to this bug feature.

Do you really expect this to get merged as-is?

Any previous attempt to spark a debate about this topic has run into broad disinterest. The point of this PR is to "put up code", so to speak; to make the cost of deprecating alias get this as small as clicking a "merge" button. Hopefully, if people really disagree with this change (which I've also not seen!), they'll take this opportunity to speak up, here or in the forum thread.

edit: added a changelog entry

edit: made title less offensively loud

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! 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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7060"

[atilaneves]

It being a breaking change worries me. How about introducing a new optional type that doesn't have this issue and deprecating Nullable?

[FeepingCreature]

Re deprecation, I could try to straight up disable it and see if anything breaks in buildkite; that'd give us a preview of how many projects actually depend on this behavior.

Re making an official std.optional, I didn't believe that was an option that would be acceptable, due to the significant feature overlap with Nullable. I'm not even sure if Nullable should be deprecated in that case, it does have a distinct niche for "types that are not expected to be null when accessed", such as late initialized values.

[Geod24]

Some libraries rely on Nullable. Vibe does, e.g. in its REST interface generator. d2sqlite3 does as well. That's just from the top of my head, as I'm currently using those two libraries.
Deprecating it for the sake of deprecating one alias would introduce much work downstream.

However, I don't see why you label it as a breaking change. It's a deprecation. The whole point of deprecated is to make it possible for library writers to not introduce breaking change.

[FeepingCreature]

Re "Some libraries rely on Nullable": I meant outright disable the alias and see what breaks. I'm skeptical of removing Nullable entirely for the reason you stated.

I've labeled this as "breaking" because a deprecation entails an intent to remove. The actual removal, which would be the actual "breaking change", should only be a formality.

[atilaneves]

Even if nothing breaks on buildkite, I'd still be very wary of changing the behaviour of a standard library type.

[FeepingCreature]

Fair enough. I'll look into writing up a std.optional module on tues.

edit: Though didn't we just have W&A saying, at dconf, with the full backing of the community, that we should be more willing to make breaking changes in the interest of improvement?

[Geod24]

Re "Some libraries rely on Nullable"

My comment was an answer to @atilaneves . Writing a new std.optional would mean all those libraries need to be adapted.

I've labeled this as "breaking" because a deprecation entails an intent to remove.

So all deprecations are breaking changes ? I think we use a different vocabulary here.

Even if nothing breaks on buildkite, I'd still be very wary of changing the behaviour of a standard library type.

Isn't that the point of a deprecation though, that we don't just "change behavior" ?
If you're extra concerned, we can make the deprecation period longer - the deprecation message itself will already bring some benefits to some.

[FeepingCreature]

So all deprecations are breaking changes?

For the purpose of a pull request, yes. The actual removal should be a near-automatic action. Accepting a deprecating PR entails accepting an obligation to remove.

[Geod24]

For the purpose of a pull request, yes. The actual removal should be a near-automatic action. Accepting a deprecating PR entails accepting an obligation to remove.

That doesn't make any sense. A deprecation doesn't cause any other component to fail. The removal does. A good example of a breaking change is a function / type starting to behave differently. This is not the case here.

[FeepingCreature]

To me, the question isn't "what will this commit in itself do" but "what will merging this commit, commit the maintainers to do." It's the PR title, not the commit title. In any case, given that it says "deprecate" in the title, I think it's clear enough.

[wilzbach]

Phobos v2 will come. This vision is there, so why not move the new Optional type in std.v2? The module isn't there yet, but it would be a great opportunity to get started on the v2 story.

[FeepingCreature]

I don't see Optional as a "better Nullable." Rather, I see Nullable as an independent feature that is currently being abused as a "bad Optional." Since they have no namespace conflict, I'm not sure what the point of sticking it in v2 would be.

[FeepingCreature]

@WalterBright Hi, before I start writing up a PR, do you think std.typecons: Optional has a chance of being accepted?

edit: nevermind I just did it #7065

[aliak00]

Some libraries rely on Nullable. Vibe does, e.g. in its REST interface generator. d2sqlite3 does as well. That's just from the top of my head, as I'm currently using those two libraries.
Deprecating it for the sake of deprecating one alias would introduce much work downstream.

The implicit cast is just error prone and should be looked at as a bug in Nullable. Languages should probably fix gaping holes even if they're in standard types and causes down stream work. The code is already broken downstream because they're using nullable as is.

[FeepingCreature]

Though it may be argued that 90% of the places that currently use Nullable "should have been using Optional to begin with". (Notwithstanding the fact that Optional did not exist.) My impression is that Nullable!T is not a type that is meant to be in null state in ordinary operation, due to things like .get being undefined for null Nullables. So I guess it's meant for things like delayed initialization, and all the problems arise because people use it as an Optional, for which it's ill-suited.

[wilzbach]

Did we check how many projects would be affected on Buildkite (i.e. add a temporary test commit to check that).

[aliak00]

Would it be possible to no go with the deprecation route on this one and just remove it. This is really really bad right now. This actually crashes:

struct Foo {
    Nullable!string a;
}

void main() {
    string a;
    auto app = appender(&a);
    formattedWrite(app, "%s", Foo());
}

You cannot, at this time, use any logging functions in vibe-d for example because of this if you have a struct with a null Nullable in it.

[aliak00]

And just to frame this a bit better. At work I've managed to get D in use a bit. I got another part of the project to start using D as well ... and of course they hit this bug, and if you can't log objects in a web framework, well... they switched to rust 😢

[Geod24]

The implicit cast is just error prone and should be looked at as a bug in Nullable. Languages should probably fix gaping holes even if they're in standard types and causes down stream work. The code is already broken downstream because they're using nullable as is.

No disagreement here.

Would it be possible to no go with the deprecation route on this one and just remove it.

No. Some people use it the right way and it's not right (nor a sustainable policy) to break their code.
We have a very easy deprecation path here. Just auto-merge this PR.

[wilzbach]

Needs to a comply with the deprecation DIP

[FeepingCreature]

Should now comply with DIP1013. Removal is set for >2.096.

[FeepingCreature]

@wilzbach see #7073

edit: Looks like dub has some issues. PRing.

[aliak00]

No. Some people use it the right way and it's not right (nor a sustainable policy) to break their code.
We have a very easy deprecation path here. Just auto-merge this PR.

Using it implicitly without a .get is not the right way though (do you think it is?). How is it breaking their code as opposed to fixing it?

Furthermore, consider how serious this bug really is: it literally triggers an assert from within the standard library, after which, your program is invalid - literally by definition. And in release mode this assert will not be hit.

This triggers it: https://github.com/dlang/phobos/blob/master/std/format.d#L3448

[Geod24]

Using it implicitly without a .get is not the right way though (do you think it is?). How is it breaking their code as opposed to fixing it?

It's not about being "the right thing to do".
It's about people having written code that works perfectly fine that would be broken without a proper deprecation.

Take this pull request as an example. The code is correct, as it only uses the implicit conversion after an !isNull check.
Without a proper deprecation, we break a perfectly working piece of code without providing any value for the user.
With a deprecation, we don't break working code, and we inform the user (developer) that they will need to change their code, which is perfectly fine. And since the compiler outputs a message when the implicit conversion is used, you already get the benefit you are looking for (noticing the users using the implicit conversion their code might be broken).

So why would you forgo the deprecation ?

[jmdavis]

@atilaneves

Even if nothing breaks on buildkite, I'd still be very wary of changing the behaviour of a standard library type.

That's the whole point of deprecation. We deprecate a symbol that we think is more harmful to keep around than to leave as-is. The change doesn't break anything. And then after the deprecation period (which is 10 full releases by default, making it roughly two years), we remove the symbol, and then anyone who didn't change their code during the deprecation period ends up with broken code. So, if we're going to make a change like this, deprecation like this PR is using is exactly the right thing to do.

So, the question is whether the current behavior is worse than requiring that any code using Nullable's alias this be updated to call get directly over the next two years.

@aliak00

Would it be possible to no go with the deprecation route on this one and just remove it.

Absolutely not. If a change is not a bug fix, then it must go through the deprecation period, otherwise we're breaking existing code that works perfectly fine. This change is being proposed on the basis that having the alias this is error-prone, not that it's actually a bug - because it isn't. At most, it's bad design. The bug is in the code that uses the alias without ensuring that the Nullable isn't null. You have exactly the same bug if you call get without ensuring that the Nullable isn't null. The alias is only a problem insofar as it arguably makes it too easy to call get accidentally when the Nullable is null.

Also, if/once this PR is merged and released, then any new code which uses Nullable's alias this will get a deprecation warning just like existing code. So, it won't be silent anymore. It just won't result in a compiler error until the deprecation period has ended, and the alias has been removed.

Furthermore, consider how serious this bug really is: it literally triggers an assert from within the standard library, after which, your program is invalid - literally by definition. And in release mode this assert will not be hit.

The assertion in get is effectively an in contract. It's just in the body of the function rather than in an explicit in contract. The assertion in get is doing exactly what it's supposed to be doing. It's catching it when the caller calls get when the Nullable is null - whether it's called implicitly or explicitly doesn't matter. Either way, it's a bug in the caller to call get with a null Nullable. It's the responsibility of the caller to use Nullable correctly, just like it's the caller's responsibility to never call front or popFront on an empty range. So, Nullable itself does not have a bug. At most, it's error-prone, because it's too easy to write code that accidentally calls get because of the alias this. So, that may be a bad design decision, and it may merit deprecating the alias, but it's not a bug in Nullable and does not merit introducing a breaking change without the appropriate deprecation period.

Simply removing the alias this will fix no code and will break quite a lot of code. On the other hand, deprecating the alias this will inform anyone using Nullable that we will eventually be removing the alias and that they will need to update their code before that happens if they don't want their code to break. So, it will allow existing code to be changed to use get explicitly before the alias is removed, and unless a project turns off deprecation messages, any new code that accidentally uses it will see a deprecation message, meaning that the problem will no longer be silent.

[jmdavis]

@aliak00

You cannot, at this time, use any logging functions in vibe-d for example because of this if you have a struct with a null Nullable in it.

Honestly, that sounds like a bug in formattedWrite. If a type has a toString on it, that arguably should be preferred over implicitly converting the type to string. Nullable already has a toString on it which takes into account whether it's null. So, if formattedWrite is barfing like that, then it's likely preferring an implicit conversion when it shouldn't. I would suggest looking into fixing formattedWrite, particularly since that's potentially a problem that extends beyond just Nullable!string.

Either way, a workaround to that problem should be to simply add a toString to your struct that contains the Nullable!string, though I'm inclined to think that we really should look at formattedString and probably change how it works, since its current behavior seems questionable.

[aliak00]

@Geod24 fair enough. The problem is that the code I posted doesn't use the implicit cast. I guess we'll just have to "fix" where/when it pops up in phobos over the next 2 years apparently, if someone notices it and reports it.

@jmdavis pieces of code can have design bugs as well. This one is clear as day. And it will fix any piece of generic code based on is(T : string). Which is > 0. Having generic code specifically check for a Nullable type because it's badly designed is not scalable. And writing a toString for every serializable/deserializable type in a vibe project is also unscalable.

Maybe formattedWrite can be fixed. I couldn't see any clear way to do it at least. But yeah, perhaps statically checking for a toString will fix this particular case. Though it feels very hacky. StringTypeOf also seemed fine.

[FeepingCreature]

@wilzbach https://buildkite.com/dlang/phobos/builds/1697 now shows a more friendly picture. Several high-profile projects would be broken, but it mostly seems to be the same diet-ng error.

[FeepingCreature]

ping?

[FeepingCreature]

ping

Complies with deprecation DIP