www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Weird DIP1000 issue

reply 0xEAB <desisma heidel.beer> writes:
Hi,

I’ve recently run into an issue with DIP1000.

After a lot of confusion by myself and suggestions from fellow 
coders on Discord (Thanks everyone!) I went on with implementing 
a potential workaround. Well… until my issue was suddenly gone 
although there was no actual workaround in place yet.

Yesterday, it came to my mind that there must be something off. 
And that our/my original conclusions might not be exactly sound.

So I went back to the reduced sample code provided by Mayonix 
(special thanks for throwing it into dustmite!) and compared it 
to the working version. I found something interesting and 
prepared this post. Then double checked my code samples… Just to 
notice the error is gone (again!). “What’s going on?”

The issue comes down to:
```
noscope360.d(5): Error: scope variable `c` assigned to non-scope 
parameter `_param_0` calling `write`
noscope360.d(13):        which is not `scope` because of `buffer 
= _param_0`
```

Here’s the code snippet:
```d
void main()  safe {
     auto mb = MBuf();
     auto vr = VRes!X();
     foreach(c; vr.errors)
         mb.write(c);
}

struct X { }

struct MBuf {
     const(ubyte)[] _bufferList;
     void write(Buffers...)(Buffers buffers) {
         foreach (buffer; buffers) { }
     }
}

struct VErr { string m; }

struct VRes(Data) {
     VErr[Data.tupleof.length] _errors;
     auto errors() {
         import std.algorithm;
         return _errors[].filter!(e => e.m);
     }
}
```

And two potential fixes:

```diff
--- noscope360.d
+++ yoscope360.d
   -8,7 +8,6   
  struct X { }

  struct MBuf {
-    const(ubyte)[] _bufferList;
      void write(Buffers...)(Buffers buffers) {
          foreach (buffer; buffers) { }
      }
```


```diff
--- noscope360.d
+++ throwscope360.d
   -10,7 +10,7   
  struct MBuf {
      const(ubyte)[] _bufferList;
      void write(Buffers...)(Buffers buffers) {
-        foreach (buffer; buffers) { }
+        static foreach (buffer; buffers) { }
      }
  }
```

The changes from *throwscope360* are obviously not doable in 
practice, as they remove code that would have been used 
eventually.

Does it make sense to have `*static* foreach` mandatory here?

I don’t feel like those changes should matter.
While I’m by no means sure that this is a bug, I would really 
hope for it to be one.
Feb 07 2023
next sibling parent 0xEAB <desisma heidel.beer> writes:
Steven told me this has to do with inference.


 […] it’s complaining about things that you didn’t even write 
 (these scope attributes are all inferred)


Not sure how to put this…
Is attribute inference “supposed” to create such issues?
Feb 07 2023
prev sibling next sibling parent reply tsbockman <thomas.bockman gmail.com> writes:
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`.
Feb 08 2023
parent tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 8 February 2023 at 08:47:45 UTC, tsbockman wrote:

 (It may be possible to fix it with changes to 
 `std.algorithm.filter`, though.)


This compiles and runs without error for me:

```D
void main()  safe {
     auto mb = MBuf();
     auto vr = VRes!X();
     foreach(c; vr.errors)
         mb.write(c);
}

struct X { }

struct MBuf {
     const(ubyte)[] _bufferList;
     void write(Buffers...)(Buffers buffers) {
         foreach (buffer; buffers) { }
     }
}

struct VErr { string m; }

struct VRes(Data) {
     VErr[Data.tupleof.length] _errors;
     auto errors() {
         return _errors[].filter!(e => e.m);
     }
}

auto filter(alias predicate, Element)(return Element[] range) {
     static struct Filtered {
         private Element[] range;

         this(return inout(Element)[] range) scope inout pure 
 safe {
             this.range = filterFront(range);
         }

         bool empty() scope const pure  safe {
             return range.length == 0; }
         ref inout(Element) front() scope inout pure  safe {
             return range[0]; }
         void popFront() scope pure  safe {
             range = filterFront(range[1 .. $]); }
         static inout(Element)[] filterFront(return 
inout(Element)[] range) pure  safe {
             while(true) {
                 if(range.length == 0 || predicate(range[0]))
                     return range;
                 range = range[1 .. $];
             }
         }
     }
     return Filtered(range);
}
 safe unittest {
     const int[8] ns = [ 0, 1, 2, 3, 4, 5, 6, 7 ];
     const int[4] odds = [ 1, 3, 5, 7 ];

     size_t o = 0;
     foreach(n; ns.filter!(n => n % 2 == 1)) {
         assert(o < odds.length);
         assert(n == odds[o]);
         ++o;
	}
     assert(o == odds.length);
}
```

Perhaps there is a bug or unnecessary restriction somewhere in 
`std.algorithm.filter`'s API?
Feb 08 2023
prev sibling parent reply ag0aep6g <anonymous example.com> writes:
On 08.02.23 00:42, 0xEAB wrote:

 ```d
 void main()  safe {
      auto mb = MBuf();
      auto vr = VRes!X();
      foreach(c; vr.errors)
          mb.write(c);
 }
 
 struct X { }
 
 struct MBuf {
      const(ubyte)[] _bufferList;
      void write(Buffers...)(Buffers buffers) {
          foreach (buffer; buffers) { }
      }
 }
 
 struct VErr { string m; }
 
 struct VRes(Data) {
      VErr[Data.tupleof.length] _errors;
      auto errors() {
          import std.algorithm;
          return _errors[].filter!(e => e.m);
      }
 }
 ```


[...]

 While I’m by no means sure that this is a bug, I would really hope for 
 it to be one.


Here's a further reduction of one aspect of your snippet that looks like 
a bug to me:

----
alias VErr = char*;

ref front_r(ref VErr r) { return r; }
ref front_p(VErr* p) { return *p; }
ref front_s(VErr[] s) { return s[0]; }

VErr g;

void main()  safe
{
     VErr[1] _errors;
     g = _errors[0]; /* copy from static array: ok */
     g = front_r(_errors[0]); /* copy from `ref` via `ref`: ok */
     g = front_p(&_errors[0]); /* copy from `ref` via pointer:
                                  fails, should work */
     g = front_s(_errors[]); /* copy from `ref` via slice:
                                fails, should work */
}
----
Feb 08 2023
next sibling parent 0xEAB <desisma heidel.beer> writes:
On Wednesday, 8 February 2023 at 13:00:35 UTC, ag0aep6g wrote:

 Here's a further reduction of one aspect of your snippet that 
 looks like a bug to me:


thank you, quite interesting
Feb 08 2023
prev sibling next sibling parent reply tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 8 February 2023 at 13:00:35 UTC, ag0aep6g wrote:

 Here's a further reduction of one aspect of your snippet that 
 looks like a bug to me:

 ```D
 alias VErr = char*;

 ref front_r(ref VErr r) { return r; }
 ref front_p(VErr* p) { return *p; }
 ref front_s(VErr[] s) { return s[0]; }

 VErr g;

 void main()  safe
 {
     VErr[1] _errors;
     g = _errors[0]; /* copy from static array: ok */
 ```


That's inside the same function, so data flow analysis can see 
that no `scope` address has been assigned to any element of 
`_errors`.


 ```D
     g = front_r(_errors[0]); /* copy from `ref` via `ref`: ok */
 ```


`ref` is head `scope`, and does not infect the `char*` itself.


 ```D
     g = front_p(&_errors[0]); /* copy from `ref` via pointer:
                                  fails, should work */
 ```


Since `&_errors[0]` is a pointer to a stack variable, it must be 
`scope`. Pointers get transitive `scope`, so the `char*` is 
infected, too.


 ```D
     g = front_s(_errors[]); /* copy from `ref` via slice:
                                fails, should work */
 ```


Again, the `_errors[]` slice contains a pointer to a stack 
variable, so it must be `scope`. Slices get transitive `scope`, 
like pointers.


 ```D
 }
 ```


I think most of the confusion surrounding `scope` and related 
attributes stems from the fact that `scope` is transitive, when 
it really shouldn't be.

Instead, we should be allowed to directly specify for each level 
of indirection, and for each aggregate field whether it is always 
`scope`, follows the `scope`ness of its parent (as now), or is 
never `scope`.

The current design is fundamentally confusing and incoherent, 
because the true root of most every chain of indirections is some 
offset from the stack pointer, which must be `scope` if its 
existence is explicitly acknowledged. Since `scope` is 
transitive, this implies that most everything should be `scope`, 
which is stupid.
Feb 08 2023
parent reply Paul Backus <snarwin gmail.com> writes:
On Wednesday, 8 February 2023 at 22:25:32 UTC, tsbockman wrote:

 Since `&_errors[0]` is a pointer to a stack variable, it must 
 be `scope`. Pointers get transitive `scope`, so the `char*` is 
 infected, too.


[...]

 Again, the `_errors[]` slice contains a pointer to a stack 
 variable, so it must be `scope`. Slices get transitive `scope`, 
 like pointers.


[...]

 I think most of the confusion surrounding `scope` and related 
 attributes stems from the fact that `scope` is transitive, when 
 it really shouldn't be.


This really is a good illustration of the confusion surrounding 
`scope`, because `scope` is not transitive and never has been:

```d
char* global;

void main()  safe
{
     char* local;
     scope char** p = &local;
     global = *p; // ok
}
```
Feb 08 2023
parent tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 8 February 2023 at 23:03:17 UTC, Paul Backus wrote:

 On Wednesday, 8 February 2023 at 22:25:32 UTC, tsbockman wrote:

 I think most of the confusion surrounding `scope` and related 
 attributes stems from the fact that `scope` is transitive, 
 when it really shouldn't be.


 This really is a good illustration of the confusion surrounding 
 `scope`, because `scope` is not transitive and never has been:

 ```d
 char* global;

 void main()  safe
 {
     char* local;
     scope char** p = &local;
     global = *p; // ok
 }
 ```


Well then, I guess I don't understand the rules either.

In the [original 
sample](https://forum.dlang.org/post/zqvewsixzjvrothpqkzz forum.dlang.org) the
compiler insists that the value of `c` and `c.m` must be `scope` if `&vr` is,
when it is only the next level of indirection up - the memory storing that
value - that is actually on the stack within `vr`.

Aggregates sometimes behave in ways that at least resemble 
`scope` transitivity, even if it's not really the same thing:

```D
struct S {
     int* i;
     int* j;
}

int* escape;
int x, y;

void main()  safe
{
     int z;

     S a;
     a.i = &x;
     a.j = &y;
     escape = a.i; // Allowed

     S b;
     b.i = &y;
     b.j = &z;
     escape = b.i; // Rejected because b.j needs scope, which 
apparently forces b and all its fields to be scope, too.
}
```
Feb 08 2023
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
Simplifying your example:


 ref front_p(int* p) { return *p; }

 void main()  safe
 {
      int _errors;
      front_p(&_errors); /* copy from `ref` via pointer:
                                   fails,
should work */
 }


1. &_errors is a pointer to the stack.

2. passing a pointer to the stack to front_p's `p` variable.

3. Since `p` is not declared `scope`, the compiler assumes that front_p allows 
`p` to escape. (Inference of attributes does not happen for regular functions.)

4. An escaping pointer to the stack is not allowed by dip1000 rules.


The compiler is working as expected.

Ruthlessly cutting down the example, as I did here, helps a lot to expose what 
the real problem is.
Feb 08 2023
parent reply ag0aep6g <anonymous example.com> writes:
On Wednesday, 8 February 2023 at 23:32:16 UTC, Walter Bright 
wrote:

 Simplifying your example:


 ref front_p(int* p) { return *p; }

 void main()  safe
 {
      int _errors;
      front_p(&_errors); /* copy from `ref` via pointer:
                                   fails,
should work */
 }




That passes compilation whereas my code fails compilation. So 
it's not a proper reduction. Try again.

[...]

 3. Since `p` is not declared `scope`, the compiler assumes that 
 front_p allows `p` to escape. (Inference of attributes does not 
 happen for regular functions.)


Attribute inference does happen for `front_p`, because it has no 
return type declared.
Feb 08 2023
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 2/8/2023 3:47 PM, ag0aep6g wrote:

 Attribute inference does happen for `front_p`, because it has no return type 
 declared.


I missed that. Thanks for the correction.
Feb 08 2023
parent reply Walter Bright <newshound2 digitalmars.com> writes:
https://issues.dlang.org/show_bug.cgi?id=23682
Feb 08 2023
parent reply 0xEAB <desisma heidel.beer> writes:
On Thursday, 9 February 2023 at 02:11:01 UTC, Walter Bright wrote:

 https://issues.dlang.org/show_bug.cgi?id=23682


Thank you very much for filing the bug report for me :)
Feb 10 2023
parent Walter Bright <newshound2 digitalmars.com> writes:
On 2/10/2023 7:48 AM, 0xEAB wrote:

 Thank you very much for filing the bug report for me :)


Welcs!

https://github.com/dlang/dmd/pull/14869
Feb 10 2023