[libcxx] [test] Fix invocable tests on Windows

[mstorsjo]

Summary

MSVC had a bug regarding preferring intergral conversions over
floating conversions. This is fixed in MSVC 19.28 and newer. Clang in
MSVC mode so far only mimics the old, buggy behaviour, but will
hopefully soon be fixed to comply with the new behaviour too
(see https://reviews.llvm.org/D99663).

Make the negative test to use a distinctly different type,
leaving checks for compiler specific bugs out of the libcxx test.

[curdeius]

That's strange. I guess you use clang-cl (with some automatically set -fms-compatibility), right?
It seems as if it uses compatibility for MSVC before version 19.28 (where this particular bug seems to have been fixed).
Cf. https://godbolt.org/z/3bsEaPToc.
Should these #ifndefs be precise about the version? Unfortunately I don't know if clang-cl correctly sets _MSC_VER.

[mstorsjo]

In D99641#2660761, @curdeius wrote:

That's strange. I guess you use clang-cl (with some automatically set -fms-compatibility), right?
It seems as if it uses compatibility for MSVC before version 19.28 (where this particular bug seems to have been fixed).
Cf. https://godbolt.org/z/3bsEaPToc.
Should these #ifndefs be precise about the version? Unfortunately I don't know if clang-cl correctly sets _MSC_VER.

Oh, interesting, thanks for digging into this!

clang-cl does try to set _MSC_VER (it can be controlled via a number of mechanisms, see https://github.com/llvm/llvm-project/blob/llvmorg-11.1.0/clang/lib/Driver/ToolChains/MSVC.cpp#L1340-L1355, it first checks for the options -fmsc-version and -fms-compatibility-version, then tries to see if there's a version number specified as part of the triple, then tries to locate a cl.exe in the PATH and extract a version number from it, and then finally defaults to 19.11.

However, for this particular issue here, Clang doesn't know about this change in behaviour yet, so regardless of targeted MSVC version, Clang mimics the old buggy behaviour.

I guess I could at least amend the ifdef with a comment mentioning this detail.

[mstorsjo]

Added comments about the MSVC and Clang bugs that this works around.

[curdeius]

LGTM. Thanks for adding the comment.
Hopefully the Windows agent will be added soon to the CI!

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

232

So, if we wanted to be precise (and if clang-cl correctly mimicked 19.28+ behaviour), it would be then #if !(defined(_MSC_VER) && defined(__clang__)) || !(defined(_MSC_VER) && (_MSC_VER < 1928)), right?

So to:

1. turn it off for clang-cl
2. turn it off for MSVC before 19.28

But I'm not sure we want that. Just noting.

233

Nit. Here and in the other test.

[• Quuxplusone]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

235

This is not "comments about the MSVC and Clang bugs that this works around": this comment doesn't explain the bug at all! It just says that the code doesn't work (for some reason, presumably a bug, but we don't say what the bug is or why).

We either need to dig in and diagnose why MSVC is treating this overload resolution as unambiguous...

...or, vastly simpler IMO, notice that the point of this test case has nothing to do with long and int! The point of this test case is to verify that overload resolution ambiguity makes the invocation fail. Apparently "long, passed to an overload set of int and double" is not an ambiguity on all supported compilers. Therefore, let's use long long instead. (Which you do on line 238.) Cool, that fixes the test issue. So delete lines 232–237; they are no longer needed.

Even better, btw, would be to use some user-defined types whose conversions we control; e.g.

struct multiple_overloads {
    struct A {};
    struct B { B(int); };
    struct AB : A, B {};
    struct O {};
    void operator()(A) {};
    void operator()(B) {};
};
static_assert(std::invocable<multiple_overloads, multiple_overloads::A>);
static_assert(std::invocable<multiple_overloads, multiple_overloads::B>);
static_assert(std::invocable<multiple_overloads, int>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::AB>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::O>);

These 13 lines accomplish the same stuff as the original 15, without "cross-polluting" with the (apparently non-portable) details of arithmetic conversions.

[mstorsjo]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

235

Thanks, that sounds sensible to me. @cjdb - which way forward do you prefer?

[curdeius]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

235

FYI, it is a bug in MSVC fixed in version 19.28 (2019 Update 8).
Precisely, MSVC before this version preferred integral-conversion over floating-to-integral conversion (and so far clang-cl implements the old behaviour only, cf. https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOverload.cpp#L4109-L4125). I have a local patch for that already.

I agree to simplify this and just use long long (or user-defined types) instead of long.

[cjdb]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

235

I would prefer that we keep the existing test and guard it against the offending versions. Part of the way this test has been designed is to flush out strange compiler bugs, so this test has done its job. Per tab:over.ics.scs, this is an ambiguity, and compilers that don't follow this table are buggy because long is simultaneously being converted to int and double, and those conversions are equally ranked.

Here's what I'd do:

• in this patch:
  • fix up the comment
  • add include guards around the offending lines for the non-conforming compiler versions
  • add a test for long long or short (or both)
• file a bug with the respective compiler teams and ask them to adjust this patch when they make the fix (or at least notify you to do so)

[mstorsjo]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

235

I can make the comment a bit more precise with wordings from @curdeius comments.

For guarding the exact version that has got the problem, it gets a bit messy, as we have MSVC proper having the bug in older versions, and Clang imitating MSVC currently always having the bug, and in the future will have it conditionally depending on what version of MSVC it impersonates:
#if !defined(_MSC_VER) || ((!defined(__clang__) || __clang_major__ >= 13) && _MSC_VER >= 1928)
(That becomes valid once the Clang fix lands - I'd assume it gets fixed by the time that 13 branches off.) That doesn't take Apple Clang versions into account though - they can technically be used to target MSVC as well (although I guess it's rather rare).

[cjdb]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

235

For guarding the exact version that has got the problem, it gets a bit messy, as we have MSVC proper having the bug in older versions, and Clang imitating MSVC currently always having the bug

I'd suggest making this a named macro of sorts.

That doesn't take Apple Clang versions into account though

This test doesn't run on AppleClang right now, so we're in the clear for a bit (I'd rather deal with that when the problem arises).

[mstorsjo]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

235

For guarding the exact version that has got the problem, it gets a bit messy, as we have MSVC proper having the bug in older versions, and Clang imitating MSVC currently always having the bug

I'd suggest making this a named macro of sorts.

Hmm, like #if (mess above) #define MSVC_CONVERSION_PREFERRAL_BUG #endif and then using #ifndef MSVC_CONVERSION_PREFERRAL_BUG around the tests? And have the definition at the top of these test files, or somewhere else?

That doesn't take Apple Clang versions into account though

This test doesn't run on AppleClang right now, so we're in the clear for a bit

Well I wouldn't expect us to ever be running CI with Apple Clang building code for windows targets. One could be using that for personal development though, but I think it's fine to ignore that case for our tests for now anyway.

[• Quuxplusone]

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

232–237

This is becoming insane. libcxx/test/std/concepts/ is not the right place to be testing MSVC compatibility features in clang-cl's overload resolution logic. Take it to clang/test/, please — and take the suggested edit here.

237–239

Take the suggested edit, please.

[curdeius]

Just to be clear. I agree with Arthur that concept test suite is not the place for testing compiler compatibility quirks.
And, the approach he suggested is superior to ifdefing.

Concerning what I wrote about the possible "precise" ifdef condition, it was just a note, and I really don't want to see such quirks in the code.

And, FYI, this particular clang-cl bug is going to be fixed by D99663 (if accepted of course).

[mstorsjo]

Went with the simplest way forward, making the test less ambiguous, leaving testing for compiler correctness to some other testsuite.

[curdeius]

LGTM now.

[cjdb]

In D99641#2663092, @mstorsjo wrote:

Went with the simplest way forward, making the test less ambiguous, leaving testing for compiler correctness to some other testsuite.

Please do not do this, or at least wait for @ldionne to approve before submitting. Louis approved this test, and I believe he was aware of my intentions with the original comprehensiveness.
There's also the added problem that we're removing coverage in libc++ tests without evidence that it's been added elsewhere, but this is secondary.

[ldionne]

In the test suite, we generally try to stay divorced from compiler-specific behavior if we can, the reason being that things otherwise become quite complicated. Furthermore, we're trying to test the library after all, not the language implementation (since there's another test suite for that). I do agree that the boundary between those two things (low-level library stuff and compiler stuff) is sometimes vague, for example when testing type traits that are all implemented as intrinsics.

However, in this instance, it seems to me that we're really trying to test whether the pure library implementation does the right thing. If I had written those tests from the start, I might have gone for something like what Arthur suggested above, which IMO more clearly indicates what we're trying to test:

struct multiple_overloads {
    struct A {};
    struct B { B(int); };
    struct AB : A, B {};
    struct O {};
    void operator()(A) {};
    void operator()(B) {};
};
static_assert(std::invocable<multiple_overloads, multiple_overloads::A>);
static_assert(std::invocable<multiple_overloads, multiple_overloads::B>);
static_assert(std::invocable<multiple_overloads, int>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::AB>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::O>);

On the other hand, I also agree with @cjdb that this test has been useful as-is: it has taught us that MSVC (and Clang in MS mode) doesn't work properly w.r.t. overload resolution. However, I believe the better location for testing that overload resolution bug would be to add (roughly) the same test to the Clang test suite (which it seems is being done). By doing that, we do weaken the libc++ test suite w.r.t. language conformance, but we don't weaken it w.r.t. library conformance.

That's just my .02, but in all cases I would rather not just hide the issue by using long long.

[mstorsjo]

In D99641#2665097, @ldionne wrote:

In the test suite, we generally try to stay divorced from compiler-specific behavior if we can, the reason being that things otherwise become quite complicated. Furthermore, we're trying to test the library after all, not the language implementation (since there's another test suite for that). I do agree that the boundary between those two things (low-level library stuff and compiler stuff) is sometimes vague, for example when testing type traits that are all implemented as intrinsics.

However, in this instance, it seems to me that we're really trying to test whether the pure library implementation does the right thing. If I had written those tests from the start, I might have gone for something like what Arthur suggested above, which IMO more clearly indicates what we're trying to test:

struct multiple_overloads {
    struct A {};
    struct B { B(int); };
    struct AB : A, B {};
    struct O {};
    void operator()(A) {};
    void operator()(B) {};
};
static_assert(std::invocable<multiple_overloads, multiple_overloads::A>);
static_assert(std::invocable<multiple_overloads, multiple_overloads::B>);
static_assert(std::invocable<multiple_overloads, int>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::AB>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::O>);

On the other hand, I also agree with @cjdb that this test has been useful as-is: it has taught us that MSVC (and Clang in MS mode) doesn't work properly w.r.t. overload resolution. However, I believe the better location for testing that overload resolution bug would be to add (roughly) the same test to the Clang test suite (which it seems is being done). By doing that, we do weaken the libc++ test suite w.r.t. language conformance, but we don't weaken it w.r.t. library conformance.

That's just my .02, but in all cases I would rather not just hide the issue by using long long.

Thanks for weighing in!

@cjdb - do you want to commandeer this review and finish it in the form you prefer, based on the comments above?

[cjdb]

In D99641#2665140, @mstorsjo wrote:

In D99641#2665097, @ldionne wrote:

In the test suite, we generally try to stay divorced from compiler-specific behavior if we can, the reason being that things otherwise become quite complicated. Furthermore, we're trying to test the library after all, not the language implementation (since there's another test suite for that). I do agree that the boundary between those two things (low-level library stuff and compiler stuff) is sometimes vague, for example when testing type traits that are all implemented as intrinsics.

However, in this instance, it seems to me that we're really trying to test whether the pure library implementation does the right thing. If I had written those tests from the start, I might have gone for something like what Arthur suggested above, which IMO more clearly indicates what we're trying to test:

struct multiple_overloads {
    struct A {};
    struct B { B(int); };
    struct AB : A, B {};
    struct O {};
    void operator()(A) {};
    void operator()(B) {};
};
static_assert(std::invocable<multiple_overloads, multiple_overloads::A>);
static_assert(std::invocable<multiple_overloads, multiple_overloads::B>);
static_assert(std::invocable<multiple_overloads, int>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::AB>);
static_assert(!std::invocable<multiple_overloads, multiple_overloads::O>);

On the other hand, I also agree with @cjdb that this test has been useful as-is: it has taught us that MSVC (and Clang in MS mode) doesn't work properly w.r.t. overload resolution. However, I believe the better location for testing that overload resolution bug would be to add (roughly) the same test to the Clang test suite (which it seems is being done). By doing that, we do weaken the libc++ test suite w.r.t. language conformance, but we don't weaken it w.r.t. library conformance.

That's just my .02, but in all cases I would rather not just hide the issue by using long long.

Thanks for weighing in!

@cjdb - do you want to commandeer this review and finish it in the form you prefer, based on the comments above?

I think @ldionne's comments offer direction for this patch and there's terribly little I can achieve by commandeering this patch (as opposed to just reviewing it). I think the suggested code can be applied verbatim.
I suggest cross-referencing this review in the Clang review that Louis alluded to, so that we can make sure the coverage isn't lost; only transferred.

Following that, my LGTM is trivial.

[curdeius]

FYI, D99663 has landed and clang-cl-13 (with -fms-compatibility-version=19.28 or higher) will match MSVC's behaviour (being now standard conformant w.r.t. int/float conversion and overload).

[ldionne]

Can we please change from long long to something like Arthur's suggestion?

[mstorsjo]

In D99641#2669505, @ldionne wrote:

Can we please change from long long to something like Arthur's suggestion?

@cjdb Can you pick up on that, or do you want me to do a crude attempt at it?

libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp

 struct multiple_overloads {
   bool operator()();
   void operator()(int);
   int operator()(double);
 };
 static_assert(std::invocable<multiple_overloads&>);
 static_assert(std::invocable<multiple_overloads&, short>);
 static_assert(std::invocable<multiple_overloads&, int>);
-static_assert(!std::invocable<multiple_overloads&, long>);
+static_assert(!std::invocable<multiple_overloads&, long long>);
 static_assert(std::invocable<multiple_overloads&, double>);
 static_assert(std::invocable<multiple_overloads&, float>);
 static_assert(std::invocable<multiple_overloads&, short&>);
 static_assert(std::invocable<multiple_overloads&, int&>);
-static_assert(!std::invocable<multiple_overloads&, long&>);
+static_assert(!std::invocable<multiple_overloads&, long long>);
 static_assert(std::invocable<multiple_overloads&, float&>);
 static_assert(std::invocable<multiple_overloads&, double&>);
 } // namespace function_objects
 
 namespace pointer_to_member_functions {
 // clang-format off
   template<class Member, class T, class... Args>
   [[nodiscard]] constexpr bool check_member_is_invocable()
   {
     constexpr bool result = std::invocable<Member, T, Args...>;

libcxx/test/std/concepts/concepts.callable/concept.regularinvocable/regular_invocable.pass.cpp

 struct multiple_overloads {
   bool operator()();
   void operator()(int);
   int operator()(double);
 };
 static_assert(std::regular_invocable<multiple_overloads&>);
 static_assert(std::regular_invocable<multiple_overloads&, short>);
 static_assert(std::regular_invocable<multiple_overloads&, int>);
-static_assert(!std::regular_invocable<multiple_overloads&, long>);
+static_assert(!std::regular_invocable<multiple_overloads&, long long>);
 static_assert(std::regular_invocable<multiple_overloads&, double>);
 static_assert(std::regular_invocable<multiple_overloads&, float>);
 static_assert(std::regular_invocable<multiple_overloads&, short&>);
 static_assert(std::regular_invocable<multiple_overloads&, int&>);
-static_assert(!std::regular_invocable<multiple_overloads&, long&>);
+static_assert(!std::regular_invocable<multiple_overloads&, long long&>);
 static_assert(std::regular_invocable<multiple_overloads&, float&>);
 static_assert(std::regular_invocable<multiple_overloads&, double&>);
 } // namespace function_objects
 
 namespace pointer_to_member_functions {
 // clang-format off
   template<class Member, class T, class... Args>
   [[nodiscard]] constexpr bool check_member_is_invocable()