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
<[email protected]> 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
