Skip to content

[BUG] forward on non-deducible parameter is move #572

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

Open
JohelEGP opened this issue Aug 5, 2023 · 6 comments
Open

[BUG] forward on non-deducible parameter is move #572

JohelEGP opened this issue Aug 5, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

JohelEGP commented Aug 5, 2023

Title: forward on non-deducible parameter is move.

Description:

I inadvertently wrote the function from: <Number> (forward q: quantity<Number>),
thinking that would make q a forwarding parameter for any quantity.
Cppfront recognizes q's type as non-deducible,
and doesn't make q a forwarding parameter.
The end result is that the generated code is the same as using move instead of forward.

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

quantity: @struct <Number: type> type = {
  number: Number;
}
from: <Number> (forward q: quantity<Number>) = _ = q;
main: () = {
  q: quantity<i32> = ();
  q.from();
  _ = q;
}
Commands:
cppfront main.cpp2
clang++17 -std=c++23 -stdlib=libc++ -lc++abi -pedantic-errors -Wall -Wextra -Wconversion -fsanitize=undefined -Werror=unused-result -I . main.cpp

Expected result: A diagnostic indicating that q can't be made a forwarding parameter.

Actual result and error:

Cpp2 lowered to Cpp1:
//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"

template<typename Number> class quantity;
  

//=== Cpp2 type definitions and function declarations ===========================

template<typename Number> class quantity {
  public: Number number; 
};
template<typename Number> auto from(quantity<Number>&& q) -> void;
auto main() -> int;
  

//=== Cpp2 function definitions =================================================


template<typename Number> auto from(quantity<Number>&& q) -> void { (void) CPP2_FORWARD(q);  }
auto main() -> int{
  quantity<cpp2::i32> q {}; 
  CPP2_UFCS_0(from, q);
  (void) std::move(q);
}
Output:
main.cpp2:7:15: error: no matching function for call to 'from'
    7 |   CPP2_UFCS_0(from, q);
      |               ^~~~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:727:16: note: expanded from macro 'CPP2_UFCS_0'
  727 |         return FUNCNAME(CPP2_FORWARD(obj)); \
      |                ^~~~~~~~
main.cpp2:7:3: note: in instantiation of function template specialization 'main()::(anonymous class)::operator()<quantity<int> &>' requested here
    7 |   CPP2_UFCS_0(from, q);
      |   ^
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:729:2: note: expanded from macro 'CPP2_UFCS_0'
  729 | }(PARAM1)
      |  ^
main.cpp2:4:32: note: candidate function [with Number = int] not viable: expects an rvalue for 1st argument
    4 | template<typename Number> auto from(quantity<Number>&& q) -> void { (void) CPP2_FORWARD(q);  }
      |                                ^    ~~~~~~~~~~~~~~~~~~~~

See also:

@JohelEGP JohelEGP added the bug Something isn't working label Aug 5, 2023
@JohelEGP
Copy link
Contributor Author

JohelEGP commented Aug 5, 2023

In my current use case, I don't care about Number.
So what I really want to say is:

from: (forward q: quantity) = _ = q;

But that treats quantity as a concrete type, and lowers to:

requires (std::is_same_v<CPP2_TYPEOF(q), quantity>)

That ends up with this error:

main.cpp2:5:50: error: type/value mismatch at argument 2 in template parameter list for 'template<class _Tp, class _Up> constexpr const bool std::is_same_v<_Tp, _Up>'
main.cpp2:5:50: note:   expected a type, got 'quantity'

What I want is this behavior from P2392's §2.1:
1691262036

So given appropriate cpp::is overloads for the sublist of:
1691262555

I want

from: (forward q: quantity) = _ = q;
from: (forward q: quantity<float>) = _ = q;

to respectively translate to

from: (forward q) requires cpp2::is<quantity, CPP2_TYPEOF(q)>() = _ = q;
from: (forward q) requires cpp2::is<quantity<float>, CPP2_TYPEOF(q)>() = _ = q;

With the following two overloads,
a forward parameter can now work if its parameter type names a template (https://cpp2.godbolt.org/z/8zavMvfE8):

template <template <typename...> class C, typename T>
consteval auto is() -> bool {
    if constexpr (requires(T t) { []<typename... Ts>(C<Ts...> const&) { }(t); }) {
        return true;
    }
    return false;
}
template <typename T, typename U>
consteval auto is() -> bool {
    return std::is_same_v<T, U>;
}

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Aug 5, 2023

Expected result: A diagnostic indicating that q can't be made a forwarding parameter.

An alternative expected result of
from: <Number> (forward q: quantity<Number>)
is for q to be a forwarding parameter
and have it deduce Number.

This formulation does that (https://compiler-explorer.com/z/hq1W6PTMP):

template<typename q_t, typename Number = typename decltype([]<typename Number>(const quantity<Number>&) { return std::type_identity<Number>{}; }(std::declval<q_t>()))::type> auto from(q_t&& q) -> void;
template<typename q_t, typename Number> auto from(q_t&& q) -> void { (void) (CPP2_FORWARD(q).number = Number());  }

The error messages do leave a lot to be desired (uncomment the call with std::variant).

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Aug 5, 2023

In my current use case, I don't care about Number.

Actually, I do have an use case:

    export wide: <U, N> (forward qty: quantity<length, U, N>) -> _ = :quantity = (:width = (), qty.number, :U = ());
    export tall: <U, N> (forward qty: quantity<length, U, N>) -> _ = :quantity = (:height = (), qty.number, :U = ());

I call those on prvalues, so I didn't notice the parameters were rvalues and not forwarding.

@hsutter
Copy link
Owner

hsutter commented Aug 11, 2023

Thanks! I've read through the use cases a few times, and I think the part of this that is a feature extension can be deferred to the future. But I think the original ask was this:

Expected result: A diagnostic indicating that q can't be made a forwarding parameter.

If there's a better diagnostic we can emit for erroneous code, could we have a PR for that? Otherwise we can close this I think?

Thanks!

@JohelEGP
Copy link
Contributor Author

A PR would be trivial after #534 is fixed.
What main has right now isn't enough to detect a forward parameter with non-deducible type.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Oct 2, 2024

The comments #572 (comment) and #572 (comment)
are be pretty much the other part of P2481,
the paper mentioned in commit 36c5a9e.

Cppfront implements from P2481

  • the fully generic part with f: (forward x: _) = { ... }, and
  • the type_of constrained part with f: (forward x: of_my_type) = { ... }.

What's missing in Cppfront from P2481
is actually making the parameter be of that type_of
(with its deduced constness and reference).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants