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