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

Hooking custom enum serialization framework into CLI11 #908

Open
captainurist opened this issue Jul 13, 2023 · 16 comments
Open

Hooking custom enum serialization framework into CLI11 #908

captainurist opened this issue Jul 13, 2023 · 16 comments

Comments

@captainurist
Copy link
Contributor

captainurist commented Jul 13, 2023

This is the problem that I have:

  • Given an enum serialization framework like magic_enum,
  • Make CLI11 use it for all enums that the framework supports,
  • Automatically, as in w/o repeating myself for each of the supported types.

And it seems right now this is simply impossible to do.

There is a lexical_cast customization point, but I cannot overload it for a constrained set of types because that would result in an ambiguous overload. To be clear, this is what I'm talking about, in user code:

template<class T> requires is_supported_by_magic_enum_v<T>
bool lexical_cast(const std::string &s, T &value) {
    // call magic_enum::enum_cast here.
}

This produces an ambiguous overload, and the only way around is to provide a lexical_cast overload for each of the types. Which is tedious.

Solutions as I see them:

First

Add another layer in the implementation of CLI::detail::lexical_cast:

template<class T>
bool lexical_cast(const std::string &s, T &value) {
    return lexical_cast_impl(s, value);
}
// All existing implementations of lexical_cast are renamed to lexical_cast_impl

This way, the overload in the first example would work.

However, there is also functionality in CLI that converts enums to strings, so probably I should introduce a customization layer there too? I'd appreciate some pointers on where to look.

Second

Just add a #define for "don't do enums", the way nlohmann_json does it.

Then if it's defined, lexical_cast for enums will route through the default streaming implementation, and providing the following overload should solve the issue:

template<class T> requires is_supported_by_magic_enum_v<T>
std::istream& operator>>(std::istream &stream, T &value) {
    // call magic_enum::enum_cast here.
    return stream;
}

This, however, forces the user to either:

  1. Define operator<< in the type's namespace so that it's available via ADL, which might not be desirable. After all, operator<< is not CLI11's extension point, but an extension point of the C++ standard library.
  2. Define operator<< in namespace CLI or namespace CLI::detail, and make sure to pay attention to include order – the headers defining the operator<< should go before CLI11 headers. IMO, this is a no-go, users should be free to order their includes the way they want to.

So

I think it would make sense to implement the first solution above. If this sounds good, I will send in a PR.

Also, I'd be happy to have the second option implemented too, because I want enum parsing from the command line to either route through my custom code, or not compile at all. If this is something that CLI users might find valuable, I'd also be happy to implement it and send in a PR.

@phlptp
Copy link
Collaborator

phlptp commented Jul 13, 2023

One other thing to try is using the ADR capabilities in C++.
Basically would involve having all your magic enums defined in a particular namespace, then defining the lexical cast in that same namespace. ADR in C++ should prioritize use of that lexical cast for those classes and ignore it for everything else, so there shouldn't be any conflicts.

@captainurist
Copy link
Contributor Author

captainurist commented Jul 13, 2023

Here is a full example:

#include <concepts>

#include <CLI/CLI.hpp>

enum my_enum {
    val1,
    val2
};

bool deserialize(const std::string &src, my_enum &dst) {
    if (src == "val1") {
        dst = val1;
        return true;
    } else if (src == "val2") {
        dst = val2;
        return true;
    } else {
        return false;
    }
}

// More deserialize overloads for a gazillion different types...

template<class T>
concept Deserializable = requires (const std::string &src, T &dst) {
    { deserialize(src, dst) } -> std::same_as<bool>;
};

template<class T> requires Deserializable<T>
bool lexical_cast(const std::string &src, T &dst) {
    return deserialize(src, dst);
}

int main(int argc, char **argv) {
    CLI::App app;

    my_enum test = val2;

    app.add_option("--test", test);

    app.parse("--test val1");

    assert(test == val1);

    // More code...

    return 0;
}

(Note that it compiles on clang up to version 15. This is a clang bug, according to the standard this shouldn't compile)

@captainurist
Copy link
Contributor Author

Yes, introducing a lexical_cast overload for each type would work.

What I want is to basically marry CLI with a library that's already providing a gazillion overloads for to/from string conversion, and do this w/o enumerating all the types.

In the example above, this is the part that's trying to do it:

template<class T> requires Deserializable<T>
bool lexical_cast(const std::string &src, T &dst) {
    return deserialize(src, dst);
}

requires Deserializable<T> is basically a requirement that states "this type is supported by an external library that offers to/from string conversions".

And right now, this is not doable. I'm forced to provide lexical_cast overloads by hand for all the types.

Note that all my types are in a single namespace, so this makes things easier. If they were in different namespaces, then I'd still have to provide lexical_cast overloads in each of the namespaces, and there is no way around it.

@jzakrzewski
Copy link

I think it would be better to solve it with type traits than ADL, but yeah, would be nice to have someting generic working.

@captainurist
Copy link
Contributor Author

captainurist commented Jul 13, 2023

@jzakrzewski are you implying that I should specialize CLI::detail::classify_object for my types and overload based on a custom CLI::detail::object_category?

This won't work either, for the same reasons overloading lexical_cast doesn't work in my example above – I'll get an ambiguous partial template specialization.

@captainurist
Copy link
Contributor Author

Also, to clarify:

ADR in C++ should prioritize use of that lexical cast for those classes and ignore it for everything else, so there shouldn't be any conflicts.

This doesn't work because ADL just throws in more functions into the overload set, and prioritization is then done at the overload resolution step, which is where the problems I'm facing lie.

From cppreference:

The set of declarations found by ordinary unqualified lookup and the set of declarations found in all elements of the associated set produced by ADL, are merged, with the following special rules

@phlptp
Copy link
Collaborator

phlptp commented Jul 13, 2023

I am pretty sure the first solution would break any code that already uses a customization for lexical cast, so I am not sure that is a viable solution.

@captainurist
Copy link
Contributor Author

Hmm I don't think it will, but let me double-check.

@phlptp
Copy link
Collaborator

phlptp commented Jul 13, 2023

The basic issue is that you have a bunch of classes that you would like to do a specific lexical cast for overriding the built in one. It is possible to override 1 by 1 but that is a lot of code so using a template is required. This template creates an ambiguity for the compiler which is at present impossible to resolve?

@captainurist
Copy link
Contributor Author

Exactly. And overriding 1-by-1 is error-prone, because even if I miss one override, I'm not getting any diagnostics for it.

@captainurist
Copy link
Contributor Author

captainurist commented Jul 13, 2023

This is a quick and dirty implementation of the first solution:
captainurist#1

Tests pass.

@phlptp
Copy link
Collaborator

phlptp commented Jul 13, 2023

Ok, I am not sure there is any viable path in the current codebase for that. What I would propose is adding an object_category::custom. This would be detected via an is_custom type trait defined in the library. That type trait would only have the base definition in the library so any user could partially specialize it for any type they desired, and types of that would not trigger any of the existing lexical_cast overloads, so a user could create a template for that as well. This would enable a custom conversion of any type if a user desired.

@captainurist
Copy link
Contributor Author

captainurist commented Jul 13, 2023

If the quick&dirty PR above doesn't look good, then I can send a PR introducing a custom object category.

Anything in particular that I should pay attention to? E.g. I have seen value_string overloads for enums, these should probably also be customizable?

@phlptp
Copy link
Collaborator

phlptp commented Jul 13, 2023

ok, I see, that could work, just forcing the lexical cast to a single generic template, all the existing custom overloads would still work since they are specialized.

To be consistent I think something similar would need to be done for type_name and to_string methods since they work very similarly. And since you are working with enums probably value_string as well.

@phlptp
Copy link
Collaborator

phlptp commented Jul 13, 2023

This would have the advantage over the custom object_category, in that you wouldn't be forced to override all the methods, just the ones you need.

@captainurist
Copy link
Contributor Author

OK got it. Will throw together a PR in a couple days. Thanks!

phlptp pushed a commit that referenced this issue May 2, 2024
Also works for overloads constrained with concepts or requirements.

Previously this wasn't working, which is a problem if you want to hook
some external serialization framework into CLI11 without painstakingly
going through all the types that said external framework supports.

This PR is related to #908.
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