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

[Manu]

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!
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! (`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.
5. It's super-unlikely a function returns a primitive type via a
mutating parameter; primitive results would virtually always be
`return`ed, so instances of this pattern are only meaningfully
applicable to structs (see above).
6. Failing all that, I have accepted prior that there may exist
legitimate occurrences, but when you filter out all the cases above,
what is left? It's super rare at very least, and the scenario depends
on 2 things: code is CHANGED to use properties that return by-val, and
mutating function is niche enough that it doesn't fall into any cases
above. I don't know what that case is; at this stage, it's still
hypothetical.
Recommend: use the @disable technique defensively if you suspect the
use case is likely to be used incorrectly.

As I've said prior; this case is quite contrived, and it's clear that
the bug is actually in the property, not the use of 'ref'.
The accidental mutation of an rvalue as suggested can occur in a
variety of ways - only one of which is using ref.

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();

// the user adds a property in place of an existing member, but the
property incorrectly returns by-val
struct S {
  private M _m;
  M member() { return _m; }
}
S s;
s.member.mutate(); // <- oops, mutated an rvalue!

There are countless ways you can construct the same bug. ref doesn't
contact this problem in a general way, so a solution to this class of
problem shouldn't be ref's responsibility.
It may be possible to argue that the existing ref semantics may
provide defense against a very small surface area of this class of
bug, but I'll never find that narrow advantage weighs favourably
against all the other advantages of this DIP.


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


> By the way, this does not mean I am totally against DIP 1016. It
> has a lot of benefits in my opinion. But I also think that
> Jonathan has a very valid point which definitely needs to be
> considered.

Trust me, I've been considering Jonathan's case painstakingly for 10 years!!
I think it's a contrived, and fairly weak argument. If you consider
the problem as a *class of problem*, which it is, I don't understand
how the existing rule can be so zealously defended in the face of a
bunch of advantages, when all other constructions of the exact same
problem are silently allowed, and literally nobody complains about
them ever!