[Issue 15320] eliminate 'static assert(__traits(compiles,..'

[dcarp]

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

There are still some occurrences remaining:

• 12 in std/concurency.d: spawns create threads
• 3 in std/conv.d: couldn't figure it out
• 1 in std/exception.d: must stay, __traits(compiles is called in a loop with different checked results
• 24 in std/file.d: file operations
• 3 in std/numeric.d: don't know if it can/should be replaced
• 2 in std/traits.d: don't know how

The regex used for searching: static\s+assert\s*\(\s*__traits\s*\(\s*compiles

[dlang-bot]

Fix	Bugzilla	Description
✓	15320	static assert(__traits(compiles, xyz)) considered harmful in unittests

[andralex]

Thanks! Many of these could and should also be followed by an obvious assert that also ensures the behavior is correct. I'll provide a few examples inline.

[andralex]

assert(map!(i => i)(iota(0, 10, step)).walkLength == 10);

[JakobOvrum]

Some massive change (+- almost 1000 lines) to std.datetime appears to have snuck into the first commit.

[jmdavis]

Some massive change (+- almost 1000 lines) to std.datetime appears to have snuck into the first commit.

The changes are only snuck in in the sense that they're easy to miss. But there are a bunch of tests in std.datetime that use __traits(compilers, ...) to verify that a function is callable when the object is const or immutable, and for better or worse, this PR turns them all into runtime checks. I'd just as soon leave them as compile-time checks, since the behavior is already verified with the tests on mutable objects, and those functions don't do anything special for const or immutable, but I'm sure that Andrei would prefer that they not be compile-time checks.

[jmdavis]

I don't see anything obviously wrong with the changes to std.datetime, but it is a lot of changes, so I could have missed something.

[dcarp]

Does anybody have an idea how to translate those in std/conv.d and std/traits.d?

[jmdavis]

Are the brackets around "(int)" right? Of course the test passes, but they look strange to me.

Technically, they're parentheses (or parens), not brackets ([] would be brackets), but regardless, the reason that the parens are there because the result of Alias is pretty much the same as the result of AliasSeq/TypeTuple. It's just that Alias is more restrictive about what it accepts. So, yes, it's correct - though in general, I'm not sure that it's a good idea to have tests about the result of stringof, since there's no guarantee that it won't change. It probably won't change in this case, but I'm inclined to think that it's a bad idea to have a test which checks stringof, and I'd remove it.

[jmdavis]

Does anybody have an idea how to translate those in std/conv.d and std/traits.d?

The ones in std.conv seem to either be verifying that something doesn't compiler (in which case, it wouldn't make any sense to change them) or checking something that's deprecated. The ones checking deprecated behavior should probably be moved into a deprecated unittest block.

Similarly, most of those in std.traits test that something doesn't compile, and the ones that do can be changed to check the result like you've done with the rest.

[dcarp]

@jmdavis: It is not clear for me what the unittest bellow (from std.conv) tries to do. Does it tests that the opCall operands are not called?
But the test emplace(&s, 1); appears two times and it works because it calls the constructor. Is this deprecated?

    //With constructor
    {
        static struct S2
        {
            int i = 0;
            static S2 opCall(int*){assert(0);}
            static S2 opCall(int){assert(0);}
            this(int i){this.i = i;}
        }
        S2 s = void;
        static assert( __traits(compiles, emplace(&s, 1)));  //(works, but deprecated)
        static assert( __traits(compiles, emplace(&s, &i))); //(works, but deprecated)
        emplace(&s,  1);
        assert(s.i == 1);
    }

In std.traits I think that lvalue and rvalue are not meant to be used at runtime and their unittests can stay as they are.

[jmdavis]

It is not clear for me what the unittest bellow (from std.conv) tries to do. Does it tests that the opCall operands are not called?

Well, if you take the code inside the static assertions which is being tested to see whether it compiles and put it outside of the static assertion, you'll see the deprecation messages. For instance, if you change line# 4783 so that it's

    emplace(&s, &i);

then you get this:

std/conv.d(3959): Deprecation: function std.conv.emplaceOpCaller!(S2, int*).emplaceOpCaller is deprecated - Using static opCall for emplace is deprecated. Plase use emplace(chunk, T(args)) instead.

A separate unittest block will have to be created for the tests that test deprecated functionality so that that unittest block can be marked as deprecated (otherwise, the deprecation message will always print out with the build). Unfortunately, that does mean some code duplication, but it'll go away once the deprecated functionality has been removed (along with the associated tests). My guess is that static assertions were used to avoid having to create deprecated unittest blocks, but deprecated unittest blocks would be much clearer.

But the test emplace(&s, 1); appears two times and it works because it calls the constructor. Is this deprecated?

I would say that line 4782 is redundant and that it should be removed. It's a compile time test for something that's then tested at runtime. And it's clearly not deprecated functionality, whereas line 4783 clearly is. My guess is that it's a copy-past error.

In std.traits I think that lvalue and rvalue are not meant to be used at runtime and their unittests can stay as they are.

std.traits is all about compile time. None of its functionality is intended for runtime. Maybe Andrei would disagree, but I don't see any reason why changing those static assertions to runtime assertions would help anything. I'd just leave them as-is.

[dcarp]

Well, if you take the code inside the static assertions which is being tested to see whether it compiles and put it outside of the static assertion, you'll see the deprecation messages.

That's the problem, I don't see the deprecation message, but the runtime assertion:

  ../dmd/src/dmd -od$T -conf= -I../druntime/import  -w -dip25 -m64  -g -debug -main -unittest generated/linux/debug/64/libphobos2.a -defaultlib= -debuglib= -L-ldl -cov -run std/conv.d && \
  rm -rf $T
core.exception.AssertError@std/conv.d(4709): Assertion failure
----------------
src/core/exception.d:423 [0x52d2cb]
src/core/exception.d:622 [0x4ffdad]
posix.mak:362: recipe for target 'std/conv.test' failed
make: *** [std/conv.test] Error 1

Line 4709: static S2 opCall(int*){assert(0);}

[jmdavis]

That's the problem, I don't see the deprecation message, but the runtime assertion:> That's the problem, I don't see the deprecation message, but the runtime assertion:

Hmmm. I'm definitely seeing the deprecation message, but the assertion is caused, because the struct in question specifically asserts that opCall isn't used with emplace, making this a very weird test. Certainly, you can't make that a runtime test as-is. That particular static assertion would need to done on a different struct with the same API but without the assertion.

I'll make life simple here. Just don't worry about std.conv. The deprecation in question is almost two years old, so I'll just remove it and the offending tests in another PR which has to do with deprecations. This PR doesn't need to worry about it.

[dcarp]

I'll make life simple here. Just don't worry about std.conv. The deprecation in question is almost two years old, so I'll just remove it and the offending tests in another PR which has to do with deprecations. This PR doesn't need to worry about it.

This means that if there are no other comments, this PR is ready to be merged.

[jmdavis]

Auto-merge toggled on

[dcarp]

The autotester seems to be stuck on building Win_32_64
https://auto-tester.puremagic.com/pull-history.ghtml?projectid=1&repoid=3&pullid=3807