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

fmt/ranges.h incompatible with Eigen 3.4 #3839

Closed
baronep opened this issue Feb 2, 2024 · 10 comments
Closed

fmt/ranges.h incompatible with Eigen 3.4 #3839

baronep opened this issue Feb 2, 2024 · 10 comments

Comments

@baronep
Copy link

baronep commented Feb 2, 2024

Eigen 3.4 seems to have a significant change in behavior where begin() exists for all matrices, but returns void for non 1-D matrices where iteration is not well defined.

This currently interferes with the fmt/ranges.h implementation such that the following code compiles with Eigen 3.9, but no longer compiles with 3.4.

#include <Eigen/Core>

#include <fmt/format.h>
#include <fmt/ostream.h>
#include <fmt/ranges.h> // doesn't work if uncomment this

int main() {
  Eigen::Matrix3d m;
  m << 1, 2, 3, 4, 5, 6, 7, 8, 9;
  fmt::print("{}\n", m);

  return 0;
}

Note: This was built using the -DFMT_DEPRECATED_OSTREAM compiler flag

<source>:10:13:   required from here
/opt/compiler-explorer/libs/fmt/9.1.0/include/fmt/ranges.h:264:34: error: void value not ignored as it ought to be
  264 |     decltype(*detail::range_begin(std::declval<Range&>()));

Godbolt Example: https://godbolt.org/z/TzG7TMxWx

@baronep
Copy link
Author

baronep commented Feb 2, 2024

One potential solution appears to be patching has_const_begin_end and has_mutable_begin_end as follows ...

template <typename T>
struct has_const_begin_end<
    T,
    void_t<
        decltype(detail::range_begin(std::declval<const remove_cvref_t<T>&>())),
        decltype(detail::range_end(std::declval<const remove_cvref_t<T>&>())),
        decltype(detail::range_begin(std::declval<const remove_cvref_t<T>&>()) != detail::range_end(std::declval<const remove_cvref_t<T>&>()))>> // Added
    : std::true_type {};

template <typename T>
struct has_mutable_begin_end<
    T, void_t<decltype(detail::range_begin(std::declval<T>())),
              decltype(detail::range_end(std::declval<T>())),
              decltype(detail::range_begin(std::declval<T>()) != detail::range_end(std::declval<T>())), // Added
              enable_if_t<std::is_copy_constructible<T>::value>>> 
    : std::true_type {};

@vitaut
Copy link
Contributor

vitaut commented Feb 2, 2024

FMT_DEPRECATED_OSTREAM has been removed so this conflict won't happen on the current version. You can format objects via ostream as follows:

  fmt::print("{}\n", fmt::streamed(m));

In this case Eigen providing broken begin / end doesn't matter.

@vitaut vitaut closed this as completed Feb 2, 2024
@vitaut vitaut added the question label Feb 2, 2024
@baronep
Copy link
Author

baronep commented Feb 3, 2024

@vitaut I think that FMT_DEPRECATED_OSTREAM is a bit of a red herring in this case. Even if this is removed and a custom fomatter is provided, the issue is still present.

See the updated godbolt here for an example: https://godbolt.org/z/xbETKd5h1

@baronep
Copy link
Author

baronep commented Feb 3, 2024

I think that is_range is incorrectly returning that Eigen::Matrix3d is a range when it in fact not.

E.g. https://godbolt.org/z/xbETKd5h1

@vitaut
Copy link
Contributor

vitaut commented Feb 3, 2024

I think you are right and we should improve the begin/end check. Could you submit a PR with your proposed fix?

@vitaut vitaut reopened this Feb 3, 2024
@vitaut vitaut removed the question label Feb 3, 2024
@Skylion007
Copy link

I hit the same issue with Eigen years ago, they haven't done a new release in years.

@jdrouhard
Copy link

I came across a similar problem, but with a different compiler error: https://godbolt.org/z/PTPzGjY1P

It seems to be the same issue, where the is_range check is "true" for this type even when there's an explicit formatter defined for it. My example only uses fmt and shows an ambiguous template instantiation error. If it's a different problem, I can open a new issue. This did become more problematic for us as we use fmt::join() pretty heavily, and the new requirement of including the ranges.h header makes this show up everywhere for us.

@vitaut
Copy link
Contributor

vitaut commented May 14, 2024

@jdrouhard, if you want to override the standard range formatter you also have to opt out of it being formatted as a range or use an adapter type. is_range cannot do it for you.

@jdrouhard
Copy link

Yeah, that's not what we're trying to do.

We have some types with begin() and end() but we specifically don't want them to be formatted as ranges. Including fmt/ranges.h breaks a lot of our codebase because there is now an ambiguous template instantiation: the "detected" range formatter as well as our custom formatter that we prefer to use (either via format_as() or by specializing the formatter type - both cause ambiguous instantiations).

See the godbolt link above which shows what I'm talking about. Commenting out the ranges.h header lets it compile, but we lose fmt::join in trunk. It works currently in 10.2.1 because fmt::join() isn't in that header and we don't really use anything from the ranges.h header otherwise.

It seems that the range formatter that simply detects whether a type should be formatted as a range should be disabled if the type has an explicit format_as() or formatter<> specialization. Ambiguous template instantiations that cause code to not compile isn't great :)

Arghnews added a commit to Arghnews/fmt that referenced this issue May 15, 2024
Disable formatting type as range if user provided format_as function
exists, using that instead. Should fix issue raised in fmtlib#3839
Arghnews added a commit to Arghnews/fmt that referenced this issue May 15, 2024
Disable formatting type as range if user provided format_as function
exists, using that instead. Should fix issue raised in fmtlib#3839
Arghnews added a commit to Arghnews/fmt that referenced this issue May 19, 2024
Fixes issue fmtlib#3839
An Eigen 3.4 2x2 matrix has a begin member function that returns void
Be more strict checking that the result of calling *begin() is valid
See input_or_output_iterator concept notes about void
Arghnews added a commit to Arghnews/fmt that referenced this issue May 19, 2024
Fixes issue fmtlib#3839
An Eigen 3.4 2x2 matrix has a begin member function that returns void
Be more strict checking that the result of calling *begin() is valid
See input_or_output_iterator concept notes about void
Arghnews added a commit to Arghnews/fmt that referenced this issue May 20, 2024
Fixes issue fmtlib#3839
An Eigen 3.4 2x2 matrix has a begin member function that returns void
Be more strict checking that the result of calling *begin() is valid
See input_or_output_iterator concept notes about void
vitaut pushed a commit that referenced this issue May 20, 2024
Fixes issue #3839
An Eigen 3.4 2x2 matrix has a begin member function that returns void
Be more strict checking that the result of calling *begin() is valid
See input_or_output_iterator concept notes about void
@vitaut
Copy link
Contributor

vitaut commented May 21, 2024

Fixed in #3964 (thanks to @Arghnews).

@vitaut vitaut closed this as completed May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants