Add get(default) same-type overload in Nullable

[FeepingCreature]

Works around (I suspect) https://issues.dlang.org/show_bug.cgi?id=20670 , which may
cause Nullable with arrays of immutable structs to incorrectly strip
away the immutable, creating impossible types that can't be reused.

@RazvanN7 Since you added get(default), is there any reason why it would have to infer a different type than the Nullable's?

[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 coverage diff by visiting the details link of the codecov check)
• 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 run digger -- build "master + phobos#7437"

[thewilsonator]

Implicit conversions?

[FeepingCreature]

Should be done on the caller side by explicitly specifying the parameter type...

[FeepingCreature]

ping (@RazvanN7 )

[Geod24]

LGTM. Putting the 72h label to give @RazvanN7 time to answer.

[RazvanN7]

My code was originally used the same type as the Nullable's, however @schveiguy asked for this change: "Can you change the T to U as it masks the overarching template type from the Nullable struct?"

But I guess this was only as a precautionary measure for the case where T would be used further in the function, so if this causes trouble, I have nothing against this PR, but maybe @schveiguy has a different perspective.

[schveiguy]

Wouldn’t this be a breaking change? I.e. Nullable!ubyte.get(1000) would stop working?

I don’t remember the original discussion. On my phone right now I will look tomorrow.

[FeepingCreature]

Yeah, I think you're right. Damn. When the default value is a more generic type than the stored value, this would break.

The way to go would be a fix for 20670. But that seems hard, since the issue touches compiler data structure details afaics. (I can't tell if it's even possible to differentiate immutable struct Struct and immutable(Struct)).

I don't know if there's anyone out there who even uses get(default) that way; it hasn't been in the language for long. It may be okay to break this in the name of fixing constness, but if a fix for 20670 was possible that would be preferable.

@Geod24 please remove the merge tag pending discussion?

[Geod24]

@FeepingCreature : Sure
Original discussion here: #5695 (review)

Wouldn’t this be a breaking change? I.e. Nullable!ubyte.get(1000) would stop working?

True, but that also means it is currently kinda broken.
The problem you mentioned in the link I posted above was that, by using a ternary, you'll get the more generic type. Since number literals are int, currently the following code errors out:

import std.typecons;

void main ()
{
    Nullable!ubyte nl = 42;
    ubyte v1 = nl.get();
    ubyte v2 = nl.get(0); // Error: cannot implicitly convert expression nl.get(0) of type int to ubyte
    ubyte v3 = nl.get(1000); // Error: cannot implicitly convert expression nl.get(1000) of type int to ubyte
}

The error on v3 is understandable, but v2 ?
You'll have the same problem if you pass two completely different class types, you'll get an Object out of it. I think get returning a more generic type than what it stores is a bigger issue.

[schveiguy]

So my review of @RazvanN7's PR that includes this statement:

Can you change the T to U as it masks the overarching template type from the Nullable struct?

Was not the same as what is being proposed here. In the original, the code was:

T get(T)(T datum) {...}

And my point was simply that the T in that template is going to override the T in Nullable(T) type, and look confusing in documentation.

In other words, I just wanted a name change, not functionality change.

The error on v3 is understandable, but v2 ?

Can't we fix this with an overload?

i.e.:

T get(T defaultVal) { ... } // should be preferred when literal can fit
auto get(U)(U defaultVal) {...}

Edit: BTW, I tested this on run.dlang.io, and it works for your example (changing v3 to int of course).

The point of get(default) is to replace someVeryLongSymbolOrExpression.isNull ? defaultVal : someVeryLongSymbolOrExpression.get with someVeryLongSymbolOrExpression.get(defaultVal). Preventing defaultVal from being compatible with the desired ternary expression seems like a step backwards.

[schveiguy]

Note @FeepingCreature the proposed overload solution fixes your issue as well. I think that's the best solution (for now, of course, fixing 20670 would be good too).

[FeepingCreature]

Looks good, I'll change it tomorrow.

[Geod24]

LGTM, just a few nits

[schveiguy]

Looks good.

[schveiguy]

This didn't close any issue, so it won't show up in the changelog. Should an issue be made for this, or should a changelog entry be added?

[FeepingCreature]

There isn't really a separate issue, and the root issue is hard to fix. I think this feature is new enough that most people haven't noticed it exists yet, and immutable structs are sadly kind of rare, so it'll probably be fine - people will notice the change just by virtue of trying to use .get and it working.