Fix issue 21129: Make OnlyResult store a value tuple instead of a static array of CommonType.

[FeepingCreature]

For static arrays, only used to store a reference to its caller. This was bad. I changed it (in #7253 ) to take its arguments by value, but I didn't realize the static array case - so it started storing a reference to its constructor parameters, which was more obviously bad, as it crashed (@system) or failed to compile (@safe).

This PR changes only to store the tuple of arguments in the range and convert to the common type as late as possible.

ping @JohanEngelen

Note that I test with const(char) as there seems to be a bug with CommonType - it thinks char[] and string are best unified as char[].

[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

Auto-close	Bugzilla	Severity	Description
✓	21129	regression	[REG2.090] std.range.only broken for reference conversions of local copies of parameters

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#7584"

[JohanEngelen]

Thanks.
Can you include improvements to the documentation of only? The documentation currently uses words that have little exact meaning in programming ("assemble", "in-situ") and it is not clear that the parameters are copied into the resulting object.

[FeepingCreature]

"Copying the range means copying all elements" seems pretty clear. Afaiu this is how only was always meant to work. Not sure how else I'd phrase it while still talking about behavior, not documenting the implementation.

[schveiguy]

This might cause other problems. Now, Only!(long, int) is a different type than Only!(long, long). Whereas, before it was the same type. Might be a bit contrived.

[FeepingCreature]

Yes but I mean, it's super awkward to fix that. I'd have to sort the tuple and keep a map of indexes to tuple indexes.
Does that unittest describe an actual semantic requirement or just a "nice to have"? It's not in the only docs.

[schveiguy]

I'd have to sort the tuple and keep a map of indexes to tuple indexes.

No, that wouldn't solve it -- the types are different, not just in a different order.

What I see as a possible problem is this:

struct SomeStruct
{
  typeof(only(1L, 1L) someRange;
}

// one place
auto s = SomeStruct(only(1L, 1L));

// some other place
auto s = SomeStruct(only(2L, 3));

Definitely contrived. I don't have an example in the wild. indeed the fact that buildkite passed completely suggests there isn't [m]any.

But of course, buildkite didn't catch the problem that triggered this fix.

I definitely have code that uses the typeof(only(...)) to save an Only struct as a member, but I set it in one place, so it shouldn't be affected.

Then of course, there's the matter of proliferation of templates.

It's also a bit worse performing for opIndex. Hm.. just thought of this too -- if the implicit conversion is expensive or destructive, then it might break code which expects the conversion to only run once. Another contrived example.

It seems like your original PR is fixing a one-off odd case, and this PR is fixing a one-off odd case, only to add more one-off odd cases.

Is it worth considering to go back to the original mechanism, and fix the original one-off a different way? Like maybe using something like checking for each parameter if an implicit cast to Unqual!T is possible, and if so, use that for determining the common type?

[FeepingCreature]

No. The original version did not match the description. The revised version still did not match the description. The implementation was just wrong both times, in ways that contradicted the explicit documentation. If OnlyResult is to take its parameters by value, it has to actually match the parameters' types.

I'd say just go for it. Template proliferation and minor index performance loss should not come at the expense of correctness.

[JohanEngelen]

Perhaps a solution: Can we have the conversion take place at the call site? (i.e. only(Values...) only accepts values that are all of the same type?

Can you also add a testcase for the alias this type of "conversion" ? Thanks!

[FeepingCreature]

Wouldn't that break the static array example if the range is returned?

[JohanEngelen]

Wouldn't that break the static array example if the range is returned?

Ah yes indeed you are completely right. Somehow I am treating only (what a very weird and inexplicable function name it is!) as chain that should only be used to pass as function parameter and not stored.

[FeepingCreature]

ping

[JohanEngelen]

@FeepingCreature Can you also add a testcase for the alias this type of "conversion" ?

[FeepingCreature]

@JohanEngelen done.

[schveiguy]

Technically, I think this should target stable. Is it worth holding up the beta for this? Was going to tag auto-merge.

[FeepingCreature]

Yeah, it's sort of a regression, isn't it? Retargeting to stable.

[schveiguy]

This isn't going to pass buildkite (due to the temporary difference between master and stable for phobos+no-autodecode), but auto-merge will give it priority on the testers.

[MoonlightSentinel]

This looks like an error?

[schveiguy]

0 means frontIndex in this context, see opIndex implementation.

[MoonlightSentinel]

Nevermind

[schveiguy]

This is triggering some kind of problem in phobos for the buildkite build:

std/range/package.d(921): Deprecation: storage class `ref` has no effect in type aliases

Note, this is not what I was referring to above. This part should pass buildkite.

[FeepingCreature]

That's unrelated to this PR. Note that it happens in chain, not only - and the code is marked "This doesn't work yet", lol.

[schveiguy]

I know. What concerns me is that it doesn't happen anywhere else (I haven't seen a massive buildkite failure like that in other PRs). It's a lovely error with no indication of what the compiler was doing when it tried to compile that. Possibly some combination of chain/only in some test or something is causing this, but I don't know why then it would pass all the autotesters, etc.

[MoonlightSentinel]

This diagnostic was enabled recently in DMD and I've already removed that code in master. Seems like Buildkite uses dmd-master to build stable?

[schveiguy]

I wonder if it's possible buildkite got caught right in the middle of the switch to target the stable branch. Try a re-force-push?

[MoonlightSentinel]

Maybe. I've retriggered Buildkite

[schveiguy]

Didn't work. Maybe try a force push? or close and reopen?

[wilzbach]

This diagnostic was enabled recently in DMD and I've already removed that code in master. Seems like Buildkite uses dmd-master to build stable?

Yes, that's a bug in Buildkite. When you change the target branch in GitHub without a new commit or force-push, then Buildkite doesn't update the base branch, i.e. BUILDKITE_PULL_REQUEST_BASE_BRANCH="master". Hence, dmd master gets checked out.

It's a bit hard to workaround this as GitHub's API are rate-limited.
Anyhow, I force-pushed which fixed this problem.

[FeepingCreature]

Well, I'll be back in 12h. Good luck! ;)