cppfront icon indicating copy to clipboard operation
cppfront copied to clipboard

[BUG] `operator is` should be constrained or answered statically

Open JohelEGP opened this issue 1 year ago • 14 comments

Title: operator is should be constrained or answered statically.

Description:

This is to implement P2392's §4.2. It's also to correctly resolve #433, as split off from #491:

Comments moved from #491.

The case of this PR is:

The alternative's is-as-expression-target matches but the statement is ill-formed for the inspected type.

P2392 doesn't say what should happen in this case. If one extracted the wording from the design, it would be UB by omission

IIUC what P1371 says, then it would similarly be UB by omission:

When inspect is executed, its condition is evaluated and matched in order (first match semantics) against each pattern. If a pattern successfully matches the value of the condition and the boolean expression in the guard evaluates to true (or if there is no guard at all), then the value of the resulting expression is yielded or control is passed to the compound statement, depending on whether the inspect yields a value. If the guard expression evaluates to false, control flows to the subsequent pattern.

If no pattern matches, none of the expressions or compound statements specified are executed. In that case if the inspect expression yields void, control is passed to the next statement. If the inspect expression does not yield void, std::terminate will be called.

It also links to https://github.com/solodon4/Mach7, https://github.com/mpark/patterns, and https://github.com/jbandela/simple_match. But this corner case doesn't seem to be mentioned in their README.

Originally posted by @JohelEGP in https://github.com/hsutter/cppfront/issues/491#issuecomment-1575125192


How about this instead?

For a dependent inspect, when the is-as-expression-target of an alternative is well-formed, the corresponding statement should also be well-formed.

So we move from a runtime-checked contract to a compile-time assertion.

Originally posted by @JohelEGP in https://github.com/hsutter/cppfront/issues/491#issuecomment-1575201581


UB by omission

Actually, the proposals are clear that when there's a match, execution continues at the corresponding statement.

The proposals are also clear in that an ill-formed condition means the whole branch is a discarded statement.

So it falls out from the existing rules that a well-formed condition implies the corresponding statement is also instantiated.

The actual bug is that the validity check happens for the statement and not at the condition (and thus for the whole branch).

Another bug is that an is-expression is always be well-formed, resulting in always-false when the equivalent without is would be ill-formed.

There is an example at P2392 §3.4.3 that mixes conditions that don't work for all intended types. It can demonstrate the issue. Here is a reduction: https://cpp2.godbolt.org/z/c5rsjP6a7.

in: (min, max) -> _ = :<T> (x: T) -> bool requires std::integral<T> = { return min$ <= x <= max$; };
f: <T> (x: T) -> _ = {
  return inspect x -> std::string {
    is std::string = "a string";
    is (in(1, 2)) = "1 or 2";
    is _ = "something else";
  };
}
main: () = {
  s: std::string = ();
  std::cout << "(f(s))$\n"             // prints "a string".
            << "(f(1))$\n"             // prints "1 or 2".
            << "(f(42))$\n"            // prints "something else".
            << "(s is (in(1, 2)))$\n"; // prints "0".
}

Notice how s is (in(1, 2)) is well-formed. It should be ill-formed outside a dependent inspect. As the condition of the alternative of a dependent inspect, it should make the alternative discarded.

Originally posted by @JohelEGP in https://github.com/hsutter/cppfront/issues/491#issuecomment-1575229184


Consider this degenerate case:

  • A debug build.
  • An inspect of 100 alternatives:
    • First, 99 alternatives with a condition of a predicate well-formed for integers only,
    • then, the match-all is _.

Because a condition is always well-formed, an always-false condition for a given input type isn't optimized out.

Given an input std::string, the first 99 conditions are necessarily evaluated sequentially, even though they're all always-false.

Originally posted by @JohelEGP in https://github.com/hsutter/cppfront/issues/491#issuecomment-1575282683


Let's take the same inspect again.

  return inspect x -> std::string {
    is std::string = "a string";
    is (in(1, 2)) = "1 or 2";
    is _ = "something else";
  };

For the given conditions, the operator is overloads of std::variant<std::string, i32> should be well-formed and necessarily runtime-evaluated. The same for std::variant<std::monostate> should be ill-formed. So std::variant::operator is should only work at runtime and when the query can be forwarded to one of its variant alternatives. Nothing changes the fact that if the condition is well-formed, the statement should be instantiated.

Originally posted by @JohelEGP in https://github.com/hsutter/cppfront/issues/491#issuecomment-1575308864


Once operator is is constrained or answered statically, it becomes necessary to fix inspect to omit alternatives whose conditions are non-viable. It also becomes possible to omit alternatives whose conditions always-false.

Note that there's a distinction between an operator is that is constrained and answered statically. A constrained operator is has preconditions on the input type. There are other contexts where we might want the semantics of an inspect's alternative's condition.

g: <T> (x: T) = {
  // Continues working.
  if constexpr T is std::string { }

  // The following can work given C++23's P2280 (C++20 DR).

  if constexpr x is std::string { }
  // Will be `true` for `T == int` (using the built-in `operator is`).
  // For a `T` that is a specialization of `std::variant`,
  // it would be ill-formed,
  // but can be made to work
  // if given the semantics of the condition of an `inspect`'s alternative
  // (i.e., always-`false`).

  if constexpr x is 0 { }
  // Can be `true` for `T == std::integral_constant<i32, 0>` if overloaded.
}

Minimal reproducer (https://cpp2.godbolt.org/z/87hjsccYd):

in: (min, max) -> _ = :<T> (x: T) -> bool requires std::integral<T> = { return min$ <= x <= max$; };
f: <T> (x: T) -> _ = {
  return inspect x -> std::string {
    is std::string = :(x) -> _ = { return "a string"; }();
    is (in(1, 2)) = "1 or 2";
    is _ = "something else";
  };
}
main: () = {
  s: std::string = ();
  std::cout        //
    << "(f(s))$\n" // prints ``.
    << "(f(1))$\n" // prints `1 or 2`.
    ;
  std::cout                   //
    << "(s is (in(1, 2)))$\n" // prints `0`.
    ;
}
Commands:
cppfront -clean-cpp1 main.cpp2
clang++17 -std=c++23 -stdlib=libc++ -lc++abi -pedantic-errors -Wall -Wextra -Wconversion -I . main.cpp

Expected result:

  • f(s): An error when instantiating :(x) -> _ = { return "a string"; }().
  • f(1): Alternative with is std::string to be statically elided.
  • s is (in(1, 2)): An error, just like in(1, 2)(s).

Actual result and error:

  • f(s): Unconditionally returns std::string() because the matched alternative's statement is ill-formed.
  • f(1): Unconditionally evaluates is std::string that will always be false.
  • s is (in(1, 2)): Unconditionally results in false, even though in(1, 2)(s) is ill-formed.
Cpp2 lowered to Cpp1.


#include "cpp2util.h"


[[nodiscard]] auto in(auto const& min, auto const& max) -> auto;
template<typename T> [[nodiscard]] auto f(T const& x) -> auto;


auto main() -> int;

[[nodiscard]] auto in(auto const& min, auto const& max) -> auto { return [_0 = min, _1 = max]<typename T>(T const& x) -> bool
requires (std::integral<T>)
{return [_0 = _0, _1 = x, _2 = _1]{ return cpp2::cmp_less_eq(_0,_1) && cpp2::cmp_less_eq(_1,_2); }(); };  }
template<typename T> [[nodiscard]] auto f(T const& x) -> auto{
  return [&] () -> std::string { auto&& __expr = x;
    if (cpp2::is<std::string>(__expr)) { if constexpr( requires{[](auto const& x) -> auto{return "a string"; }();} ) if constexpr( std::is_convertible_v<CPP2_TYPEOF(([](auto const& x) -> auto{return "a string"; }())),std::string> ) return [](auto const& x) -> auto{return "a string"; }(); else return std::string{}; else return std::string{}; }
    else if (cpp2::is(__expr, (in(1, 2)))) { if constexpr( requires{"1 or 2";} ) if constexpr( std::is_convertible_v<CPP2_TYPEOF(("1 or 2")),std::string> ) return "1 or 2"; else return std::string{}; else return std::string{}; }
    else return "something else"; }
  ();
}
auto main() -> int{
  std::string s {};
  std::cout        //
    << cpp2::to_string(f(s)) + "\n" // prints ``.
    << cpp2::to_string(f(1)) + "\n";// prints `1 or 2`.

  std::cout                   //
    << cpp2::to_string(cpp2::is(std::move(s), (in(1, 2)))) + "\n";// prints `0`.

}
Output.
Program returned: 0

1 or 2
0

JohelEGP avatar Jun 04 '23 16:06 JohelEGP

There are other contexts where we might want the semantics of an inspect's alternative's condition.

See also P2392's §2.1.2, §4.1.8.

A: It's a better spelling for a chain of `if constexpr` `else`.

Given a resolution for this issue, what's the use case for inspect constexpr? From §3.1:

inspect constexpr requires expr and all alternative conditions to be compile time constant expressions. All non-selected alternative results are discarded, and their return statements do not participate in function return type deduction.

JohelEGP avatar Jun 04 '23 17:06 JohelEGP

as returning cpp2::nonesuch needs to be similarly reviewed.

JohelEGP avatar Jun 07 '23 14:06 JohelEGP

It also becomes possible to omit alternatives whose conditions always-false.

It's not OK, it's necessary. To omit them. Otherwise the following would be ill-formed.

f: <T> (x: T) -> _ = {
  return inspect x -> std::string {
    is std::string = x.substr(0);
    is _ = "something else";
  };
}
main: () = {
  std::cout        //
    << "(f(1))$\n" // error: `1.substr(0)`.
    ;
}

JohelEGP avatar Jun 20 '23 16:06 JohelEGP

All compilers agree that this can be made to work if I just add constexpr to relevant is overloads: https://compiler-explorer.com/z/j5q9co99c.

JohelEGP avatar Jun 20 '23 17:06 JohelEGP

I agree that constexpr is necessary for the pure type matching. This also becomes critical if inspect is used in the context of a template parameter.

Did you create a test file where all common cases are evaluated?

MaxSagebaum avatar Jun 20 '23 21:06 MaxSagebaum

Did you create a test file where all common cases are evaluated?

The one in the OP works, thought you have to inspect the generated code to see they are:

f(1): Unconditionally evaluates is std::string that will always be false.

JohelEGP avatar Jun 20 '23 21:06 JohelEGP

All compilers agree that this can be made to work if I just add constexpr to relevant is overloads: https://compiler-explorer.com/z/j5q9co99c.

But not to the point necessary to optimize inspect. It definitely doesn't work in dependent contexts: https://compiler-explorer.com/z/3jsE8oz4z. Seems like support for P2280 will be necessary.

JohelEGP avatar Jun 20 '23 21:06 JohelEGP

It should still be possible to fix these bugs and continue supporting currently supported compilers (so support for P2280 is not needed).

Have operator is either return bool, return std::bool_constant<B> for some B, or be non-viable (for type-unsafe inputs).

Then, we can optimize inspect by inspecting each alternative's condition in the condition of a generated constexpr if that guards the alternative.

If the chosen operator is overload returns std::false_type, or if there is no viable operator is, the alternative is statically elided. If the chosen operator is overload returns std::true_type, the alternative's statement is unconditionally evaluated. If the chosen operator is overload returns bool, the alternative's statement is conditionally evaluated on it.

JohelEGP avatar Jun 20 '23 22:06 JohelEGP

This is how it should look: https://cpp2.godbolt.org/z/s17G457aP.

#include <functional>
operator_is: <T, U> (_: U) -> _ = std::bool_constant<std::is_same_v<T, U>>();
operator_is: <T, F> (v: T, f: F) -> bool requires std::predicate<decltype((f)), decltype((v))> = { return std::invoke(f, v); }
template <class T, class U> concept has_type_operator_is = requires(U u) { operator_is<T>(u); };
template <class T, class U> concept has_value_operator_is = requires(T t, U u) { operator_is(t, u); };
in: (min, max) -> _ = :<T> (x: T) -> bool requires std::integral<T> = { return min$ <= x <= max$; };
f: <T> (x: T) -> _ = {
  // return inspect x -> std::string {
  //   is std::string = :(x) -> _ = { return "a string"; }();
  //   is (in(1, 2)) = "1 or 2";
  //   is _ = "something else";
  // };
  if constexpr has_type_operator_is<std::string, decltype((x))> {
    if constexpr !std::is_same_v<decltype(operator_is<std::string>(x)), std::false_type> {
      if constexpr std::is_same_v<decltype(operator_is<std::string>(x)), std::true_type> {
        return :(x) -> _ = { return "a string"; }();
      } else {
        if operator_is<std::string>(x) {
          return :(x) -> _ = { return "a string"; }();
        }
      }
    }
  }
  else
  if constexpr has_value_operator_is<decltype((x)), decltype(in(1, 2))> {
    if constexpr !std::is_same_v<decltype(operator_is(x, in(1, 2))), std::false_type> {
      if constexpr std::is_same_v<decltype(operator_is(x, in(1, 2))), std::true_type> {
        return "1 or 2";
      } else {
        if operator_is(x, in(1, 2)) {
          return "1 or 2";
        }
      }
    }
  }
  return "something else";
}
main: () = {
  s: std::string = ();
  std::cout        //
    // << "(f(s))$\n" // Now errors during instantiation.
    << "(f(1))$\n" // Now statically elides `is std::string`-alternative, but doesn't return `"1 or 2"`, why?
    ;
  // std::cout                   //
  //   << "(operator_is(s, in(1, 2)))$\n" // Still fails at link-time rather than compile-time, why?
  //   ;
}

JohelEGP avatar Jun 20 '23 23:06 JohelEGP

I would guess it is the wrong forward declaration of operator_is:

template<typename T, typename F> [[nodiscard]] auto operator_is(T const& v, F const& f) -> bool;

The requirement is missing. This makes the compiler think that there is an alternative definition which leads to the linker error.

On a quick check, I could not find if the requires clause can be defined in a forward declaration.

MaxSagebaum avatar Jun 21 '23 06:06 MaxSagebaum

It should still be possible to fix these bugs and continue supporting currently supported compilers (so support for P2280 is not needed).

Have operator is either return bool, return std::bool_constant<B> for some B, or be non-viable (for type-unsafe inputs).

This is actually necessary since an operator is isn't necessarily constexpr.

JohelEGP avatar Dec 03 '23 19:12 JohelEGP

I will take a look on your cases and will check if they are fixed or might be fixed in #701

filipsajdak avatar Dec 04 '23 23:12 filipsajdak

It certainly is an improvement.

JohelEGP avatar Dec 04 '23 23:12 JohelEGP