Fix Issue 20552 - Deprecated Nullable.get warning with Appenders

[RazvanN7]

Nullable!T has the following assignment operator: opAssign()(T value). This opAssign can be called for both Nullable!T and T because of the inner alias get this of Nullable. However, when a Nullable is assigned to another Nullable
a bit copy is preferred rather then an alias this conversion. Despite this, hasElaborateAssign specifically calls opAssign thus triggering the alias this. To bypass this, I used the solution proposed by @FeepingCreature and added an opAssign overload that receives a Nullable!T. This does get rid of the deprecation, however, I don't know how to test this because when compiled with -de, the deprecation does not appear (and phobos is compiled that way). Suggestions?

[dlang-bot]

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

Auto-close	Bugzilla	Severity	Description
✓	20552	enhancement	Deprecated Nullable.get warning with Appenders

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 "stable + phobos#7759"

[FeepingCreature]

Isn't it time to remove alias get this yet?

Ah, "after 2.096". Soon... sooon...

impatiently hovers finger over $ git push upstream fix/remove-alias-get-this _

[PetarKirov]

Isn't it time to remove alias get this yet?

Yes, since according to DIP1013:

[..] a comment of the format @@@DEPRECATED_[version]@@@ should be added to the top of the code which is to be removed or disabled, where the version number is the version where the feature will be fully removed from the language or standard library. [..]

//@@@DEPRECATED_2.096@@@

=> alias get this should not be part of dmd 2.096.0, so the time to remove it is now.

impatiently hovers finger over $ git push upstream fix/remove-alias-get-this _

↵ Enter! 😛

[Geod24]

impatiently hovers finger over $ git push upstream fix/remove-alias-get-this _

Please don't push to upstream :D

=> alias get this should not be part of dmd 2.096.0, so the time to remove it is now.

Looking at the release notes, it was deprecated in v2.088.0. So it should go with v2.098.0. However, I have a feeling most (if not all) stakeholders will be happy when this is gone. And non-stakeholders will also be very happy to see an unrelated deprecation message gone.

However, the solution brought by @RazvanN7 is simple, can be put into stable (next point release is nearing), so I think this should be merged into stable regardless.

[Geod24]

Please target stable, and feel free to self-merge.

[RazvanN7]

std/conv.d(5710): Error: static assert: !__traits(compiles, emplace(& s2, 1)) is false

I've seen this failure on other PRs. I think it is unrelated to this change.

[Geod24]

Since it triggers on all Cirrus workflow, I don't think we should disregard it.

EDIT: Could Cirrus be checking out the wrong branch (master instead of stable) ?

[schveiguy]

Just a note, the deprecation message says "removed after 2.096", so I would say we should target 2.097. Pushing to 2.098 is not going to make a difference, I would do 2.097, that's what everyone's been reading (over and over and over), and by now if you haven't fixed your Nullable deprecations, you probably won't.

(Not for this PR of course, just in general for the removal of the alias this)

[UplinkCoder]

This PR caused a regression. in the reduced example code:

import std.typecons;
struct T {
        Nullable!S s;
}
struct S {}
S f(S s)
{
  return s;
}
T f2(const(T) t) {
        T ret;
        t.s.apply!((v) {
          return ret.s = nullable(f(v));
        });
        return ret;
}

[UplinkCoder]

I strongly suspect that your since opAssign overload is a template the opAssign inside the template body does not go through proper overload-resolution and therefore you are trying to call the function created by the template instance.

[UplinkCoder]

@RazvanN7

[FeepingCreature]

Afaics, Nullable.opAssign just returns void, and you can't return void as a value. Not sure how this would ever have worked.

Compare:

T f2(const(T) t) {
        T ret;
        t.s.apply!(delegate Nullable!S(S v) {
            Nullable!S a = nullable(f(v));
            Nullable!S s;
            pragma(msg, typeof(a).stringof);
            pragma(msg, typeof(s = a).stringof);
            void foo() { }
            return foo;
            return s = a;
        });
        return ret;
}

[FeepingCreature]

This PR causes another regression.

For struct types with invariants that contain Nullables, the invariant is now checked, because the fact that Nullable has an opAssign with its own type triggers recursive opAssign generation, which checks the invariants in every containing type.

Now that alias get this is gone, is this still needed?

( Filed https://issues.dlang.org/show_bug.cgi?id=22176 )