www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - The evils of __traits(compiles)

reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
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
Jan 21 2021
next sibling parent Paul Backus <snarwin gmail.com> writes:
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.
Jan 21 2021
prev sibling next sibling parent reply jmh530 <john.michael.hall gmail.com> writes:
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);
}
Jan 21 2021
parent reply Paul Backus <snarwin gmail.com> writes:
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
Jan 21 2021
parent reply jmh530 <john.michael.hall gmail.com> writes:
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...
Jan 21 2021
parent Paul Backus <snarwin gmail.com> writes:
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.
Jan 21 2021
prev sibling parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
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
Jan 21 2021
parent SealabJaster <sealabjaster gmail.com> writes:
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.
Jan 21 2021