[Issue 18985] bad error message for += operation on shared Object

[d-bugmail at puremagic.com]

https://issues.dlang.org/show_bug.cgi?id=18985

ag0aep6g <ag0aep6g at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
                 CC|                            |ag0aep6g at gmail.com
            Summary|shared variable of          |bad error message for +=
                   |user-defined type with      |operation on shared Object
                   |opOpAssign                  |
           Severity|major                       |normal

--- Comment #1 from ag0aep6g <ag0aep6g at gmail.com> ---
(In reply to OlegZ from comment #0)
> class Some {
> 	static immutable Some One = new immutable Some();
> 	auto opOpAssign( op )( Some val ) { writefln( "Some.op" ~ op ); }
> }
> auto var = new shared Some();
> var += Some.One;
> // and we'v got here: Error: read-modify-write operations are not allowed
> for `shared` variables. Use `core.atomic.atomicOp!"+="(var, One)` instead.

There a couple errors in your opOpAssign. If you implement it correctly, you
don't get an error:

----
class Some
{
    static immutable Some One = new immutable Some();
    auto opOpAssign( string op )( const Some val ) shared
    {
        import std.stdio: writefln;
        writefln( "Some.op" ~ op );
    }
}

void main()
{
    auto var = new shared Some();
    var += Some.One; /* no error */
}
----

But the "read-modify-write" error you got is wrong/confusing. It should be
fixed.

A simplified test for the bad error message:

----
Object foo;
shared Object bar;

void main()
{
    foo += 1; /* Error: foo += 1 is not a scalar, it is a object.Object */
    bar += 1; /* Error: read-modify-write operations are not allowed for shared
variables. Use core.atomic.atomicOp!"+="(bar, 1) instead. */
}
----

The first error message is a bit weird (shouldn't it say "foo is not a
scalar"?), but one can figure out the meaning.

The second error message is bad. `shared` is a red herring. Would be better if
the first message would be displayed here, too.


> 1) this error should be a warning only, cause not a beginners usually know
> what they do. and compiler should allow  to do what they want.

By that logic these should also be warnings instead of errors:

1) assignment as a condition: int x; if (x = 1) {}
2) narrowing conversions: long x; int y; y = x;
3) all the @safe checks: void main() @safe { int* p = void; }

You see that's not how D rolls. But you can usually tell the compiler to "just
do it" by using the unsafe tools `cast` and `@trusted`. In the case of
`shared`, you can always cast it away and do whatever you want then.


> 2) this error is stupid cause we can't use atomicOp for user defined type
> (in most cases)

Yup. The error message is bad.


> 3) "shared" keyword is stupid thing in D. 

Bugzilla is not particularly suited for rants about how D sucks. The General
section of the forum is the right place for that.

https://forum.dlang.org/group/general


> 4) synchronized method(became shared) differ from methods with 
> auto meth(...) { synchronized( this ) { ... } }
> meaning is same but behavior is not same - WTF? 

This isn't directly related to the opOpAssign thing anymore, is it? If there's
something wrong with `synchronized`, please file a separate issue. Or maybe
make a thread on the forum first to find out if it's worth filing. Might just
be a design decision that you don't like.


I'm adding the "diagnostic" keyword here, because the error message you got is
confusing and could be improved.
I'm changing the title reflect that the error message is the problem.
I'm reducing the severity to "normal", because the error message is all that
needs fixing here. The code works if opOpAssign is written correctly.

--