Repository navigation
[BUG] No type safety when comparing two incompatible variables #817
Description
Activity
@MaxSagebaum Is this what you meant with #810 (comment)?
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; | ~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~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::formatand 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, andstd::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
.cpp2source attribution though...Reacted by Johel Ernesto Guerrero PeñaOK, I've found a solution that makes the original error message a lot better... I can stick a
requires requireson thecpp2::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
.cpp2line
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 thestd::comparisons that didn't work (becauses << "\n"isn't a valid thing to do to astd::stringobjects).@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.
- added a commit that references this issue
on Dec 5, 2023 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 becausea < bwill always be well-formed regardless ofaandb.Can you give an example that shows the problem, that we can track with a new issue?
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>()); }- added a commit that references this issue
on Feb 1, 2024
Describe the bug
No type safety when comparing two variables
To Reproduce
Steps to reproduce the behavior:
cppfront -p main.cpp2 && g++ main.cpp -std=c++23 -I/root/cppfront/include/ -o mainSome kind of error message saying you can't compare incompatible types
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.