@safe tests in std.uni

[John-Colvin]

No need for a lot of them to be @system any more, things have improved.

[dlang-bot]

Thanks for your pull request and interest in making D better, @John-Colvin! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#6330"

[John-Colvin]

Relates to https://issues.dlang.org/show_bug.cgi?id=18187, I didn't put "Fixed ...." in the commit because it's not a "fix for a bug" as such.

[wilzbach]

I think some of these unittest are only now @safe, because we recently added -dip1000 to some of the modules of the Posix Makefile (std.uni being one of them).
This would explain the Windows failures:

std\uni.d(1548): Error: cannot take address of local `idxArray` in `@safe` function `__unittest_L1545_C7`
std\uni.d(1573): Error: cannot take address of local `other` in `@safe` function `__unittest_L1545_C7`
std\uni.d(1574): Error: cannot take address of local `idxArray` in `@safe` function `__unittest_L1545_C7`
std\uni.d(1577): Error: cannot take address of local `idxArray` in `@safe` function `__unittest_L1545_C7`

We could change sliceOverIndexed to accept an ref overload too ...

I didn't put "Fixed ...." in the commit because it's not a "fix for a bug" as such.

Well, then it would appear in the changelog. Even though the actual fix might not have happened in this PR, it doesn't matter from the user PoV as they then see "hey I can now use XXX in @safe" in the changelog. The changelog is generated from commit messages, so it's rather hard to add Bugzilla references that aren't mentioned in a commit.

[wilzbach]

Relates to https://issues.dlang.org/show_bug.cgi?id=18187, I didn't put "Fixed ...." in the commit because it's not a "fix for a bug" as such.

FYI: if you don't have "Fix(es)" in the commit message, the bot will just detect the issue, but won't close it nor mention it in the changelog.

[carblue]

Referring to Auto-Tester's failing unittest (std/uni.d(1548): Error...), my results (Linux x86_64) are:

@System unittest <= compiles with both, either -dip25 or -dip1000
@safe unittest <= compiles with -dip1000 only (-dip25 results in the same errors as in Auto-Tester)

If You want this @safe unittest, then You must use version(DIP1000) here, otherwise You're going to break any users builds, who don't use -dip1000 !
These will be the related changes required then:

user@host:~/git/phobos$ git diff
diff --git a/dip1000.mak b/dip1000.mak
index 603a391..e1c4bc5 100644
--- a/dip1000.mak
+++ b/dip1000.mak
@@ -48,7 +48,7 @@ aa[std.system]=-dip1000
 aa[std.traits]=-dip1000
 aa[std.typecons]=-dip25 # cannot call @system function std.format.formattedWrite!(Appender!string, char, Nullable!int)
 aa[std.typetuple]=-dip1000
-aa[std.uni]=-dip1000 # merged https://github.com/dlang/phobos/pull/6294, https://github.com/dlang/phobos/pull/6041 (see also TODO-list there); supersedes/includes https://github.com/dlang/phobos/pull/5045; see also https://github.com/dlang/phobos/pull/6104 for improvements proposed by Seb
+aa[std.uni]=-dip1000 -version=DIP1000 # merged https://github.com/dlang/phobos/pull/6330, https://github.com/dlang/phobos/pull/6294, https://github.com/dlang/phobos/pull/6041 (see also TODO-list there); supersedes/includes https://github.com/dlang/phobos/pull/5045; see also https://github.com/dlang/phobos/pull/6104 for improvements proposed by Seb
 aa[std.uri]=-dip1000
 aa[std.utf]=-dip1000 # for me (carblue) std.utf is -dip1000 compilable even without applying https://github.com/dlang/phobos/pull/5915, i.e. I don't observe a depends on (?); after applying PR 5915 it's still dip1000
 aa[std.uuid]=-dip1000
diff --git a/std/uni.d b/std/uni.d
index fa81234..f465f2d 100644
--- a/std/uni.d
+++ b/std/uni.d
@@ -1542,7 +1542,8 @@ if (is(Unqual!T == T))
     return SliceOverIndexed!T(a, b, x);
 }
 
-@system unittest
+version(DIP1000)
+@safe unittest
 {
     int[] idxArray = [2, 3, 5, 8, 13];
     auto sliced = sliceOverIndexed(0, idxArray.length, &idxArray);

@wilzbach For this PR to pass Windows build-testing, we need the "dip1000.mak-mechanism" in the win*.mak files as well

[wilzbach]

@wilzbach For this PR to pass Windows build-testing, we need the "dip1000.mak-mechanism" in the win*.mak files as well

It's not as easy as that, because with -deps DMD currently compiles ALL unittest into your program. So we can't add a DIP1000 only test as long as DIP1000 isn't the default and will need to version it out until then. This caused quite some problems when we introduced StdUnittest (see e.g. #6159 (comment)).
There's also the issue of keeping track of three dip1000.mak's is more work and confusing as just getting rid of all non-dip1000 modules in dip1000.mak and then making it default in all Makefiles.

[carblue]

@wilzbach
Did I understand right, that what You call "will need to version it out until then" is what we are doing with version(DIP1000)? Yes, then it's what I think is correct:

1. In this PR, this one @System => @safe unittest change must be inactive for general use, same as
2. In std.container.slist: Fix a @safe cannot call @system issue #6295, the lately added unittest (in the end) inactive for general use
   "general use" meaning, that it's not guaranteed that -dip1000 is set.

I was wrong with "For this PR to pass Windows build-testing...". As long as we take care, that modules (in dip1000,mak marked as -dip1000) compile with both (-dip25 / -dip1000) PRs should continue to compile on Windows as well (just not -dip1000 unittested)

[wilzbach]

Did I understand right, that what You call "will need to version it out until then" is what we are doing with version(DIP1000)?

Yup, essentially what's failing on Windows would be failing with -deps too.
(CC @FeepingCreature who ran into this problem previously - that's how I understood your problem with -deps?)

In this PR, this one @System => @safe unittest change must be inactive for general use, same as

Yes :/

Alternatives that come to my mind:

1. leave this test as is
2. version it with version(DIP1000)
3. make a @safe copy of the @System unittest with version(DIP1000)
4. fix the unittest to be @safe with DMD's current @Safe-ty checker (though that's a bit pointless if we hopefully soon do the dip1000 transition)
5. put it into a version(unittest) function and call it from an @safe version(DIP1000) unittest and a normal @System one

The downside of (2) would be that we don't run this test on Windows during this transition, so probably (5) is the best.

In #6295, the lately added unittest (in the end) inactive for general use
"general use" meaning, that it's not guaranteed that -dip1000 is set.

Yes :/

[FeepingCreature]

Correct, this issue is anologous. -unittest -deps needs to recurse into Phobos unittests because unittests may import modules, causing dependency relations, and -deps doesn't know which unittests are in a module that's in a library and which are in user code.