Weird DIP1000 issue

[tsbockman]

On Tuesday, 7 February 2023 at 23:42:38 UTC, 0xEAB wrote:
> I’ve recently run into an issue with DIP1000.

On Tuesday, 7 February 2023 at 23:45:05 UTC, 0xEAB wrote:
> Not sure how to put this…
> Is attribute inference “supposed” to create such issues?

There are no relevant attributes being incorrectly inferred in 
your sample code, as far as I can tell.

Rewriting your code to avoid attribute inference entirely - by 
manually instantiating templates and supplying explicit types in 
place of `auto` - does not fix the problem. (It may be possible 
to fix it with changes to `std.algorithm.filter`, though.)

On Tuesday, 7 February 2023 at 23:42:38 UTC, 0xEAB wrote:
> And two potential fixes:

Neither of those "fixes" would reliably fix your code in more 
realistic contexts; they only work in your reduced test case by 
accident.

The actual problem is a combination of three things:

(1) You allocate `vr` on the stack, meaning it has a finite 
lifetime.

(2) DIP1000 considers `c` to be restricted by `vr`'s lifetime.

(3) You pass `c` as a non-`scope` parameter, which by DIP1000's 
logic means it might escape beyond the lifetime of `vr`'s stack 
frame, and potentially cause a use-after-free error.

Fix at least one of those properly to eliminate the error while 
preserving memory safety. Valid solutions include:

(A) Fix (1) by allocating `vr` with "infinite" lifetime (like 
globals, string literals, and `new` GC allocations):
```D
     auto vr = new VRes!X();
```

(B) Fix (3) by marking `buffers` as `scope`:
```D
     void write(Buffers...)(scope Buffers buffers) {
```

(C) Fixing (2) is the hard one.

Even with DIP1000, D currently lacks a way to directly indicate 
what the relationship is between an aggregate instance's 
lifetime, and the lifetime of the target of one of its member 
indirections: an instance member cannot be `scope`, nor can it be 
the opposite of `scope`.

Instead, the compiler relies on a combination of conservative 
assumptions, each member function's attributes, their parameter's 
attributes, and analysis of their data flow.

In practice, it seems to be possible with `@safe` code to induce 
the compiler to treat a member indirection's target in one of 
three ways:

(1) As always restricted to the lifetime of the aggregate 
instance. (This is what I think `scope` should do, if it were 
allowed to apply to member variables.)

(2) As restricted to the lifetime of the aggregate instance only 
if the aggregate instance itself is `scope`. (This is the 
default.)

(3) Inconsistently, if some member functions are correctly 
written and annotated to provoke (1), and others are not. (This 
is perhaps the easiest to achieve, despite having no obvious 
valid uses.)

The missing option is:

(4) As possessing "infinite" lifetime, regardless of whether the 
aggregate instance is `scope`, with the compiler forbidding any 
operation in `@safe` code which might cause the indirection to be 
assigned a target with finite lifetime. (This is the opposite of 
`scope`, a concept which has no name in D at the moment.)

Since there is no `@safe` way to *increase* a target's perceived 
lifetime, restrictions are transitive and contagious. In order to 
achieve something resembling (4) we must somehow 
[launder](https://en.wikipedia.org/wiki/Money_laundering) our 
indirection, to hide its origin from DIP1000's data flow analysis.

A very stupid, but `@safe` way:
```D
struct VErr {
     // For simplicity, this assumes that each VErr instance will 
only ever be accessed from a single thread.
     private static string[size_t] _mTable;
     @property ref string m() scope @safe {
         return _mTable.require(cast(size_t) &this, null); }
     @property ref const(string) m() scope const @safe {
         return _mTable.require(cast(size_t) &this, null); }

     ~this() @safe {
         _mTable.remove(cast(size_t) &this);
     }
}
```

An efficient way, but it requires `@trusted` and might need more 
work to ensure it doesn't accidentally enable memory safety 
violations in some way that I missed:
```D
struct VErr {
     private string _m;
     @property ref inout(string) m() scope inout pure @trusted {
         return _m; }

     this(scope ref inout(VErr) that) scope inout pure @safe {
         this._m = that.m; }
     ref typeof(this) opAssign(scope ref inout(VErr) that) return 
scope pure @safe {
         this._m = that.m;
         return this;
     }
}
version(unittest)
     static string VErr_escape;
@safe unittest {
     scope string z = "z";

     scope VErr a;
     a.m = "y";
     assert(a.m == "y");
     static assert(!__traits(compiles, a.m = z));
     VErr_escape = a.m;

     VErr b = a;
     b.m = "x";
     assert(b.m == "x");
     b.m = a.m;
     assert(b.m == "y");
     static assert(!__traits(compiles, b.m = z));
}
```

Again, you don't need to apply all three of those fixes - just 
one will do. Which one you should use depends on whether 
`MBuf.write` needs to escape, and what allocation strategies you 
want to use for `vr` and the target(s) of `VErr.m`.