Breaking change: Rebindable is always a struct with get.

[FeepingCreature]

Previously, Rebindable!T aliases itself away to T if T is an array. This is bad, because it means we cannot reliably get rid of Rebindable with r.get again, forcing an additional cryptic static if in all code where the callee should never be Rebindable.

Let's just get rid of it.

See #8734 for a PR made awkward by Rebindable!T sometimes aliasing itself to T.

Note: This PR is largely to run Buildkite to see if this breaks anything.

Seems surprisingly green!

Okay, what do I need to do to get this merged?

[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#8735"

[ghost]

what do I need to do to get this merged?

At first glance the ddox failure seems unrelated, you need to bring the attention of the author on that failure however.

[FeepingCreature]

Right, I've seen the ddox thing before on other PRs, I suspect it's some build timeout or memory usage, especially since it doesn't happen locally.

Ping @s-ludwig

edit: I mean, what do I need to do unrelated to the ddox issue? This is a pretty big change, and not amenable to deprecated.

[s-ludwig]

unrelated ddox fail: dlang/ddox#239

[atilaneves]

I didn't understand the explanation for why the current behaviour is bad.

[FeepingCreature]

When you have Rebindable!T foo, due to alias get this, you can always do T bar = foo. However, if you're chain-calling a function that does IFTI, there is no way to say "I want foo but without Rebindable" even if you know T - there is no inverse operation to rebindable that always gets you the original type back. (D does not have an "implicit conversion expression".) foo.get could be that operation, if only Rebindable didn't alias itself away for some types. That's why #8734 for instance has to static if detect whether the Rebindable actually had an effect.

[FeepingCreature]

I guess I should investigate why "alias to T for arrays" was done in the first place.

[atilaneves]

I think investigating would be wise. I don't know either.

[FeepingCreature]

Looks like "that's just how Andrei wrote it back in 2008."

Ping @andralex ?

[andralex]

Honestly I forgot the particulars, but probably that alias has to do with qualified arrays having an obvious way to rebind (dropping the top-level qualifier and passing it to the element type).

Rebindable has had a nice intent but its practicality didn't go too far. If there are more ideas on how to improve this type, I think the nicest thing to do would be to redo Rebindable under a different name and quietly undocumment Rebindable.

[FeepingCreature]

I mean, we do have "struct Rebindable" now! I think it's salvageable. Since pretty much every other type now results in a struct with .get, do you think arrays are worth keeping as an alias? Upside is I don't think pretty much anyone else used Rebindable since it was never fully generic until now. So if we want to fix this, now's the time.

[FeepingCreature]

ping <3

[dkorpel]

@atilaneves

[FeepingCreature]

hi

bump btw :))

(Srsly, if you're not gonna merge, just reject, this silent waiting is just bad project management practice.)

[dkorpel]

@atilaneves

[dkorpel]

This needs a changelog entry

[dkorpel]

this silent waiting is just bad project management practice

I'm sorry, I'll address this stalling of Phobos PRs in the next meeting

[FeepingCreature]

Changelog entry added.

[FeepingCreature]

Wait, why did I misremember that this was mostly about arrays? :stares at the diff in confusion: I mean, the change makes sense but ... I should add classes to the changelog as well.

Okay, updated: Everything that uses Rebindable!X, classes and arrays, gets a struct Rebindable with X get(). I forgot that classes had hacky "alias away Rebindable" magic as well.

This type is messed up, yo.

[FeepingCreature]

Wait. No. How did this used to work for classes? The tests say that Rebindable!C just is a C. But the code only checks for isDynamicArray.

Wait.

Wait, no. The code used to alias away Rebindable if the type was not const, or rather, not qualified const or immutable. Okay, this makes slightly more sense.

Out with it anyways. Being able to always write T.get is simply more useful in generic code. Consistency beats corner-case convenience.

[FeepingCreature]

Update: I have tried to get rid of RebindableCommon entirely (inline it in UnqualRef), but this has run face-first into https://issues.dlang.org/show_bug.cgi?id=18755 . I'm not sure what Rebindable is actually supposed to do in that case. The struct Rebindable tries to use cast() if it is available (to avoid the union cast hack that doesn't work in CTFE), but opCast can be declared to not work on certain constnesses. Should I just turn off the cast() path if an opCast overload is detected, on the basis of "surely it is not intended for us"?

edit: Seems to work. 3 files changed, 127 insertions(+), 147 deletions(-) Should I push it?

edit: Hm, unittests don't compile. If somebody can sanity check me on this? std.datetime.systime contains this function

    pragma(inline, true) @property void _timezone(return scope immutable TimeZone tz) \
        @safe pure nothrow @nogc scope
    {
        _timezoneStorage = tz;
    }

I had to remove the return scope. And... this is nonsense, right? tz outlives _timezone in _timezoneStorage. This is exactly not "return scope", it's practically the definition of an escaping value. But if I do that, a bunch of other code breaks.

[dkorpel]

This is exactly not "return scope", it's practically the definition of an escaping value.

Since the function returns void, return scope in this case means it may be assigned to the first parameter this, so this._timezoneStorage = tz; is okay.

[FeepingCreature]

Ah, so any (sensible) opAssign may take its first parameter return scope?

Excellent, seems that made it work. Since you clicked Approve, I'll make a separate PR.

edit: nevermind, more errors...

edit: Yeah that attempt fails in cryptic ways further in. I'll make a separate PR if I can make it work, it shouldn't hold up this one.

[dkorpel]

Ah, so any (sensible) opAssign may take its first parameter return scope?

Only when it returns void. opAssign usually returns this though.

[FeepingCreature]

hi ping

[FeepingCreature]

Since it's been "an issue" lately (I disagree, but whatever): I'm entirely fine with renaming this to Rebindable2! (Or std.v2.typecons.Rebindable.) I just want a Rebindable with sane behavior in Phobos, I don't care what it's called.

Ie. I want to use it in my own Phobos std.algorithm PRs, I don't care who else uses it.

[dkorpel]

Ie. I want to use it in my own Phobos std.algorithm PRs, I don't care who else uses it.

In that case, you can make a private / package(std) Rebindable2 template, so we can defer the breaking change to Phobos v2 or something.

Although I don't mind the PR as is, since I don't think it breaks much.

@atilaneves

[FeepingCreature]

In case this (against expectations) actually gets merged, let me document my rationale of why it's okay to break behavior here.

I don't think anybody was actually using Rebindable for arrays. The first big usecase for it is generic code, but in generic code it didn't support structs, making it extremely marginal. If you know you have an array, you can make it rebindable by Unqual, and probably did. Rebindable was useful for classes, but that behavior is unaffected by this change.

In order for the Rebindable array case to be useful, you had to be writing generic code that took either arrays or objects, but not structs. Feel free to point out my folly if you actually did, but my naive expectation is that this has literally never happened.

Of course, now that Rebindable does support structs, this rationale becomes less true by the day. That's why I was kind of hoping to get this in before 2.104.

(Note: part of the problem is that it is impossible to deprecate this behavior.)

[FeepingCreature]

Correction: I just saw that this change affects classes as well. Nevermind this, I'm making a Rebindable2.

Be right back.

[andralex]

Given that the name Rebindable was not very inspired to begin with, this may be a good opportunity to find a better name and leave Rebindable on the deprecation path.