www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Issue 1974 - What's your opinion?

reply RazvanN <razvan.nitu1305 gmail.com> writes:
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
Oct 24 2019
next sibling parent Zoadian <no no.no> writes:
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?).
Oct 24 2019
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
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.
Oct 24 2019
parent Jonathan M Davis <newsgroup.d jmdavisprog.com> writes:
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
Oct 24 2019
prev sibling next sibling parent jmh530 <john.michael.hall gmail.com> writes:
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
}
Oct 24 2019
prev sibling next sibling parent reply Dukc <ajieskola gmail.com> writes:
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?
Oct 24 2019
next sibling parent Dukc <ajieskola gmail.com> writes:
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; }`
Oct 24 2019
prev sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
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.
Oct 25 2019
parent Dukc <ajieskola gmail.com> writes:
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.
Oct 26 2019
prev sibling next sibling parent ShadoLight <ettienne.gilbert gmail.com> writes:
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.
Oct 24 2019
prev sibling next sibling parent reply kinke <noone nowhere.com> writes:
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.
Oct 24 2019
next sibling parent Paul Backus <snarwin gmail.com> writes:
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.
Oct 24 2019
prev sibling parent reply Exil <Exil gmall.com> writes:
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.
Oct 25 2019
parent kinke <noone nowhere.com> writes:
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.
Oct 25 2019
prev sibling parent reply John Colvin <john.loughran.colvin gmail.com> writes:
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.
Oct 26 2019
next sibling parent reply kinke <noone nowhere.com> writes:
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
Oct 26 2019
parent Atila Neves <atila.neves gmail.com> writes:
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
}
---------
Oct 28 2019
prev sibling parent John Colvin <john.loughran.colvin gmail.com> writes:
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);
}
Oct 26 2019