Fix Issue 19754 - Re-apply #9505 and fix it, making isLvalue() logic more restrictive wrt. literals

[kinke]

These issues have surfaced with #9505 (mainly with non-DMD backends).

[dlang-bot]

Thanks for your pull request and interest in making D better, @kinke! 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
✗	19754	normal	cast() sometimes yields lvalue, sometimes yields rvalue
✓	20608	regression	[REG2.087] Cannot pass tuple.expand to auto ref T... template argument pack

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

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 + dmd#10124"

[ghost]

No default parameters (rule 12)

[kinke]

There are 14 other call sites in this file I'd rather not touch.

[ghost]

Let's say it's okay since it's not a new func.

[kinke]

https://github.com/dlang/dmd/blob/master/test/fail_compilation/test6883.d doesn't fail anymore (compile-time bounds checks).

[kinke]

In the 3rd commit, I tried to tentatively optimize the IndexExp even for keepLvalue = true and to 'undo' it in case it yields no lvalue (no idea if it ever would) - because that's where the compile-time errors for trivial out-of-bounds cases are generated (as part of calling Index() from constfold.d). The expected errors are back, but now twice, as each assignment is optimize()d twice - once as part of AssignExp-sema (where the 1st error is generated, but now not left at the optimized constant lhs, but 'undone' to the partially optimized IndexExp), and then again as ExpStatement-sema. Apparent hacks/architectural flaws (need to try to const-fold (=> rvalue) an assignment's lhs for compile-time out-of-bounds errors in that lhs) seem to be surfacing...

[kinke]

Just stumbling from one problem to the next, I'll probably give up on this. I'd suggest reverting #9505 for the time being, it has opened a can of worms and doesn't work as intended. Pinging @ibuclaw.

[kinke]

After much trial and error, there seems to be light at the end of the tunnel.

The Phobos unittest compile errors are due to some refRange(&["foobar"][0])... instead of string s = "foobar"; refRange(&s)..., as ["foobar"][0] is now an rvalue (as is now enum s = ["foobar"], s[0]). To me, it seems like a questionable shortcut to save a line; the original testcase in the associated issue report uses the explicit 2nd version.

[ghost]

The problem with structLiteral is not wether they are Lvalue or not it's rather that when they are they are not modifiable Lvalues.

[ghost]

The messages in my PR are better. Yours don't mention anything about constness and are rather confusing

[kinke]

I totally disagree.

[ghost]

The test cases are in now. Free to you to rework but I still think that it's sub-accurate to just output that the func cant be called. The user would have to think more about the why.

[kinke]

Rebased once more...

[ibuclaw]

Let's go, as ICE's would also be present in gdc-master due to bad AST sent from the front-end.

[Geod24]

This broke our code. Reduced test case:

public T deserializeFull (T) () @safe
{
    static if (is(T : E*, E))
    {
        return &[ deserializeFull!(typeof(T.init[0]))() ][0];
    }
    return T.init;
}

struct Block { string value; }

void main ()
{
    auto b = deserializeFull!(immutable(Block)*)();
}

Here, the literal is a GC allocation, so why is the compiler refusing it ? Moreover, I'm asking for an immutable value (originally was asking for const, doesn't make a difference), so the error message is at least misleading.

[ibuclaw]

This broke our code. Reduced test case:

Thanks, can we have a regression issue?

[Geod24]

Sure. Just wanted to confirm if that was intended. Anyway, opened https://issues.dlang.org/show_bug.cgi?id=21312

[ibuclaw]

Sure. Just wanted to confirm if that was intended. Anyway, opened https://issues.dlang.org/show_bug.cgi?id=21312

It may be intended, but I think is better to digress in a bug report.

[kinke]

This is intended. People seem to have abused this [<rvalue>][0] to turn an rvalue into an lvalue, surely many times without realizing that this entails creating a GC array, e.g., #10124 (comment).

[kinke]

It might be nice to have a little moveToGC helper in core.memory or so, for something like this without array overhead:

extern (C) void* _d_newitemU(in TypeInfo _ti);

T* moveToGC(T)(auto ref T value)
{
    import core.lifetime;
    auto mem = cast(T*) _d_newitemU(typeid(T)); // allocate without initializing
    moveEmplace(value, *mem);
    return mem;
}

public T deserializeFull (T) () @safe
{
    static if (is(T : immutable E*, E))
        return () @trusted { return cast(T) deserializeFull!E.moveToGC(); }();
    else
        return T.init;
}

[Geod24]

Well I'd be happy to use [ ... ].ptr, unfortunately someone decided it wasn't @safe.

[kinke]

I guess that would be fixable for array literals with at least one element, but I'd still consider [<elem>].ptr obscure and an explicit moveToGC() much better, even without considering the array overhead.

[Geod24]

but I'd still consider [].ptr obscure and an explicit moveToGC() much better, even without considering the array overhead.

"I consider this syntax obscure" is not reason enough to break the language. If it was, I would have nuked with statements from orbit :)
The code in question is in a serializer. Using &[ ... ][0] worked as intended, and now it does not, for what seems to be an arbitrary limitation. Worse it comes without a deprecation. Where is the benefit to the user ?

[kinke]

That comment was wrt. your code example, not in general.

The benefit IMO is consistency. Indexing or dot-var'ing a literal now consistently yields an rvalue, array literals being no exception. My thinking was that a part of a literal is still a literal. E.g., this used to compile too (without @safe) and was the reason for the Ocean issue IIRC:

struct S { int x; }
void main() { auto p = &S(0).x; *p = 123; }

IIRC (it's been a while), NOT doing this would lead to compile errors with master not being caught by the compiler anymore after cast-related changes/fixes in earlier commits.

[Geod24]

The benefit IMO is consistency. Indexing or dot-var'ing a literal now consistently yields an rvalue, array literals being no exception.

I would rather have the compiler be consistent on the whole expression than just two levels of it.
The full expression includes a & which should not do this optimization unless there's also a dereference in the same expression, or the compiler has another mean to prove the address doesn't escape.

My thinking was that a part of a literal is still a literal.

Right, but [] has always returned an lvalue. If we can optimize it to sometimes return rvalues it's great, but the current state seems problematic.
For example, what about:

const foo = [ 42, 24 ];

If we want to be consistent, shouldn't this give us a const int[2] ?

And yeah, the code you liked is obviously wrong, and I'm glad you put so much effort into fixing this. I just think we can do this fix while still retaining the old behavior when someone takes the address of an array literal.

[kinke]

I gave this some more thought yesterday. I do see that a (dynamic) array literal is a special case, and while the slice itself is an rvalue, its elements clearly are not. On the other hand, emplacing a single element in a new GC-allocation is the only use case I can think of for immediately indexing an array literal, and that comes with associated array overhead (it will never be appended to, as there's no reference to the array literal anymore when immediately indexing it). So I think moveToGC() is better for this task, but comes with a limitation, as moving means the source value must be mutable.

I can open a new PR treating array literal elements as lvalues again, to check the effect on the compile errors.

[JohanEngelen]

This introduced regression https://issues.dlang.org/show_bug.cgi?id=23273