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

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

"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."

Thats what I would expect it to do. And it improves on the current behaviour. If need be we can still restrict it more in the future (maybe enforce that rvalues have to be consumed?).

On 10/24/2019 1:02 AM, RazvanN wrote:
> 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."

Warnings are problematic - is the code correct or not? We should avoid them and just decide if it is an error or not.

It's not so simple that if it's pure, it should be an error. For example, pure functions can throw exceptions. Then you're into if it's pure and nothrow, then it should be an error. But then the rule might just be too clever, and just make the language harder to learn.

Language rules are better when they are orthogonal, and giving errors on vacuous code make it not orthogonal causes other problems. See the discussion on what to do about unreachable code errors with:

    int test() {
       static if (1) return 3;
       return 1;
    }

I'd suggest leaving the behavior as is and mark the issue invalid.

On Thursday, 24 October 2019 at 08:02:28 UTC, RazvanN wrote:
> [snip]

D1 operators are deprecated, so at the very least the example should be updated to reflect that (run.dlang.org did not give me a message about them being deprecated when I ran it though...maybe older version of DMD?). Also, I was getting an error with writefln because there is no format string, so I replaced that with writeln.

With those changes (below), the writeln prints 0, rather than 1337. So if it is adding to the temporary, then it seems to then throw it away. Is that what was expected to happen?

import std.stdio;
struct Foo
{
    int y;
    void opOpAssign(string op)(int z)
        if (op == "+")
    {
        y += z;
    }
}
struct Bar {
    Foo fou;
    Foo test()
    {
        return fou;
    }
}
void main() {
    Bar x;
    x.fou.y = 0;
    x.test() += 1337;
    writeln(x.fou.y); //prints 0
}

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` when in this case it would be more appropriate to take it as `ref`.

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

On Thursday, 24 October 2019 at 11:45:02 UTC, Dukc wrote:
> `void opAddAssign(int z) ref { _this.y += z; }` fails to compile.

Meant `void opAddAssign(int z) ref { y += z; }`

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

Since it is in fact assigning to a rvalue, I would argue the current behaviour is in fact correct - the same way as calling a pure function without taking its return value...

    int zoo(int x) pure { //pure, so guaranteed no side-effects
        return x + 3;
    }
...
     zoo(6);
... is also 'useless', yet, completely legal code

To make it work, you can simply do...
    ref Foo test()
    {
        return fou;
    }
...which turns it into an lvalue - to which you can assign.

This is completely in line with how rvalues and lvalues behave, so I would vote for closing as invalid issue.

On Thursday, October 24, 2019 4:09:52 AM MDT Walter Bright via Digitalmars-d wrote:
> On 10/24/2019 1:02 AM, RazvanN wrote:
> > 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."
>
> Warnings are problematic - is the code correct or not? We should avoid them and just decide if it is an error or not.
>
> It's not so simple that if it's pure, it should be an error. For example, pure functions can throw exceptions. Then you're into if it's pure and nothrow, then it should be an error. But then the rule might just be too clever, and just make the language harder to learn.
>
> Language rules are better when they are orthogonal, and giving errors on vacuous code make it not orthogonal causes other problems. See the discussion on what to do about unreachable code errors with:
>
>      int test() {
>         static if (1) return 3;
>         return 1;
>      }
>
> I'd suggest leaving the behavior as is and mark the issue invalid.

And even if the function were pure and nothrow, it wouldn't be enough, because both the argument and the object being assigned to could have references to other data that opAssign then mutates. I'm not sure if there are really any valid reasons to something like that in opAssign - especially when it's done on a temporary - but we can't guarantee that nothing like that is happening unless the functions is strongly pure and nothrow (and you can't have opAssign be strongly pure, because then you couldn't actually mutate the object).

I think that the only way that it would make sense to disallow the assigment would be to just flag it prior to the lowering based on the fact that it's a temporary (and I have no clue how easy that would be to do) with the idea that it makes no sense to assign to a temporary, but I'm not sure that it's enough a problem to be worth worrying about. Regardless, I completely agree that we shouldn't be special casing things here based on how a particular opAssign is implemented - especially since it clearly doesn't buy us enough here to be worth the confusion that it would almost certainly cause.

- Jonathan M Davis

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.

On Friday, 25 October 2019 at 01:20:02 UTC, kinke wrote:
> Allowing rvalues for an explicit opOpAssign!"+" call is IMO fine, but not when using the operator, as that's just a leaking implementation detail.

Making explicit opOpAssign!"+" and += work *differently* from each other seems like a really bad idea. Either they should both work for rvalues, or neither should.