On 24.10.19 13:45, Dukc wrote:
> On Thursday, 24 October 2019 at 08:02:28 UTC, RazvanN wrote:
>> Hello everyone,
>>
>> [snip]
> 
> The root cause here, I think, is that `Foo.opAddAssign` takes the implicit member parameter as `auto ref`

That's not what `auto ref` means! The `this` parameter is a `ref` parameter within the body of the called member function, so it is not `auto ref`.

> when in this case it would be more appropriate to take it as `ref`.
> ...

It does take the parameter by `ref`.

> Unfortunately, you can't mark it like that - `void opAddAssign(int z) ref { _this.y += z; }` fails to compile. Perhaps it should?
> 
> 

`ref` on a function applies to the return value.

On Friday, 25 October 2019 at 01:20:02 UTC, kinke wrote:
> On Thursday, 24 October 2019 at 08:02:28 UTC, RazvanN wrote:
>> What are your thoughts on this? Should be close the issue as an invalid one?
>
> I'm clearly in favor of the filed issue, i.e., see it as valid. This is for consistency with primitive types, and lvalue-ness is IMO a clear intrinsic requirement for a (bin)assign operator's left hand side:
>
> ```
> struct S { int i; }
>
> struct O
> {
>     int i;
>     void opOpAssign(string op)(int) {}
> }
>
> T make(T)() { return cast(T) 1; }
>
> void foo()
> {
>     make!int() += 2; // Error: `make()` is not an lvalue and cannot be modified
>     make!S() += 2;   // Error: `make()` is not an lvalue and cannot be modified
>     make!O() += 2;   // compiles
> }
> ```
>
> Allowing rvalues for an explicit opOpAssign!"+" call is IMO fine, but not when using the operator, as that's just a leaking implementation detail.

The core problem with this is that you can't distinguish rvalue and lvalues for methods. It's a similar problem to this: https://issues.dlang.org/show_bug.cgi?id=19507

And you can still make calls `make!S().i += 2` or `make!S().modifyingFoo()`. There's no point putting a band aid on the problem for this one particular case.


>     make!S() += 2;   // Error: `make()` is not an lvalue and cannot be modified

That error message is deceptive. You can't use "+=" with S either way. This would be a more correct error message:

    S s;
    s += 2; // Error: s is not a scalar, it is a S

Not the best error message either for the problem. Would make more sense if it told the user the "+=" isn't overridden for S.

On Friday, 25 October 2019 at 17:49:15 UTC, Exil wrote:
> On Friday, 25 October 2019 at 01:20:02 UTC, kinke wrote:
>>     make!S() += 2;   // Error: `make()` is not an lvalue and cannot be modified
>
> That error message is deceptive.

Yes, of course; I mentioned it to illustrate that the lvalue check apparently precedes that check, while the opOpAssign lowering seems to precede the lvalue check, and that's what I see as leaking implementation detail.

On Friday, 25 October 2019 at 11:44:46 UTC, Timon Gehr wrote:
> On 24.10.19 13:45, Dukc wrote:
>> On Thursday, 24 October 2019 at 08:02:28 UTC, RazvanN wrote:
>>> Hello everyone,
>>>
>>> [snip]
>> 
>> The root cause here, I think, is that `Foo.opAddAssign` takes the implicit member parameter as `auto ref`
>
> That's not what `auto ref` means! The `this` parameter is a `ref` parameter within the body of the called member function, so it is not `auto ref`.

Yeah, I wasn't precise. I meant that optimally, `Foo.opAddAssign` should not accept a rvalue member argument, just like `ref` parameters normally don't accept rvalue arguments.

As Manu has often said, it's oftentimes problematic when `ref` parameters don't accept rvalues. Here we have the opposite problem.

On Thursday, 24 October 2019 at 08:02:28 UTC, RazvanN wrote:
> Hello everyone,
>
> I am currently trying to solve old issues but some of them I'm not entirely sure how to handle. One of them is 1974 [1]. Here's the thing:
>
> struct Foo { int y; void opAddAssign(int z) { y += z; } }
> struct Bar { Foo fou; Foo test() { return fou; } }
> void main() {
>   Bar x;
>   x.fou.y = 0;
>   x.test() += 1337;
>   writefln(x.fou.y);
> }
>
> The problem is that  x.test() += 1337; calls opAddAssign on a temporary and the person that filed the bug report claimed that this should be an error since the call is useless (in this case).
>
> I have responded to the issue with the following:
>
> "This is a bit problematic because there are 2 points of view on this:
>
> 1. From a high-level point of view, the bug report is valid, you are trying to assign to an rvalue (x.test() = x.test() + 1337) which should an error.
>
> 2. From a lowering point of view you are basically calling the member function of a temporary object (x.test.opAddAssign(1337)), which by the current rules is valid behavior.
>
> I guess that what could be implemented is something along the lines of: if the opAssign/op*Assign function is pure, then you can error/warn because the call has no effect, otherwise it is possible that the assign function has side effects so calling it is correct."
>
> What are your thoughts on this? Should be close the issue as an invalid one?
>
> Cheers,
> RazvanN
>
> [1] https://issues.dlang.org/show_bug.cgi?id=1974

I think the lowering is a distraction.

+= on an rvalue is invalid, so the lowering should never happen.

The implementation of operator overloads may be done with regular functions, but the "interface" of += doesn't make sense on an rvalue. If you want to call your opOpAssign on an rvalue, that's fine, but that's not the same as +=

If you want to stick with thinking/implementing by lowering, you could do something like this (for structs at least):

auto ref __opOpAssignImpl(string op, S, T)(ref S s, auto ref T s)
{
    return s.opOpAssign!op(forward!s);
}

and then a+=b is lowered to a.__opOpAssignImpl(b) and if a is an rvalue you will get your error.

On Saturday, 26 October 2019 at 14:41:38 UTC, John Colvin wrote:
> I think the lowering is a distraction.
>
> += on an rvalue is invalid, so the lowering should never happen.
>
> The implementation of operator overloads may be done with regular functions, but the "interface" of += doesn't make sense on an rvalue. If you want to call your opOpAssign on an rvalue, that's fine, but that's not the same as +=

Exactly what I'm thinking. Implementation-wise, it's probably not that hard to perform the lhs-lvalue check before lowering.

C++ btw allows rvalues: https://godbolt.org/z/qZ3qrj

On Saturday, 26 October 2019 at 14:41:38 UTC, John Colvin wrote:
>
> If you want to stick with thinking/implementing by lowering, you could do something like this (for structs at least):
>
> auto ref __opOpAssignImpl(string op, S, T)(ref S s, auto ref T s)
> {
>     return s.opOpAssign!op(forward!s);
> }
>
> and then a+=b is lowered to a.__opOpAssignImpl(b) and if a is an rvalue you will get your error.

Rather obvious typo correction:

auto ref __opOpAssignImpl(string op, S, T)(ref S s, auto ref T v)
{
    return s.opOpAssign!op(forward!v);
}

On Saturday, 26 October 2019 at 15:30:13 UTC, kinke wrote:
> On Saturday, 26 October 2019 at 14:41:38 UTC, John Colvin wrote:
>> I think the lowering is a distraction.
>>
>> += on an rvalue is invalid, so the lowering should never happen.
>>
>> The implementation of operator overloads may be done with regular functions, but the "interface" of += doesn't make sense on an rvalue. If you want to call your opOpAssign on an rvalue, that's fine, but that's not the same as +=
>
> Exactly what I'm thinking. Implementation-wise, it's probably not that hard to perform the lhs-lvalue check before lowering.
>
> C++ btw allows rvalues: https://godbolt.org/z/qZ3qrj

Moreover, C++ can overload on the "rvalueness" of this, thereby allowing the user to disallow calling operator+= on rvalues:

---------
struct S {
    int i = 0;
    // notice the `&` between `(int)` and the opening curly
    // this means that `this` can only be bound to lvalues
    S& operator+=(int) & { return *this; }
};


int main() {
    S s{};
    s += 42;   // fine, lvalue
    S() += 3;  // won't compile
}
---------