[BUG] No type safety when comparing two incompatible variables

[alexgb0]

Describe the bug
No type safety when comparing two variables

To Reproduce
Steps to reproduce the behavior:

1. Sample code - distilled down to minimal essentials please

main: () -> int = {
	number: int = 123;
	string: std::string = "test";

	std::cout << number > string;
}

3. Command lines, including which C++ compiler you are using

cppfront -p main.cpp2 &&
g++ main.cpp -std=c++23 -I/root/cppfront/include/ -o main

2. Expected result - what you expected to happen
   Some kind of error message saying you can't compare incompatible types
3. Actual result/error

main.cpp:22:19:   required from here
/root/cppfront/include/cpp2util.h:1808:28: error: no match for ‘operator>’ (operand types are ‘std::basic_ostream<char>’ and ‘std::__cxx11::basic_string<char>’)
 1808 |     return CPP2_FORWARD(t) > CPP2_FORWARD(u);
          |                                              ^

Additional context
I know it is a stupid problem, but I didn't pay attention and I have to admit that it took me some time to realize where was the problem.

[JohelEGP]

@MaxSagebaum Is this what you meant with #810 (comment)?

[JohelEGP]

That's the same error message you get in Cpp1: https://cpp1.godbolt.org/z/q4a3aTfG6.
This is the Cpp2 lowered to Cpp1: https://cpp1.godbolt.org/z/KdMKx1nEf.
And if cmp_greater was constrained: https://cpp1.godbolt.org/z/bqeYaxxsd.

[JohelEGP]

Sorry, I was wrong.
If I remove the parentheses that is added when lowering to Cpp1,
Clang's outputs makes it clear: https://cpp1.godbolt.org/z/f3s9caE57.

<source>:8:22: warning: overloaded operator << has higher precedence than comparison operator [-Woverloaded-shift-op-parentheses]
    8 |         std::cout << number > string;
      |         ~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~
<source>:8:12: note: place parentheses around the '<<' expression to silence this warning
    8 |         std::cout << number > string;
      |                   ^
      |         (                  )
<source>:8:22: note: place parentheses around comparison expression to evaluate it first
    8 |         std::cout << number > string;
      |                             ^       
      |                      (              )
<source>:8:22: error: invalid operands to binary expression ('ostream' and 'std::string' (aka 'basic_string<char>'))
    8 |         std::cout << number > string;
      |         ~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~

[JohelEGP]

So this is a related discussion: #623.

[hsutter]

Yes, I agree this is annoying.

C and C++ << and >> rightly have high precedence because of their built-in meaning as shift operators, where you want the high precedence. But then C++ allowed operator overloading, and particularly in the 1990s (and to a lesser extent today) there was a fashion of overusing of operator overloading. We now know that operator overloading works best when the overloaded operators closely follow the built-in meanings, which avoids the problem that when you do overload them you of course can't change their precedence. We now know that a poster child "bad" example was overloading << and >> for stream I/O, because for that use the operators would want low precedence. Yet it became popular, and it continues to cause problems like this.

For Cpp2, I briefly considered changing << and >> to be lower precedence because their overwhelmingly common use is for streaming... but that would break code that uses them for their original built-in purpose, and be a compatibility issue.

For Cpp2, and all C++ generally including both Cpp1 and Cpp2, I think the best solution is to encourage more use of C++20 std::format and Cpp2 string interpolation, which prevent << chains and naturally put the comparison into a function argument grammar where the precedence is clear:

main: () -> int = {
    n: int = 123;
    s: std::string = "test";

    //  1. Classic << iostream << chaining << style -- needs ( ) around the comparison
    //std::cout << "n>s evaluates to " << n>s << "\n";

    //  2. C++20 format style (will be even nicer in C++26)
    //std::cout << std::vformat("n>s evaluates to {}\n", n>s);

    std::cout << "n>s evaluates to (n>s)$\n";
}

This gives better diagnostics, by avoiding the << precedence issue and leaving only the > type mismatch issue.

Now all the compilers do a lot better -- all compilers still spit a LOT of diagnostics for this because they list all the possible std:: comparison operators they considered but that failed to match, and std:: has a LOT of overloaded comparison operators. But their first message tells you what's going on:

// First diagnostic from Clang
cppfront/include/cpp2util.h:1808:28: error: invalid operands to binary expression ('int' and 'std::basic_string<char>')
    return CPP2_FORWARD(t) > CPP2_FORWARD(u);
           ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~

// First diagnostic from GCC
cppfront/include/cpp2util.h:1808:28: error: no match for ‘operator>’ (operand types are ‘int’ and ‘std::__cxx11::basic_string<char>’)
 1808 |     return CPP2_FORWARD(t) > CPP2_FORWARD(u);
      |                            ^

// First diagnostic from MSVC (because it's considering reversed operators, but still helpful I think)
cppfront\include\cpp2util.h(1808): error C2678: binary '>': no operator found which takes a left-hand operand of type '_T1' (or there is no acceptable conversion)
        with
        [
            _T1=std::basic_string<char,std::char_traits<char>,std::allocator<char>>
        ]

I do wish it was easier to find the original .cpp2 source attribution though...

[hsutter]

OK, I've found a solution that makes the original error message a lot better... I can stick a requires requires on the cpp2::cmp_* comparison functions to require that the requested comparison is valid. That will handle a lot of common cases, including the original example code at top, with a single readable diagnostic.

Now the code at top gets:

• a nicer error message
• and in most cases(*) just one! (not a spew of messages)
• that points back to the original .cpp2 line

The original code at top was:

main: () -> int = {
    number: int = 123;
    string: std::string = "test";

    std::cout << number > string;
}

And with the commit I just pushed, this now gets these error messages:

//== Clang (entire output) ======================================================

In file included from demo.cpp:7:
/mnt/c/GitHub/cppfront/include/cpp2util.h:1843:5: error: static_assert failed due
 to requirement 'program_violates_type_safety_guarantee<std::basic_ostream<char, 
 std::char_traits<char>> &, std::basic_string<char, std::char_traits<char>, std::
 allocator<char>> &&>' "attempted to compare '>' for incompatible types"
    static_assert(
    ^
demo.cpp:22:11: note: in instantiation of function template specialization 'cpp2:
:cmp_greater<std::basic_ostream<char> &, std::basic_string<char>>' requested here
    cpp2::cmp_greater(std::cout << std::move(number),std::move(string));
          ^


//== GCC (entire output) ========================================================

In file included from demo.cpp:7:
/mnt/c/GitHub/cppfront/include/cpp2util.h: In instantiation of ‘constexpr decltyp
e(auto) cpp2::cmp_greater(auto:81&&, auto:82&&) [with auto:81 = std::basic_ostrea
m<char>&; auto:82 = std::__cxx11::basic_string<char>]’:
demo.cpp:22:71:   required from here
/mnt/c/GitHub/cppfront/include/cpp2util.h:1844:9: error: static assertion failed:
 attempted to compare '>' for incompatible types
 1844 |         program_violates_type_safety_guarantee<decltype(t), decltype(u)>,
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


//== MSVC (entire output) =======================================================

cppfront\include\cpp2util.h(1844): error C2338: static_assert failed: 'attempted 
to compare '>' for incompatible types'
demo.cpp(22): note: see reference to function template instantiation 'decltype(au
to) cpp2::cmp_greater<std::basic_ostream<char,std::char_traits<char>>&,std::basic
_string<char,std::char_traits<char>,std::allocator<char>>>(_T0,_T1 &&)' being com
piled
        with
        [
            _T0=std::basic_ostream<char,std::char_traits<char>> &,
            _T1=std::basic_string<char,std::char_traits<char>,std::allocator<char>>
        ]

Much better. Thanks @alexgb0 for pointing this out, and @JohelEGP for the discussion.

(*) There are other examples, such as std::cout << n>s << "\n";, that will still have the usual C++ error message spew mentioned above listing all the std:: comparisons that didn't work (because s << "\n" isn't a valid thing to do to a std::string object s).

[MaxSagebaum]

@MaxSagebaum Is this what you meant with #810 (comment)?

Kind of. But the list of the possible operators is a cpp1 problem. The solution of Herb the way to go I think.

[JohelEGP]

The solution with commit 6b48a72 adds more uses of nonesuch.
These are going to come back biting often when Cpp2 has requires expressions.
That's because a < b will always be well-formed regardless of a and b.

[hsutter]

The solution with commit 6b48a72 adds more uses of nonesuch.
These are going to come back biting often when Cpp2 has requires expressions.
That's because a < b will always be well-formed regardless of a and b.

Can you give an example that shows the problem, that we can track with a new issue?

[JohelEGP]

For starters (https://cpp2.godbolt.org/z/7r14PWh43):

f: <T, U> (_: T, _: U)
  requires T() < U()
= { }
main: () = {
  // Hard `error: atomic constraint must be of type 'bool' (found 'const nonesuch_')` 👎.
  f(std::integral_constant<int, 4>(),
    std::array<int, 2>());

  // SFINAE-rejected 👍.
  // f(std::integral_constant<int, 4>(),
  //   std::integral_constant<int, 2>());
}

That might not work so well, so here's another example (https://cpp2.godbolt.org/z/zfsjYfobe):

#define REQUIRES_EXPRESSION(X) requires { X }
#define SIMPLE_REQUIREMENT(X) X;
f: <T, U> (_: T, _: U)
  requires REQUIRES_EXPRESSION(SIMPLE_REQUIREMENT(T() < U()))
= { }
main: () = {
  // Static assertion instead of SFINAE-rejection 👎.
  f(std::integral_constant<int, 4>(),
    std::array<int, 2>());

  // Accepted 👍.
  f(std::integral_constant<int, 4>(),
    std::integral_constant<int, 2>());
}