Skip to content
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

Do not use static_assert() to prevent template instantation errors #77

Open
mpusz opened this issue Apr 20, 2023 · 17 comments
Open

Do not use static_assert() to prevent template instantation errors #77

mpusz opened this issue Apr 20, 2023 · 17 comments

Comments

@mpusz
Copy link

mpusz commented Apr 20, 2023

Using static_assert() allows great error messages but is a really bad solution for verification of a template instantiation. It does not SFINAE and is harmless to overload resolution.

For example, I just obtained the following error message:

[build] /home/mpusz/.conan2/p/wg21-711143a21ed44/p/include/linear_algebra/op_traits_multiplication.hpp: In instantiation of ‘struct std::experimental::math::detail::multiplication_engine_traits<void, std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major> >’:
[build] /home/mpusz/.conan2/p/wg21-711143a21ed44/p/include/linear_algebra/op_traits_multiplication.hpp:309:24:   required from ‘struct std::experimental::math::detail::multiplication_arithmetic_traits<void, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void> >’
[build] /home/mpusz/.conan2/p/wg21-711143a21ed44/p/include/linear_algebra/arithmetic_operators.hpp:60:32:   required from ‘constexpr auto std::experimental::math::operator*(const matrix<ET1, COT1>&, const matrix<ET2, COT2>&) [with ET1 = matrix_storage_engine<int, 3, 1, void, matrix_layout::column_major>; COT1 = void; ET2 = matrix_storage_engine<int, 3, 1, void, matrix_layout::column_major>; COT2 = void]’
[build] /usr/include/c++/12/bits/stl_function.h:286:37:   required by substitution of ‘template<class _Tp, class _Up> constexpr decltype ((forward<_Tp>(__t) * forward<_Up>(__u))) std::multiplies<void>::operator()(_Tp&&, _Up&&) const [with _Tp = std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>; _Up = std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>]’
[build] /usr/include/c++/12/type_traits:2565:26:   required by substitution of ‘template<class _Fn, class ... _Args> static std::__result_of_success<decltype (declval<_Fn>()((declval<_Args>)()...)), std::__invoke_other> std::__result_of_other_impl::_S_test(int) [with _Fn = std::multiplies<void>; _Args = {std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>}]’
[build] /usr/include/c++/12/type_traits:2576:55:   required from ‘struct std::__result_of_impl<false, false, std::multiplies<void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void> >’
[build] /usr/include/c++/12/type_traits:3038:12:   recursively required by substitution of ‘template<class _Result, class _Ret> struct std::__is_invocable_impl<_Result, _Ret, true, std::__void_t<typename _CTp::type> > [with _Result = std::__invoke_result<std::multiplies<void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void> >; _Ret = void]’
[build] /usr/include/c++/12/type_traits:3038:12:   required from ‘struct std::is_invocable<std::multiplies<void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void> >’
[build] /usr/include/c++/12/type_traits:3286:71:   required from ‘constexpr const bool std::is_invocable_v<std::multiplies<void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void>, std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void> >’
[build] /usr/include/c++/12/concepts:336:25:   required by substitution of ‘template<class Q>  requires (Quantity<Q>) && (InvokeResultOf<(mp_units::quantity<R, Rep>::quantity_spec * (Q::quantity_spec)).character, std::multiplies<void>, Rep, typename Q::rep>) constexpr auto [requires mp_units::Quantity<<placeholder>, >] mp_units::operator*(const quantity<reference<isq::position_vector(), si::metre()>(), std::experimental::math::matrix<std::experimental::math::matrix_storage_engine<int, 3, 1, void, std::experimental::math::matrix_layout::column_major>, void> >&, const Q&) [with Q = mp_units::reference<mp_units::isq::position_vector(), mp_units::si::metre()>()]’
[build] ../../example/linear_algebra.cpp:103:34:   required from here
[build] /home/mpusz/.conan2/p/wg21-711143a21ed44/p/include/linear_algebra/op_traits_multiplication.hpp:171:61: error: static assertion failed: mis-matched/invalid number of rows and columns for multiplication
[build]   171 |     static_assert((C1 == R2  ||  C1 == std::dynamic_extent  ||  R2 == std::dynamic_extent),
[build]       |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

After commenting out the static_assert() in op_traits_multiplication.hpp, my code compiled fine because a better overload was found.

Use concepts instead to prevent instantiations of types like multiplication_engine_traits.

@BobSteagall
Copy link
Owner

What is the offending code that caused the failure? That static assert is there for a reason -- if it fires, then the dimensions of the multiplication don't line up, and multiplication is not possible.

@mpusz
Copy link
Author

mpusz commented Apr 20, 2023

This is not the only overload participating in the overload resolution. The compiler can't choose the correct one because static_assert() aborts the instantiation of an invalid overload.

@mpusz
Copy link
Author

mpusz commented Apr 20, 2023

BTW, there are many other similar problems like that with this library. We have to meet at some point and work together to try to resolve those. Until then I recommend testing the library with various custom representation types.

@BobSteagall
Copy link
Owner

I need some example code to test against.

@dwith-ts
Copy link

The offending traits are in the body of the method but the method has an auto return type. As @mpusz is using decltype() with that method the compiler needs to look at the method body to determine the return type which fires the static assert. L

For a normal method call it would probably work.

@mpusz
Copy link
Author

mpusz commented Apr 20, 2023

I understand, but I can't provide it now as my code base is not stable. When it is on the mainline, I will provide one.

In the meantime, here is a simple code showing why static_assert() is a really bad idea: https://godbolt.org/z/v4qPxP5EW and should not be used. If the check is really needed use concepts to prevent the instantiation rather then abort while instantiating the type.

@BobSteagall
Copy link
Owner

Yes, but overload resolution is not the source of the static_assert failing in your Godbolt example; the definition of foo(double) is. Comment out the call to foo in main and compilation still fails.

If I understand correctly, what you're saying is that overload resolution in your code is trying to resolve at least one overload whose matrix dimensions are incompatible with multiplication. And that overload is causing the static_assert to fail.

And so my question is this -- how is it that you have an overload where you are trying to multiply two matrices with incompatible dimensions? This is why I wanted to see an example.

@BobSteagall
Copy link
Owner

Note that I am not disagreeing with your statements about using concepts -vs- static assert. I am however trying to understand the source of the static_assert failure.

@dwith-ts
Copy link

dwith-ts commented Apr 20, 2023

The offending traits are in the body of the method but the method has an auto return type. As @mpusz is using decltype() with that method the compiler needs to look at the method body to determine the return type which fires the static assert. L

For a normal method call it would probably work.

As I said, I strongly suspect that it’s a decltype usage (not an actual call, see the error output) in @mpusz’s code and the usage of an auto return type in @BobSteagall’s code which means the method body has to be instantiated to infer the return type which causes the static assert.

@BobSteagall
Copy link
Owner

As I said, I strongly suspect that it’s a decltype usage (not an actual call, see the error output) in @mpusz’s code and the usage of an auto return type in @BobSteagall’s code which means the method body has to be instantiated to infer the return type which causes the static assert.

Yes, quite possibly true. But that begs the question -- decltype of what? Of a multiplication with incompatible dimensions?

@BobSteagall
Copy link
Owner

In fact, as I look at the error messages, it appears to be an attempt to multiply a 3x1 matrix by a 3x1 matrix, which is incompatible.

@dwith-ts
Copy link

As I said, I strongly suspect that it’s a decltype usage (not an actual call, see the error output) in @mpusz’s code and the usage of an auto return type in @BobSteagall’s code which means the method body has to be instantiated to infer the return type which causes the static assert.

Yes, quite possibly true. But that begs the question -- decltype of what? Of a multiplication with incompatible dimensions?

This is a multiplication of a vector with a physical unit from @mpusz’s library, so the multiplication from that library is supposed to be used IMHO. However, when the compiler determines the viable candidates for the operator* overload set (due to invoke_result / decltype usage) it needs the return type of all of these methods. And in order to determine its return type it needs to instantiate the method (because it has an auto return type) even though it will not be selected in the end.
At least that’s my understanding.

@mpusz
Copy link
Author

mpusz commented Apr 20, 2023

What is the offending code that caused the failure? That static assert is there for a reason -- if it fires, then the dimensions of the multiplication don't line up, and multiplication is not possible.

The offending code is:

template<typename Rep = double>
using vector = STD_LA::fixed_size_column_vector<Rep, 3>;

Quantity auto v = make_quantity<isq::position_vector[m]>(vector<int>{1, 2, 3});
std::cout << "2 * v = " << 2 * v << "\n";

There is NO matrix * matrix multiplication here. My understanding is that there are a few overloads in the overload set and one of them aborts the compilation even though it is not a valid match anyway.

I need some example code to test against.

As I wrote before, my code is not ready to share now :-( However, the function that is being called here is just:

  template<typename Value>
    requires(!Quantity<Value>) && detail::InvokeResultOf<quantity_spec.character, std::multiplies<>, const Value&, rep>
  [[nodiscard]] friend constexpr Quantity auto operator*(const Value& v, const quantity& q)
  {
    return make_quantity<reference>(v * q.number());
  }

@BobSteagall
Copy link
Owner

What are the types of Value and rep? Are they both 3x1 matrices?

@mpusz
Copy link
Author

mpusz commented Apr 20, 2023

Value = int
rep = STD_LA::fixed_size_column_vector<int, 3>

@mpusz
Copy link
Author

mpusz commented May 26, 2023

Today I came back to LA and found the cause of static_assert():

https://github.com/mpusz/units/blob/b7e467ff42eb95dbefcf4c835b64d3a8760be3be/src/core/include/mp-units/bits/sudo_cast.h#L73

If LA types and operators were guarded with concepts rather than static_assert(), this code would compile fine.

@mpusz
Copy link
Author

mpusz commented Jul 11, 2024

Hi @BobSteagall, could you please remove this static_assert from the code as it breaks other code for me as well? See more here: https://github.com/mpusz/mp-units/actions/runs/9669968920/job/26677689242.

mpusz added a commit to mpusz/mp-units that referenced this issue Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants