[BUG] Signed to unsigned conversion warnings in bounds checking.

[gregmarr]

Describe the bug
warning: implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long')

To Reproduce
Steps to reproduce the behavior:

1. Sample code - distilled down to minimal essentials please

https://cpp2.godbolt.org/z/Wh7jTczzh

main: () = {
    v: std::vector = ( 1, 2, 3, 4, 5 );
    v[6] = 0;
}

Can avoid the warning by using v[6u].

2. Command lines including which C++ compiler you are using
   -std=c++23 -pedantic-errors -Wall -Wextra -Wconversion -Werror=unused-result -Werror=unused-value

3. Expected result - what you expected to happen

Clean compile, and out of bounds access error when executed.

4. Actual result/error

raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:467:30: warning: implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long') [-Wsign-conversion]
  467 |     return CPP2_FORWARD(x) [ CPP2_FORWARD(arg) ];
      |            ~~~~~~~~~~~~~~~   ^~~~~~~~~~~~~~~~~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:271:37: note: expanded from macro 'CPP2_FORWARD'
  271 | #define CPP2_FORWARD(x)             std::forward<decltype(x)>(x)
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/main.cpp:19:11: note: in instantiation of function template specialization 'cpp2::assert_in_bounds<std::vector<int>, int>' requested here
   19 |     cpp2::assert_in_bounds(std::move(v), 6) = 0;
      |           ^
1 warning generated.

Additional context
Investigating #798 File name and line number when the application terminates

[JohelEGP]

4. Expected result - what you expected to happen

Clean compile, and out of bounds access error when executed.

I disagree.
I like that I get a reminder that 6 should have been unsigned.
And although you get the warning, you still get the contract failure: https://cpp2.godbolt.org/z/db6Waze1z.

In file included from /app/build/main.cpp:6:
In file included from /app/cpp2util.h:1:
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:467:30: warning: implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long') [-Wsign-conversion]
  467 |     return CPP2_FORWARD(x) [ CPP2_FORWARD(arg) ];
      |            ~~~~~~~~~~~~~~~   ^~~~~~~~~~~~~~~~~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:271:37: note: expanded from macro 'CPP2_FORWARD'
  271 | #define CPP2_FORWARD(x)             std::forward<decltype(x)>(x)
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
build/main.cpp:19:11: note: in instantiation of function template specialization 'cpp2::assert_in_bounds<std::vector<int>, int>' requested here
   19 |     cpp2::assert_in_bounds(std::move(v), 6) = 0;
      |           ^
1 warning generated.
[5/5] Linking CXX executable main
Program returned: 139
Bounds safety violation: out of bounds access attempt detected
libc++abi: terminating
Program terminated with signal: SIGSEGV

[realgdman]

Won't it be inconvenient on big codebases? For example cppfront itself has many [1]

[JohelEGP]

Just because there's guaranteed runtime safety
doesn't mean the compile-time warnings lose their value.
If you think so, you can just opt-out of such warnings.

[gregmarr]

I disagree.
I like that I get a reminder that 6 should have been unsigned.

I don't see a need for obviously positive integer literals to be manually marked as unsigned just to avoid this warning.

[JohelEGP]

That makes sense.
I know the heuristics on such warnings can vary widely between Clang and GCC.
Maybe you should try reporting to them.

[gregmarr]

This is not a warning in Cpp1. It shouldn't result in warnings in the support scaffolding in Cpp2.

    std::vector v {1, 2, 3, 4, 5}; 
    v[0] = 0;

There's nothing to report to Clang or GCC here. The issue is entirely due to the addition of the assert_in_bounds wrapper. It is already doing checks for signed vs unsigned and converting to unsigned to check the range, so it should do what it can to make sure that it doesn't add a warning. It may be that this isn't feasible in current Cpp1 to determine whether it should pass signed or unsigned, since not every operator[] takes unsigned. Maybe it requires warning suppression around the x[arg] call, which would be a bit ugly, but it's something that the Cpp2 support functions should strive to do.

[JohelEGP]

You're right.
In Cpp1, that doesn't warn: https://compiler-explorer.com/z/r98dd1Gvj.

It seems that for assert_in_bounds to also not warn
it needs to do the same thing Cppfront does for is expressions.
That is, raise the index to a template argument: https://compiler-explorer.com/z/nTqnY6vWM.

It is already doing checks for signed vs unsigned and converting to unsigned to check the range

Is it?

[gregmarr]

It is already doing checks for signed vs unsigned and converting to unsigned to check the range
Is it?

Yes, it is.

cppfront/include/cpp2util.h

Lines 459 to 468 in 083c8a0

	auto assert_in_bounds(auto&& x, auto&& arg CPP2_SOURCE_LOCATION_PARAM_WITH_DEFAULT) -> decltype(auto)
	requires (std::is_integral_v<CPP2_TYPEOF(arg)> &&
	requires { std::size(x); std::ssize(x); x[arg]; std::begin(x) + 2; })
	{
	Bounds.expects(0 <= arg && arg < [&]() -> auto {
	if constexpr (std::is_signed_v<CPP2_TYPEOF(arg)>) { return std::ssize(x); }
	else { return std::size(x); }
	}(), "out of bounds access attempt detected" CPP2_SOURCE_LOCATION_ARG);
	return CPP2_FORWARD(x) [ CPP2_FORWARD(arg) ];
	}

It seems that for assert_in_bounds to also not warn
it needs to do the same thing Cppfront does for is expressions.
That is, raise the index to a template argument: https://compiler-explorer.com/z/nTqnY6vWM.

I was able to fix it for operator[](std::size_t index) but then it warns for a class with operator[](std::ptrdiff_t index).

The issue is that in order to prevent a possible conversion warning, it needs to make sure that x[arg] is called with arg being exactly the type that operator[] expects, and that could be a template or be overloaded, so it gets very messy.

auto assert_in_bounds(auto&& x, auto&& arg ...
     return CPP2_FORWARD(x) [ CPP2_FORWARD(arg) ];

This doesn't change the issue above at all, but since arg is guaranteed to be an integral type, I don't think the && and CPP2_FORWARD() are needed.

[JohelEGP]

That was introduced by #551.
Was it a wrong move to use the same signedness of the argument?

[gregmarr]

I think that commit is fine. It resolved the signedness compare issue in the contract. There is still a signedness forwarding issue however in the body.

It could be that this wasn't triggered by your CE setup at the time. I found it using one of your most recent magnificent 4-pane configs that transpiles and then compiles and runs.

[JohelEGP]

It seems that for assert_in_bounds to also not warn
it needs to do the same thing Cppfront does for is expressions.
That is, raise the index to a template argument: https://compiler-explorer.com/z/nTqnY6vWM.

I was able to fix it for operator[](std::size_t index) but then it warns for a class with operator[](std::ptrdiff_t index).

That doesn't happen with a template argument: https://cpp2.godbolt.org/z/qW45c4vP9.

[gregmarr]

That doesn't happen with a template argument: https://cpp2.godbolt.org/z/qW45c4vP9.

It also doesn't work with variables, only literals. It might be enough to fix this only for literals, not sure.

[JohelEGP]

The fix for variables is #799 (comment).

[gregmarr]

Yeah, that's what I meant by "enough to fix this only for literals".