fail_if broken with check 0.15.1

[buscher]

fail_if is broken with check-0.15.1 (did not test 0.15.0)

The following code no longer compiles

#include <check.h>

void test_check0151()
{
    int a = 1, b = 2;
    fail_if(a == b);
}

with the output

test_check0151.c: In function 'test_check0151':
test_check0151.c:8:5: error: too few arguments to function '_ck_assert_failed'
    8 |     fail_if(a == b);
      |     ^~~~~~~
/usr/include/check.h:502:27: note: declared here
  502 | CK_DLL_EXP void CK_EXPORT _ck_assert_failed(const char *file, int line,
      |                           ^~~~~~~~~~~~~~~~~

I know fail_if is deprecated like forever... but old code is still using it :/

[brarcher]

Ah, I see. This was probably broken in 0.15.0 as well. Check has a test for fail_if and passes a NULL as a second argument. There is a comment around there as to why::

/* taking out the NULL provokes an ISO C99 warning in GCC */

The changes in 0.15.0 enabled the checking of the printf style arguments, and in doing so likely now points out when no arguments are provided, such as with fail_if(a == b).

The documentation on the deprecated fail_if mention:

(Deprecated) Fails test if supplied condition evaluates to true and
displays user provided message.

The intent appears to be to have callers pass in a message, rather than making it optional. Up to 0.15.0 it was not enforced, and now a missing message is being pointed out.

I tried adding something like this:

#define fail_if(expr) fail_if(expr, NULL)

to see if it could be worked around, but this re-defines the macro rather than provides an alternative, so it will not work.

There are a few ways to solve this:

1. Uses of fail_if() that have no message are given a NULL message (requires client code to change)
2. Printf style warnings introduced in 0.15.0 are removed

I'd rather not remove the new warnings as they will help code use the correct printf arguments for Check's APIs. Do you have some other suggestions that may work?

[mandree]

I'll prove "regression after 0.15.0 before 0.15.1" below, on FreeBSD ports and Git. I've tried building openvpn-auth-ldap, which is old code, and uses fail_() macros - and fail_() macros fail to compile with 0.15.1 due to one missing argument, but compiling the same source with 0.15.0 just fine.

I have also bisected this, and it got broken in 7ac1fcb and it doesn't look too surprising looking at the change.

$ git bisect bad
7ac1fcbcefe8813e2a75388ec61e20a184ddc8c8 is the first bad commit
commit 7ac1fcbcefe8813e2a75388ec61e20a184ddc8c8
Author: Jerry James <loganjerry@gmail.com>
Date:   Mon Jun 22 15:59:44 2020 -0600

    Make CK_ATTRIBUTE_FORMAT refer to the right arguments.

 src/check.c    | 12 +++++-------
 src/check.h.in | 13 +++++++------
 2 files changed, 12 insertions(+), 13 deletions(-)

[brarcher]

One other option that occurred to me is to have fail_if bypass the printf validation check, whereas the other assert macros continue to do so. Maybe something like this:

1da77e4

That definitely is a hack, but it should keep fail_if without arguments working. It does cause a new build warning on Clang, as the version of ck_assert_failed which does not validate its arguments is passing a non string literal to vsnprintf:

check.c:413:4: warning: format string is not a string literal [-Wformat-nonliteral]
   _ck_assert_failed_inner();
   ^~~~~~~~~~~~~~~~~~~~~~~~~
check.c:386:32: note: expanded from macro '_ck_assert_failed_inner'
        vsnprintf(buf, BUFSIZ, msg, ap);\
                               ^~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/secure/_stdio.h:75:63: note: 
      expanded from macro 'vsnprintf'
  __builtin___vsnprintf_chk (str, len, 0, __darwin_obsz(str), format, ap)
                                                              ^~~~~~

Does 1da77e4 look like it might work? Also, as a part of that change a new test will need to be added to check that confirms fail_if with no arguments continue to work.

[heftig]

I think the lazy fix for this would be adding back the extra NULL argument to the deprecated fail* macros.

You would have to tell projects which continue using the deprecated macros to either ignore the -Wformat-extra-args warning this causes (if they try to pass a message) or to port their tests to the non-deprecated macros.

[heftig]

Something like this, I think:

diff --git c/src/check.h.in i/src/check.h.in
index 56187fb..c6d4eb3 100644
--- c/src/check.h.in
+++ i/src/check.h.in
@@ -466,29 +466,32 @@ static void __testname ## _fn (int _i CK_ATTRIBUTE_UNUSED)
  *
  * This call is deprecated.
  */
-#define fail_unless ck_assert_msg
+#define fail_unless(expr, ...) \
+  (expr) ? \
+     _mark_point(__FILE__, __LINE__) : \
+     _ck_assert_failed(__FILE__, __LINE__, "Assertion '"#expr"' failed" , ## __VA_ARGS__, NULL)
 
 /*
  * Fail the test case if expr is false
  *
  * This call is deprecated.
  *
  * NOTE: The space before the comma sign before ## is essential to be compatible
  * with gcc 2.95.3 and earlier.
  * FIXME: these macros may conflict with C89 if expr is
  * FIXME:   strcmp (str1, str2) due to excessive string length.
  */
 #define fail_if(expr, ...)\
   (expr) ? \
-     _ck_assert_failed(__FILE__, __LINE__, "Failure '"#expr"' occurred" , ## __VA_ARGS__) \
+     _ck_assert_failed(__FILE__, __LINE__, "Failure '"#expr"' occurred" , ## __VA_ARGS__, NULL) \
      : _mark_point(__FILE__, __LINE__)
 
 /*
  * Fail the test
  *
  * This call is deprecated.
  */
-#define fail ck_abort_msg
+#define fail(...) _ck_assert_failed(__FILE__, __LINE__, "Failed" , ## __VA_ARGS__, NULL)
 
 /*
  * This is called whenever an assertion fails.

[brarcher]

@heftig, that is a pretty straight-forward idea, thanks for it. That should work. I'll pull that change in.

[brarcher]

This should now be resolved by #298

[buscher]

Thanks for the fix! :)

Will there be a 0.15.2 release for this?
And are there some (more or less) concrete plans to remove the deprecated fail_* ? For example for 0.16? 0.20? Expected date?

[brarcher]

I'll be cutting a release with just the fix in a few days. That will be 0.15.2.

I do not currently have a plan for removing the deprecate APIs. With #298 there now may be a warning when using the fail* APIs. Otherwise, the APIs continue to work. If the fail* APIs are to be removed, I'll research what a good plan for doing so looks like, communicate it, and give lots of warning.