DIP 1016--ref T accepts r-values--Community Review Round 1

[Atila Neves]

On Saturday, 21 July 2018 at 08:59:39 UTC, Manu wrote:
> On Sat, 21 Jul 2018 at 00:15, Johannes Loher via Digitalmars-d 
> <digitalmars-d at puremagic.com> wrote:
>>
>> On Saturday, 21 July 2018 at 05:59:37 UTC, Manu wrote:
>> > [...]
>>
>> Let me give a concrete example of one of the situations 
>> Jonathan is describing. Consider the following code:
>>
>>
>> struct Secret
>> {
>> public:
>>      string key;
>> }
>>
>> /* ... */
>>
>> genrateRandomKey(ref string key) {
>>      key = /*  some code to actually generate the key */
>> }
>>
>> Secret secret;
>> generateRandomKey(secret.key);
>>
>>
>> Now somebody else working on the project who sees the 
>> definition of Secret might think "Having public access to 
>> member variables is bad, so lets use property methods instead. 
>> This even allows us to do some contract checks!" and he 
>> changes the definition of Secret to the following:
>>
>>
>> struct Secret
>> {
>> private:
>>      string _key;
>>
>> public:
>>      string key() @property const
>>      {
>>          return this._key;
>>      }
>>
>>      void key(string key) @property
>>      in
>>      {
>>          assert(key.length == 256)
>>      }
>>      do
>>      {
>>          this._key = key;
>>      }
>> }
>>
>>
>> Now with DIP 1016, the use of generateRandomKey silently 
>> "fails", i.e. secret._key will not be changed by it, which in 
>> this case is a big problem as the key is still default 
>> initialized!
>>
>> Of course one might argue that genrateRandomKey should not 
>> take its argument by reference and rather just return the key 
>> instead. But in my experience, there is quite a lot of code 
>> like this out there (e.g. in order to avoid copying, string is 
>> probably not the best example here...).
>>
>> In one of your earlier answers you argued, that in cases like 
>> this, the @property function should probably have returned by 
>> reference and that not doing so is the real bug. Do you also 
>> think this is true in this case? I don't think so. One reason 
>> is for example, that you can not put contracts on your setter 
>> @property method then...
>>
>> In my opinion, there is no bug with the definition of the 
>> @property methods here. The bug only arises through 
>> interactions with functions which take its parameters by 
>> reference.
>>
>> Do you think this example in contrived? If yes, why?
>
> So, to be clear; the 'gotcha' moment as proposed is this:
>   1. Function mutates an input received by ref.
>   2. Existing code is structured so the function is called with 
> a
> member of some lvalue.
>   3. Member is _changed_ to be an accessor property for some 
> reason
> *and* the property returns the value by-val.
>   4. Gotcha!
>
> It is definitely pretty contrived.
>
> 1. The function in this scenario is clearly an 'out' parameter, 
> so it
> should use 'out', not ref.
> 2. A function like that would almost certainly return its 
> result, not
> mutate an argument. Using ref this way is a poor choice and 
> subverts
> move semantics.
> 3. Scenario depends on introduction of a getter, but any getter
> property that returns a member by-val is almost certainly a 
> primitive
> type. A getter for a struct member would necessarily return a 
> ref, or
> it would experience large copy semantics every time the get is 
> called!

This is assuming anything that isn't a primitive is a large 
struct that is copied, which, in my experience, is rarely the 
case.

I don't recall ever implementing a getter in D that returns by 
ref. I'd consider that a code smell in pretty much every 
language, allowing mutation from the outside. For context, I 
think that getters are a faint code smell and that setters always 
stink.

All of this to say that I disagree that getters will usually 
return by ref unless the return type is a primitive. That's not 
how I write code and I don't remember encountering this in the 
wild.

> 4. A struct-type getter that returns by-val exhibits this 
> gotcha in a
> variety of ways; you 'get' the member (a by-val copy), then 
> mutate a
> member in any way, (ie, call a method), and you've accidentally
> modified the copy, not the source value!

I have trouble understanding why anyone would expect to mutate 
the original object without first consulting the API to see if 
was returned by ref. I'd never expect that to happen. I don't 
think that's a bug because I'd never expect things to work that 
way. Nearly all of my variables and function parameters are const 
anyway. Maybe my brain is weird. All I know is that I'd never 
encounter this and consider it a bug. To me not mutating my 
object is a feature.

> (`ref` is not the bug)
>   - note, in all other constructions of this same 'bug', 
> existing
> language semantics find it acceptable to silently accept the
> accidental mutation of an expiring rvalue. Nobody is losing 
> sleep at
> night.

That's because T().mutate() is obviously not going to do 
anything. Nobody would expect it to.

> The same 'bug' expressed in a simpler and more likely way:
>
> // a struct that shall be the member
> struct M {
>   int x;
>   void mutate() { ++x; }
> }
>
> // the original (working) code
> struct S {
>   M member;
> }
> S s;
> s.member.mutate();

It'd take roughly 5s between me seeing this in code review and 
typing the words "Law of Demeter violation". To me that's TRWTF.

> ...all that said, we understand that there is value in 
> inhibiting calls with rvalues in some cases. We address this in 
> a very nice way with @disable, which is also nicely symmetrical 
> such that the limitation may by applied to rval or lval's.

I like using @disable this way. It's unclear to me the impact on 
existing code that doesn't already have a @disable since it 
wasn't needed before.

I'm not against the DIP - I think easier interop with C++ is a 
good thing and this would help it. I have to think a bit more 
about the points Jonathan has brought up, because it sounds like 
there's a possibility that bugs might be introduced if the DIP 
goes through, at least as-is. I'm not sure.

Atila