-preview=in might break code

[Iain Buclaw]

On Saturday, 3 October 2020 at 05:02:36 UTC, Andrei Alexandrescu 
wrote:
> On 10/2/20 6:11 PM, Walter Bright wrote:
>> On 10/2/2020 10:31 AM, Steven Schveighoffer wrote:
>>> And this might not be true on a different compiler.
>> 
>> This is looking like a serious problem.
>
> They say one should choose one's fights wisely, so I spent some 
> time
> pondering. I could just let this thread scroll by and not think 
> twice
> about it. By next week, I may as well forget.
>
> But then I realized this is /exactly/ the kind of crap that 
> we'll all
> scratch our heads six months from now, "How did this ever pass 
> review?

I take offense to that.  I'd prefer if you'd moderate your tone 
please.

> Who approved this? How in the world did a group of competent,
> well-intended people, looked at this and said - yep, good idea. 
> Let's."
>
> ???
>

*You* approved it.

https://github.com/dlang/dmd/pull/11000#issuecomment-675605193


> This glib take is EXTREMELY concerning:
>
>> In the general case, no. You can have two distinct pointers 
>> with the
>> same value, and there's nothing the frontend can do to detect 
>> it.
>> 
>> This scenario has been brought up during the review. I doubt 
>> it will,
>> in practice, be an issue though. This is not a common pattern, 
>> nor
>> does it seems useful. It rather looks like a code smell.
>
> Wait a SECOND! Are we really in the market of developing and 
> deploying language features that come unglued at the slightest 
> and subtlest misuse? We most certainly shouldn't.
>
> I sincerely congratulated Mathias and the other participants 
> for working on this. It's an important topic. Knowing that all 
> those involved are very good at what they do, and without 
> having looked closely, I was sure they got something really 
> nice going that avoids this absolutely blatant semantic 
> grenade. And now I see this is exactly it - we got a -preview 
> of a grenade. How is this possible? How can we sleep at night 
> now?
>
> Again: take a step back and reconsider, why did this pass 
> muster?
>
> This is important, folks. It's really important as parameter 
> passing goes to the core of what the virtual machine does. You 
> can't say, meh, let's just fudge it here, and whatever is 
> surprising it's on the user.
>
> Please, we really need to put back the toothpaste in the tube 
> here. I could on everybody's clear head here to reconsider this.

Frankly, I think you are making a mountain out of a molehill 
here.  You are imagining a problem that doesn't exist; and if one 
does find an issue, the fault lies with the DMD compiler and not 
the D language specification.  Though evidently having clearer 
wording in the spec benefits all.

If you read nothing more of this reply, at least finish up until 
the end of this paragraph.  Please hold fire until GDC and LDC 
have implemented this feature, then we can discuss the pitfalls 
that we've encountered with it.  Basing decisions on behaviors 
observed with DMD is not the right approach, and if you are 
currently finding the situation to be a mess, it is a mess of 
DMD's own doing.

Plucking a fitting example from a Phobos unittest that 
demonstrates the kind of things DMD let's people get away with:
```
RefCounted!int* p;
{
     auto rc1 = RefCounted!int(5);
     p = &rc1;
     assert(rc1 == 5);
     assert(rc1._refCounted._store._count == 1);
     auto rc2 = rc1;
     assert(rc1._refCounted._store._count == 2);
}
assert(p._refCounted._store == null);
```
Why is this not a compile-time error?  DMD just isn't punishing 
users enough for the buggy code they've written.

- - -

I don't really have the heart to go through and unpick all points 
raised in this thread, but I think it's worth sharing the three 
key conclusions I took away from reviewing the pull request:

1. This is behind a -preview flag, and so should be treated as 
experimental.  Nothing breaks by having it there.  Nothing breaks 
if it were to be suddenly removed without any deprecation cycle.

2. Aliasing was raised multiple times throughout the review, I 
even gave this example at time to demonstrate my concerns:
```
void bar(in int a, out int b) { }
int a = 42;
bar(a, a);
```
But ultimately, worrying about this is missing the point, as the 
problem is already present in the compiler even without 
`-preview=in`, and Walter is working on fixing it.  In the 
meantime, these sorts of cases are relatively trivial to pick up 
and can be added as warnings in GDC and LDC until the front-end 
implements the semantic guarantees.

3. There is no danger of ref/non-ref mismatches in ABI, because 
`in` parameters that are inferred `ref` are going to be passed in 
memory anyway.

In the following:
```
alias A = void delegate(in long);
alias B = void delegate(const long);
```
Either `long` always gets passed in memory, or always gets passed 
in registers, in both cases, they are always going to be 
covariant.  The same remains true whatever type you replace 
`long` with.

The one place where `in` parameters start to get interesting is 
rather at the call site.  I think it is best illustrated with the 
following:
```
struct Foo { this(this); }
void fun1(const Foo f) { }
void fun2(in Foo f) { }
```
The compiler already ensures that non-trivially copy-able types 
never end up in a situation where a temporary is needed.  So 
`Foo` is always passed by ref, otherwise there'd be a double copy 
done at the call site.

So once again, both are covariant as far as the callee is 
concerned.  But in the case of `fun2`, the caller does something 
different.  The copy constructor is elided entirely, and that is 
the crux of the optimization that is at play here.  If you've 
assumed anything else, you're assumptions are sorely misplaced.

- - -

I've also skimmed past a passing concern that the ABI between 
compilers would be different.  Well, let me rest assure you that 
DMD, GDC and LDC have never been compatible in the first place, 
so there's no point worrying about that now.  Though DMD is 
really the one at fault for all incompatibilities by choosing to 
have a non-standard calling convention for extern(D) code...