Safe ref counted

changelog/borrow_for_refcounted.dd

@@ -0,0 +1,52 @@
+Added `SafeRefCounted`, that can be used in `@safe` with `-preview=dip1000`.
+
+`RefCounted` is only available for `@system` code, because of the possibility of
+escaping a reference to its payload past the end of its lifetime. a modified
+copy of it, `std.typecons.SafeRefCounted` has been added. Also added is a
+`borrow` function, that lets one safely access and modify the payload.
+`-preview=dip1000` prevents escaping a reference to it in `@safe` code.
+
+-------
+@safe pure nothrow void fun()
+{
+    import std.typecons;
+
+    auto rcInt = safeRefCounted(5);
+    assert(rcInt.borrow!(theInt => theInt) == 5);
+    auto sameInt = rcInt;
+    assert(sameInt.borrow!"a" == 5);
+
+    // using `ref` in the function
+    auto arr = [0, 1, 2, 3, 4, 5, 6];
+    sameInt.borrow!(ref (x) => arr[x]) = 10;
+    assert(arr == [0, 1, 2, 3, 4, 10, 6]);
+
+    // modifying the payload via an alias
+    sameInt.borrow!"a*=2";
+    assert(rcInt.borrow!"a" == 10);
+}
+-------
+
+Direct access to the payload unfortunately has to be `@system`, though. While
+`-dip1000` could prevent escaping the reference, it is possible to destroy the
+last reference before the end of it's scope:
+
+-------
+int destroyFirstAndUseLater()
+{
+    import std.typecons;
+
+    auto rc = SafeRefCounted!int(123);
+    int* ptr = &rc.refCountedPayload();
+    destroy(rc);
+    return *ptr; // Reads from freed memory. Don't do this.
+}
+-------
+
+As a side effect, this enabled us to make $(REF dirEntries, std, file) `@safe`
+with `-preview=dip1000`.
+
+Some member functions of `RefCounted` that are `@safe` are not so in
+`SafeRefCounted`. The `RefCounted` type and `refCounted` function are still
+available for the old behaviour. However, their main purpose is backwards
+compatibility. They are not recommended for new code.

std/container/binaryheap.d

@@ -89,6 +89,8 @@ if (isRandomAccessRange!(Store) || isRandomAccessRange!(typeof(Store.init[])))
         Store _store;
         size_t _length;
     }
+    // TODO: migrate to use the SafeRefCounted. The problem is that some member
+    // functions here become @system with a naive switch.
     private RefCounted!(Data, RefCountedAutoInitialize.no) _payload;
     // Comparison predicate
     private alias comp = binaryFun!(less);

std/file.d

@@ -4465,9 +4465,10 @@ void rmdirRecurse(ref scope DirEntry de) @safe
     }
     else
     {
-        // dirEntries is @system because it uses a DirIterator with a
-        // RefCounted variable, but here, no references to the payload is
-        // escaped to the outside, so this should be @trusted
+        // dirEntries is @system without DIP1000 because it uses
+        // a DirIterator with a SafeRefCounted variable, but here, no
+        // references to the payload are escaped to the outside, so this should
+        // be @trusted
         () @trusted {
             // all children, recursively depth-first
             foreach (DirEntry e; dirEntries(de.name, SpanMode.depth, false))
@@ -4863,20 +4864,31 @@ private struct DirIteratorImpl
     }
 }
 
-struct DirIterator
+// Must be a template, because the destructor is unsafe or safe depending on
+// whether `-preview=dip1000` is in use. Otherwise, linking errors would
+// result.
+struct _DirIterator(bool useDIP1000)
 {
-@safe:
+    static assert(useDIP1000 == dip1000Enabled,
+        "Please don't override useDIP1000 to disagree with compiler switch.");
+
 private:
-    RefCounted!(DirIteratorImpl, RefCountedAutoInitialize.no) impl;
+    SafeRefCounted!(DirIteratorImpl, RefCountedAutoInitialize.no) impl;
+
     this(string pathname, SpanMode mode, bool followSymlink) @trusted
     {
         impl = typeof(impl)(pathname, mode, followSymlink);
     }
 public:
-    @property bool empty() { return impl.empty; }
-    @property DirEntry front() { return impl.front; }
-    void popFront() { impl.popFront(); }
+    @property bool empty() @trusted { return impl.empty; }
+    @property DirEntry front() @trusted { return impl.front; }
+    void popFront() @trusted { impl.popFront(); }
 }
+
+// This has the client code to automatically use and link to the correct
+// template instance
+alias DirIterator = _DirIterator!dip1000Enabled;
+
 /++
     Returns an $(REF_ALTTEXT input range, isInputRange, std,range,primitives)
     of `DirEntry` that lazily iterates a given directory,
@@ -4890,6 +4902,11 @@ public:
     operating system / filesystem, and may not follow any particular sorting.
 
     Params:
+        useDIP1000 = used to instantiate this function separately for code with
+                     and without -preview=dip1000 compiler switch, because it
+                     affects the ABI of this function. Set automatically -
+                     don't touch.
+
         path = The directory to iterate over.
                If empty, the current directory will be iterated.
 
@@ -4955,9 +4972,13 @@ foreach (d; dFiles)
     writeln(d.name);
 --------------------
  +/
-auto dirEntries(string path, SpanMode mode, bool followSymlink = true)
+
+// For some reason, doing the same alias-to-a-template trick as with DirIterator
+// does not work here.
+auto dirEntries(bool useDIP1000 = dip1000Enabled)
+    (string path, SpanMode mode, bool followSymlink = true)
 {
-    return DirIterator(path, mode, followSymlink);
+    return _DirIterator!useDIP1000(path, mode, followSymlink);
 }
 
 /// Duplicate functionality of D1's `std.file.listdir()`:
@@ -4976,13 +4997,14 @@ auto dirEntries(string path, SpanMode mode, bool followSymlink = true)
             .array;
     }
 
-    void main(string[] args)
+    // Can be safe only with -preview=dip1000
+    @safe void main(string[] args)
     {
         import std.stdio;
 
         string[] files = listdir(args[1]);
         writefln("%s", files);
-     }
+    }
 }
 
 @system unittest
@@ -5057,14 +5079,15 @@ auto dirEntries(string path, SpanMode mode, bool followSymlink = true)
 }
 
 /// Ditto
-auto dirEntries(string path, string pattern, SpanMode mode,
+auto dirEntries(bool useDIP1000 = dip1000Enabled)
+    (string path, string pattern, SpanMode mode,
     bool followSymlink = true)
 {
     import std.algorithm.iteration : filter;
     import std.path : globMatch, baseName;
 
     bool f(DirEntry de) { return globMatch(baseName(de.name), pattern); }
-    return filter!f(DirIterator(path, mode, followSymlink));
+    return filter!f(_DirIterator!useDIP1000(path, mode, followSymlink));
 }
 
 @safe unittest
@@ -5145,7 +5168,7 @@ auto dirEntries(string path, string pattern, SpanMode mode,
 
 // Make sure that dirEntries does not butcher Unicode file names
 // https://issues.dlang.org/show_bug.cgi?id=17962
-@system unittest
+@safe unittest
 {
     import std.algorithm.comparison : equal;
     import std.algorithm.iteration : map;

std/traits.d

@@ -9136,3 +9136,11 @@ package(std) template DeducedParameterType(T)
 {
     static assert(is(DeducedParameterType!(inout(int[])) == inout(int)[]));
 }
+
+private auto dip1000Test(int x) {return *&x;}
+// We don't use isSafe, because betterC client code needs to instantiate
+// core.internal.array.comparison.__cmp in the client side. isSafe uses
+// __cmp of two strings, so using it would instantate that here instead. That
+// won't do because betterC compilations do not link the Phobos binary in.
+package(std) enum dip1000Enabled
+    = is(typeof(&dip1000Test) : int function(int) @safe);