dip1000 and preview in combine to cause extra safety errors

[Mathias LANG]

On Wednesday, 8 June 2022 at 19:02:53 UTC, Steven Schveighoffer 
wrote:
>
> But for some reason, this specific code doesn't fail with 
> `-preview=dip1000` or `-preview=in`, but only when both are 
> specified.
>
> Apparently `in` under preview in doesn't really mean the same 
> thing as `scope const`. So does it have nothing to do with 
> preview in? simple experimentation says it does.

Well the real bug in this code lies in `scope` + array being 
allocated on stack. Cherry picking the instance where it happens 
with `-preview=in` is just pointing the finger at a subset of the 
problem, which is probably where you first encountered the issue 
? You unfortunately are at the weird intersection of a few design 
decisions made by a few different people.


Story time:

When DIP1000 was first introduced, there was a lot of discussion 
to make sure we have it behind a switch. So `-dip1000` was born. 
Later on, we introduced the `-preview` / `-revert` / 
`-transition` trifecta as more work was being done on potentially 
breaking change features.


Now the decision that Walter made, and that I, honestly, still do 
not understand to this day, was to make `in` means simply 
`const`. The rationale was that it would "break too much code", 
and if I'm not mistaken, that people expected `scope` to do 
nothing (exception for delegates / class alloc), but changing the 
semantic of `in` would have too much of an impact. The DIP1000 
changes being behind a preview switch, IMO, it shouldn't have 
been a problem.

Later on, there was a push to get rid of most, if not all, usages 
of `in`, especially in druntime C bindings were they were favored.
In parallel, Atila submitted a PR for a new `-preview` switch 
which would make `in` have its actual meaning (dear old 
`-preview=inMeansConstRef`).
I once again expressed my (unfavorable) opinion on the topic 
(https://github.com/dlang/dmd/pull/10769#issuecomment-583229694).

Now, that's where I come in. I implemented `-preview=in` as you 
see it today.
But there was ONE constraint I had to keep in, when I did so: 
`-preview=in` should also make `in` act as `scope`.

That's how you end up with: 
https://github.com/dlang/dmd/blob/7e1b115a0c62c04b74fecfed8a220d5e31cf4fe0/src/dmd/mtype.d#L4397-L4399

Does it make sense ? I don't think so. If it was up to me, `in` 
would always means `scope`.
Should we change it now ? If we couldn't do it when `scope` had 
*no* effect unless `-dip1000` was used, I don't see how we could 
now that `scope` does have an effect even without `-dip1000`.


> Whether it's right or wrong, it's a change that silently 
> introduces memory corruption. It ought to produce a warning for 
> such code. I'm not blaming anybody here for behavior that is 
> probably correct per the spec. But is there no mechanism to be 
> had for warning about this? D already doesn't allow returning a 
> pointer to stack data, even in `@system` code. Doesn't this 
> also qualify?

I have argued with Walter for a long time that having `scope` 
enforced only in `@safe` code was a grave mistake, and would be 
extremely confusing. It would have avoided this situation.