Thread overview
The evils of __traits(compiles)
Jan 21, 2021
H. S. Teoh
Jan 21, 2021
Paul Backus
Jan 21, 2021
jmh530
Jan 21, 2021
Paul Backus
Jan 21, 2021
jmh530
Jan 21, 2021
Paul Backus
Jan 22, 2021
SealabJaster
January 21, 2021
Ran into this today: was submitting a PR for the ddbus package, which has a function .registerMethods that allows automatic registration of DBus methods by introspecting a class object, and generating the appropriate setHandler calls to the router object.  Since not all method signatures can be supported by DBus, .registerMethods uses __traits(compiles) as a quick way of determining whether setHandler is usable on a particular method (unsupported methods will not compile when passed to setHandler, so they will be skipped).

Unfortunately, my PR touches one of the functions used by setHandler. A typo caused a compile error inside setHandler itself, but since __traits(compiles) does not distinguish between errors caused by the candidate method and errors inside setHandler itself, the erroneous code simply caused *all* methods to be skipped -- silently.  The problem would not be noticed until runtime when some methods mysteriously go missing. :-(

This problem isn't specific to ddbus; I've run into this in a lot of D code that's heavy on compile-time introspection.  __traits(compiles) is a quick and easy way of getting code to work, but it inevitably leads to pain because of the way it gags *all* compile errors, including the ones you didn't expect and probably should be reported.

tl;dr: __traits(compiles) is evil, and should be avoided. Unless you *really* mean, literally, "does this piece of code compile", and you're not using that as a stand-in for some other intended semantics (like "does X conform to Y's signature constraints" or some such).  SFINAE is evil. >:-(


T

-- 
Unix is my IDE. -- Justin Whear
January 21, 2021
On Thursday, 21 January 2021 at 20:27:36 UTC, H. S. Teoh wrote:
> Unfortunately, my PR touches one of the functions used by setHandler. A typo caused a compile error inside setHandler itself, but since __traits(compiles) does not distinguish between errors caused by the candidate method and errors inside setHandler itself, the erroneous code simply caused *all* methods to be skipped -- silently.  The problem would not be noticed until runtime when some methods mysteriously go missing. :-(

This seems like a bug that would have been easily caught by a unit test for setHandler.

Remember: the compiler does not check your templates until you actually try to instantiate them. If you want to be sure that they instantiate correctly, you have to test them.
January 21, 2021
On Thursday, 21 January 2021 at 20:27:36 UTC, H. S. Teoh wrote:
> [snip]
>
> tl;dr: __traits(compiles) is evil, and should be avoided. Unless you *really* mean, literally, "does this piece of code compile", and you're not using that as a stand-in for some other intended semantics (like "does X conform to Y's signature constraints" or some such).  SFINAE is evil. >:-(
>
>
> T

It's just so easy and when it works it works without requiring much thought. Check out below. Not so easy to replace isAddable.

struct Foo
{
    int payload;

    Foo opBinary(string op)(Foo x)
        if (op == "*")
    {
     	return Foo(payload * x.payload);
    }
}

enum bool isAddable(T) = __traits(compiles, {
    auto temp = T.init + T.init;
});

void main() {
    static assert(isAddable!int);
    static assert(!isAddable!Foo);
}

January 21, 2021
On Thursday, 21 January 2021 at 21:20:50 UTC, jmh530 wrote:
>
> It's just so easy and when it works it works without requiring much thought. Check out below. Not so easy to replace isAddable.
[...]
> enum bool isAddable(T) = __traits(compiles, {
>     auto temp = T.init + T.init;
> });
>
> void main() {
>     static assert(isAddable!int);
>     static assert(!isAddable!Foo);
> }

This is also a great illustration of the pitfalls of using __traits(compiles) without thorough unit testing, because your code has a bug in it that isn't covered by your tests:

    static assert(isAddable!(inout(int))); // fails
January 21, 2021
On Thursday, 21 January 2021 at 21:42:19 UTC, Paul Backus wrote:
> [snip]
>
> This is also a great illustration of the pitfalls of using __traits(compiles) without thorough unit testing, because your code has a bug in it that isn't covered by your tests:
>
>     static assert(isAddable!(inout(int))); // fails

The code as it is used in the library requires isMutable!T before T gets passed into that logic.

However, I still think it's weird. Changing inout to const or immutable works. I never would think to use inout(int) that way...
January 21, 2021
On 1/21/21 3:27 PM, H. S. Teoh wrote:
> Ran into this today: was submitting a PR for the ddbus package, which
> has a function .registerMethods that allows automatic registration of
> DBus methods by introspecting a class object, and generating the
> appropriate setHandler calls to the router object.  Since not all method
> signatures can be supported by DBus, .registerMethods uses
> __traits(compiles) as a quick way of determining whether setHandler is
> usable on a particular method (unsupported methods will not compile when
> passed to setHandler, so they will be skipped).
> 
> Unfortunately, my PR touches one of the functions used by setHandler. A
> typo caused a compile error inside setHandler itself, but since
> __traits(compiles) does not distinguish between errors caused by the
> candidate method and errors inside setHandler itself, the erroneous code
> simply caused *all* methods to be skipped -- silently.  The problem
> would not be noticed until runtime when some methods mysteriously go
> missing. :-(
> 
> This problem isn't specific to ddbus; I've run into this in a lot of D
> code that's heavy on compile-time introspection.  __traits(compiles) is
> a quick and easy way of getting code to work, but it inevitably leads to
> pain because of the way it gags *all* compile errors, including the ones
> you didn't expect and probably should be reported.
> 
> tl;dr: __traits(compiles) is evil, and should be avoided. Unless you
> *really* mean, literally, "does this piece of code compile", and you're
> not using that as a stand-in for some other intended semantics (like
> "does X conform to Y's signature constraints" or some such).  SFINAE is
> evil. >:-(

https://forum.dlang.org/post/rj5hok$c6q$1@digitalmars.com

-Steve
January 21, 2021
On Thursday, 21 January 2021 at 22:09:46 UTC, jmh530 wrote:
> On Thursday, 21 January 2021 at 21:42:19 UTC, Paul Backus wrote:
>> [snip]
>>
>> This is also a great illustration of the pitfalls of using __traits(compiles) without thorough unit testing, because your code has a bug in it that isn't covered by your tests:
>>
>>     static assert(isAddable!(inout(int))); // fails
>
> The code as it is used in the library requires isMutable!T before T gets passed into that logic.
>
> However, I still think it's weird. Changing inout to const or immutable works. I never would think to use inout(int) that way...

You probably wouldn't use inout that way directly, but you might write something like

    auto fun(T)(T a, T b)
        if (isAddable!T)
    {
        // do something with a + b
    }

...and then later on down the line you might call `fun` in some context where T is inout-qualified, like a copy constructor.
January 22, 2021
On Thursday, 21 January 2021 at 23:05:16 UTC, Steven Schveighoffer wrote:
> https://forum.dlang.org/post/rj5hok$c6q$1@digitalmars.com
>
> -Steve

If there's any singular thing I'd ever want to see from D this year, this is it.

Something like that would make debugging so much nicer, especially when using libraries.