-
Notifications
You must be signed in to change notification settings - Fork 261
[SUGGESTION][FIX] Add narrowing 'as' cast and static assert failure instead of nonesuch… #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SUGGESTION][FIX] Add narrowing 'as' cast and static assert failure instead of nonesuch… #106
Conversation
v1 := max_uint8 as std::int8_t; // throws narrowing_error That seems like a departure from https://wg21.link/P2392, which matches
I think you can confirm this with Circle. |
Thanks! General timing note (maybe I should paste/link this in a few other PRs/issues if this isn't visible enough): My responses will be delayed because of the Kona meeting next week, which will be the first face-to-face ISO C++ meeting in nearly three years, and the week before and after the meeting are generally also consumed with travel and committee-related work. I plan to be back to a normal schedule after that and will be catching up with PRs and issues then. Thanks! |
But quick ack on this thread: My current thought is to require
Some considerations:
That currently tilts things in favor of (2) in my mind, which is the direction of this PR, but I'd be happy to see usability evidence/examples arguing for any these options as we consider this PR. @JohelEGP P2392 is a living document that will be updated based on this and other experience (and committee feedback) and can be expected to be a little out of sync sometimes. But for |
Thanks for the feedback. I assumed that you are preparing for the Kona meeting and I am not expecting that you will process these PRs at the moment. I hope I will be able to join one of the committee meetings in the future. Regarding this PR. I like the cleanliness of the x narrowed_to T; // or something that you can easily read The good thing is that we can have args : std::span<Arg> = (argv, argc narrowed_to std::size_t);
c : std::ifstream = args.back(); |
Maybe |
We can brainstorm on the keyword name to make it clear. Let's first agree on the general idea of how to tackle that cases. |
I have a broader language suggestion that may be useful in more cases than just narrow casts: Maybe we could indicate whether we want to throw on mismatch, static assert, or return an optional instead by adding The motivation for having built-in defaults for matching when no pattern applies is easily delegating control over what happens to the caller. In the context of this problem, this would mean we just match on cases where there's no data loss, and have the caller decide if they want an optional, a thrown exception, or a static assertion in case there's data loss by selecting the type of match in the calling site. |
I think it makes sense for v1 := max_uint8 as std::int8_t; // throws narrowing_error to use
You're right. I think it actually matches
|
Thanks to @feature-engineer I decided to play with exclamation marks and question marks. I don't like pass(x as std::uint16_t); I decided to try So if you want to be strict about casting you can write code adding exclamation marks after the type: main: () -> int = {
max_uint8 := std::numeric_limits<std::uint8_t>::max();
max_int8 := std::numeric_limits<std::int8_t>::max();
max_double := std::numeric_limits<double>::max();
max_float := std::numeric_limits<float>::max();
v := max_uint8 as std::uint16_t!; // OK
v1 := max_uint8 as std::int8_t!; // compile-time error
v2 := -1 as std::uint8_t!; // compile-time error
v3 := -1.0 as std::uint8_t!; // compile-time error
v4 := max_double as float!; // compile-time error
v5 := -1.0 as long!; // compile-time error
v6 := -1 as double!; // compile-time error
v7 := max_float as double!; // OK
v8 := max_uint8 as std::uint16_t!; // OK
v9 := max_int8 as std::uint16_t!; // compile-time error
v10 := 10 as std::size_t!; // compile-time error
v11 := max_uint8 as std::int16_t!; // OK
} If you don't want static asserts but you don't want to have exceptions you can use question marks to get main: () -> int = {
max_uint8 := std::numeric_limits<std::uint8_t>::max();
max_int8 := std::numeric_limits<std::int8_t>::max();
max_double := std::numeric_limits<double>::max();
max_float := std::numeric_limits<float>::max();
v := max_uint8 as std::uint16_t?; // OK, 255
v1 := max_uint8 as std::int8_t?; // OK, (empty)
v2 := -1 as std::uint8_t?; // OK, (empty)
v3 := -1.0 as std::uint8_t?; // OK, (empty)
v4 := max_double as float?; // OK, (empty)
v5 := -1.0 as long?; // OK, -1
v6 := -1 as double?; // OK, -1.000000
v7 := max_float as double?; // OK, 340282346638528859811704183484516925440.000000
v8 := max_uint8 as std::uint16_t?; // OK, 255
v9 := max_int8 as std::uint16_t?; // OK, 127
v10 := 10 as std::size_t?; // OK, 10
v11 := max_uint8 as std::int16_t?; // OK, 255
std::cout << "v = (v)$" << std::endl;
std::cout << "v1 = (v1)$" << std::endl;
std::cout << "v2 = (v2)$" << std::endl;
std::cout << "v3 = (v3)$" << std::endl;
std::cout << "v4 = (v4)$" << std::endl;
std::cout << "v5 = (v5)$" << std::endl;
std::cout << "v6 = (v6)$" << std::endl;
std::cout << "v7 = (v7)$" << std::endl;
std::cout << "v8 = (v8)$" << std::endl;
std::cout << "v9 = (v9)$" << std::endl;
std::cout << "v10 = (v10)$" << std::endl;
std::cout << "v11 = (v11)$" << std::endl;
} If you don't want to have Currently, My only doubt is that there is a very subtle difference between these cases: pass(x as std::uint8_t?);
pass(x as std::uint8_t!);
pass(x as std::uint8_t); probably a better option would be to have something like pass(x as_optional std::uint8_t);
pass(x as_strict std::uint8_t);
pass(x as std::uint8_t); or pass(x as optional<std::uint8_t>);
pass(x as strict<std::uint8_t>);
pass(x as std::uint8_t); PS: have you noticed how easy it is to print |
I don't like the |
My understanding of the future of exceptions (per https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0709r4.pdf ) is that |
Ok. Will read that through. |
Continuing my experiments with the syntax I have pushed the new update that allows using two syntaxes: pass(x as std::uint8_t?);
pass(x as std::uint8_t!);
pass(x as std::uint8_t); or pass(x as std::optional<std::uint8_t>);
pass(x as cpp2::strict<std::uint8_t>);
pass(x as std::uint8_t); Taking a look at the possible options and current experiments probably strict mode shall be the default one as it is not narrowing. Possibly narrowing cast shall be emphasized instead of a strict case so maybe we should change it to something similar to: pass(x as std::optional<std::uint8_t>);
pass(x as std::uint8_t); // static_assert failure if possibly dangerous
pass(x as cpp2::narrowing<std::uint8_t>); |
I don't think your assessment is correct - according to https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/n4907.pdf, P0323R12 (std::expected) is going to be included in the language for C++23. It seems that it'd be quite some time before p0709, or P2232, or some other yet to be proposed proposal are going to deprecate std::expected. |
I guess it's just a matter of how forward you look. The paper I linked is clear about what you said:
But that paper is Herb writing 60 pages about how he dislikes adding (with |
3fb4d2f
to
145bc7e
Compare
Hey, very interesting discussion. Wondering why you don't like them? They are directly at the operator that does the action so they seem very clear and upfront to me. to me putting them at the end feels like trying to form normal language sentences which feels like a bit of an illusion: they are operators in a mathematical sense. I kinda find them very beautiful as they are very short and to the point yet very expressive. a bit in contrast to the second syntax proposal you have that seems to reintroduce the noise that these operators reduce. |
Yes, you are right. I did the experiments and came to that conclusion as well. To be honest I am waiting for a feedback from Herb as I don't know if he has something in mind regarding this feature. Probably I can make all these syntaxes working. I am currently waiting to clarify the defaults and how to distinguish other cases from the default behavior. |
One thing I have learned is that we probably want to have default version that is statically checked on compile time. If that is not working than we shall be able to choose if we want to have exception or e.g. optional. |
From my understanding, Ex. when connecting to a web socket, you expect to be returned some kind of a handle object, however you also may expect an error code. IMO error codes fit this use-case more since a network error is an actually expected (although erroneous) result. Normally, you would use a nullable return value or Although, granted, out |
The way I read that proposal I linked, "unexpected errors" as in "programmer errors" should be asserting, rather than throwing exceptions that propagate all the way out and terminate the program there. If you're programmaticaly handling errors, they are by definition errors that you anticipate could happen. Like your socket example. I can't do what I wanted to do (normal control flow), but I can do something about the error. The proposal argues in several places, and I agree, that normal and error control flow in C++ should be separate. I think separable is a better way to call it (as nothing ever stops you from handling errors immediately next to the call site). It has to be that way for constructors, and constructors are central for RAII. And it has to be that way to let you do compositions as simple as chaining methods, That said, the new exceptions mechanism proposed is very much like "using So maybe the best course for cppfront is to enshrine |
I would definitely not want to have some kind of 3d way of handling runtime errors in addition to classic exceptions and Perhaps opening a new issue would be best? @jcanizales |
Personally, I think classic exceptions are better in this case, since they handle RAII and can be used to "catch all non-fatal errors". Ex. in a game engine's editor you could catch all non-fatal exceptions and log the error message, then pause the game and allow the user to fix it without crashing. Assertions are great for debugging, but do not allow you to preform this kind of error handling and may cause resource leaks because of unhandled RAII destructors. |
Classic exceptions are already forbidden in many types of codebases, for the range of issues the paper describes. The point of the proposal is to fix those issues (like When I wrote "programmer error" I meant "the code is buggy" errors. The user can't recover from that, and trying to recover programmatically is problematic because there's no knowing how much of your program state is already corrupted by the time you detect the issue. Assertion libraries like glog usually do print an error message and a stack trace before terminating the program. Chrome survives assertions in tabs without crashing by isolating each in its own OS process. That said, like with all guidance I'm sure there's no shortage of cases in which it's not the best. New exceptions are also better for this than old exceptions in any case. |
I was not thinking about runtime assertions but static assertions that will make your build to fail. No impact on runtime error handling. It should prevent developer from writing potentially buggy code. Additionally it should provide guidance what to do instead. |
I have done more experiments and tests around that topic. I like the idea of having default Currently, it works like that: template< typename C, typename... Ts >
auto as(Ts... ts) -> auto {
static_assert(program_violates_type_safety_guarantee<Ts...>, "safe 'as' cast is not defined");
} I realized that this interfered with the In inspect x -> std::string {
is int = "integer " + std::to_string(x as int);
is std::string = '"' + x as std::string + '"';
is _ = "not an int or string";
}
if (cpp2::is<int>(__expr)) {
if constexpr( requires{ "integer " + std::to_string(cpp2::as<int>(x)); } )
if constexpr( std::is_convertible_v<CPP2_TYPEOF( "integer " + std::to_string( cpp2::as<int>(x) ) ),
std::string> )
return "integer " + std::to_string(cpp2::as<int>(x));
else
return std::string{};
else
return std::string{};
} If TLDR: I have renamed all enum class cast_failure_policy {
static_assertions,
throws
};
// ...
template< typename C, typename X >
auto as(X const& x) -> decltype(auto) {
return internal_as<cast_failure_policy::static_assertions, C>(x);
} cppfront emits statement to the string that is later printed using that string: return_prefix = "{ if constexpr( requires{" + statement + ";} ) if constexpr( std::is_convertible_v<CPP2_TYPEOF((" + statement + "))," + result_type + "> ) return "; In auto internal_statement = replace_all(statement, "cpp2::as<", "cpp2::internal_as<cpp2::cast_failure_policy::throws, "); |
145bc7e
to
11c9d47
Compare
Thanks Filip. I appreciate the Let me take a browse through the latest tonight and either suggest changes or perhaps take some parts and start merging them in a separate commit... more soon... and thanks again, I know we've iterated a lot on this one and you've put a lot of work into it, thanks for your patience as I wrap my head around the various parts being suggested. |
…l `unsafe_narrow` into `cpp2::`
OK, I've taken parts of this in a separate commit linked above... please check that I didn't make mistakes in merging that subset? I should have the literal not-really-narrowing allowed cases (I think?), and this one now does the assertion:
I didn't take the |
OK, I will collect everything I have learned so far. The |
I will explain more to you when I finish my other work. TLDR: it probably, will not be possible to use a static asserted version of I have taken a few steps back, reverted some of the changes, and rechecked it. |
…ntract precondition Signed/unsigned conversions to a not-smaller type are handled as a precondition, and trying to cast from a source value that is in the half of the value space that isn't representable in the target type is flagged as a `Type` safety contract violation
Thanks. After more thought, I think your use cases are pointing out a useful distinction, including especially your Here's how I'm thinking about it:
In the last case I think a dynamic For convenience, here's my current implementation (please point out flaws if I missed anything):
With this, your opening example now works:
And this example works:
And this example is flagged:
In this last case, the default
or, with the
and one nice thing about the Cpp2 contracts system is that this is easy to customize with a violation handler, to integrate with any contract/logging framework a given project is already using. I think I like the above approach because each of the three cases now feels like it's handled in an appropriate way, and we are still safe by default in all cases -- in the first we guarantee safety statically, in the second the programmer has to explicitly opt into an Perhaps this treatment of the third case will take most of the sting out of How does this look? In terms of your longer list of cases:
For
Does that seem to cover the cases now? I think the only ones that are not covered now are integer/floating-point narrowing, which still fail statically by yielding |
Thank you for your patience. I tried to gather all cases I saw regarding arithmetic/literal types. Case 1 x := 1000 as u8; Case 2 out_of_scope := -1.0;
x := out_of_scope as u8; Case 3 x := 1000 as std::optional<u8>; // currently buggy Case 4 x := 1000;
std::cout << inspect (x) -> std::string {
is u8 = cpp2::to_string(x as u8);
is _ = "(no match)";
} << std::endl; Case 5 x := 1000;
if x is int {
print(x as u8);
} Case 6 x := 128;
if x is (in(0,255)) {
print(x as u8);
}
(1) template<typename T>
requires std::is_same_v<T, std::any>
inline auto to_string(T const&) -> std::string {
return "std::any";
} In case of the inline auto to_string(nonesuch_ const&) -> std::string {
return "(nonsuch - as cast failed returning nonsuch)";
} |
Probably I need to add your current solution to the picture. |
TLDR: everything is fine with the Case 6 is something I have yet to find a solution to. |
I have corrected case 3 as it should be marked as ❌ also for cases |
Bug discovered with (1) probably also will give false positives for other types, e.g. v := 10;
x := v as std::vector<double>; |
I have run tests on the latest main. // v1 := max_uint8 as std::int8_t; // Type.expects() failure
// v2 := -1 as std::uint8_t; // static_assert failure
v3 := -1.0 as std::uint8_t; // returns nonsuch, prints std::any
// v4 := max_double as float; // static_assert failure
v5 := -1.0 as long; // returns nonsuch, prints std::any
// v6 := -1 as double; // static_assert failure
v7 := max_float as double; // OK v7 = 340282346638528859811704183484516925440.000000
v8 := max_uint8 as std::uint16_t; // OK v8 = 255
v9 := max_int8 as std::uint16_t; // OK v9 = 127
v10 := 10 as std::size_t; // OK v10 = 10
v11 := "string" as std::string; // OK v11 = string
v12 := 'a' as int; // OK v12 = 97
v13 := 36 as char; // OK v13 = 36 |
To demonstrate a problem with Case 4 on the current main, you can run: main: () -> int = {
x := double(1000);
std::cout << inspect (x) -> std::string {
is float = cpp2::to_string(x as float);
is u8 = cpp2::to_string(x as u8);
is _ = "(no match)";
} << std::endl;
} Expected: print
|
OK, I think the next commit fixes all of these:
This now prints (no match).
This now static_asserts with And it doesn't static-assert if the number is, say, 100.
This now static_asserts with
This now static-asserts with
This now prints "(no match)". The result is the same even if the value is changed to 100, which is correct because it's not a
This now static-asserts with
This correctly static-asserts with |
…ng, pull `unsafe_narrow` into `cpp2::`
…" as contract precondition Signed/unsigned conversions to a not-smaller type are handled as a precondition, and trying to cast from a source value that is in the half of the value space that isn't representable in the target type is flagged as a `Type` safety contract violation
…ases including bad narrowing, closes hsutter#106
The current implementation allows for writing below code
It fails as
as<std::size_t>(argc)
matchesas(...)
that returnsnonesuch
.This change introduces
as
casts:as
- static asserted casts,as std::optional<T>
- optional cast,cpp2::unsafe_narrow<T>()
- narrow cast,If the safe cast is not possible, the
as
cast will fail to compile with information to use optional castas std::optional<T>
.The specialization of the
as()
functions try to match arithmetic types (algorithm based on thegsl::narrow()
function). It also removes the version of theas(...)
function that silently returnsnullptr
- now it fails at compile time thanks tostatic_assert
.After this change, the below code does not compile all narrowing cases, and will trigger static assertion with the information to use two other possible casts:
Thanks to optional as
as std::optional<T>
we can rewrite it to:Thanks to
unsafe_narrow()
:That will also fix below code:
Or with
unsafe_narrow
:Closes #104