Should enum_name(E value) (and maybe others?) be prevented from working with empty enums?

[ts826848]

I just finished helping a colleague track down some strange test failures that appear to stem from enum_name(E value) unintentionally behaving as if E were an empty enum. It took a bit to track down this specific issue because the code that actually used enum_name(E value) wasn't touched in the problematic commit. Some other unrelated shuffling of code appears to have confused the compiler so it started treating the enum in question as a plain forward declaration rather than its full definition, so enum_name(E value) was returning empty strings instead of the correct names.

This got me thinking - should enum_name(E value) work at all if the enum is empty? enum_name<auto V>() has a static_assert to bail if the returned name is empty, so changing enum_name(E value) to bail on empty enums and/or empty names would arguably make the behaviors match and could help catch unintentional changes like this where a previously-defined enum starts being treated as a forward declaration.

I think this question could also be extended to other parts of what magic_enum offers, but enum_name is definitely the place where I've experienced issues the most.

And if enum_name(E value) should work with empty enums, should it return a std::optional<std::string_view> to distinguish between the "no name due to invalid value/empty enum" and "empty name" cases?

[ts826848]

For full disclosure - my current guess for the root cause was that the refactoring accidentally introduced an ODR violation. The code that was pulled out was missing the include for the enum, so enum_name(E value) was treating it as an empty enum in that TU. This conflicts with the definition in the symptomatic TU, and the linker (?) ends up pulling in the empty-enum definition into the symptomatic TU.

Having enum_name reject empty enums would have helped catch this much earlier, and would also have pointed me to the problematic TU, which would have made debugging this much faster.

[Neargye]

Hi,

1. I think that adding asserts is the easiest option without breaking backward compatibility (like return std::optional<std::string_view>)

2. I’m also thinking of adding settings via macros(?) to disable these asserts.

3. I think it's worth adding this to all APIs.

What do you think about it?

[ts826848]

I think that approach sounds reasonable, especially if backwards compatibility is a concern. Macros to disable the asserts are a nice suggestion as well.

One alternative I can think of is a custom override similar to is_flags (is_empty?). That way the library can generally assume non-empty enums are used, but end users can override that assumption for specific empty enums so they still get the sanity checks for other enums in their code. I think this should still catch accidental use of forward-declared enums, so I don't believe it sacrifices anything.

Edit: Fixed mangling from email reply

[thirtythreeforty]

I am doing something similar on purpose:

    // Forward declare (values are intentionally deferred until after the
    // names TESTs are defined):
    enum MyModuleTapNames: uint32_t;

    // later, in the .cpp file:
    enum MyModuleTapNames {
        Foo,
        Bar,
    };

This allows me to hide the definitions from all translation units except the one that cares about it, and erase type information to generically have a system to grab enum names and counts from each translation unit.

should enum_name(E value) work at all if the enum is empty

We should distinguish here between enum MyModuleTapNames {}; and enum MyModuleTapNames;; the latter type is still incomplete and I think it should be rejected. (Or, provide an API to check if it is complete - which is how I found this issue :) But the former type has been completely defined to have no entries and I think it should be accepted.

If there's some technical restriction that prevents distinguishing the two cases, then let me know.

[ts826848]

If there's some technical restriction that prevents distinguishing the two cases, then let me know.

At least based on some cursory searches it seems a fully conforming generally-useable is_complete template is not possible to implement. The tricky part is that template instantiations using the same type(s) need to behave the same, otherwise you (can?) get ODR violations with unpredictable results (like the behavior that spawned this issue :P). In addition, the behavior of such a thing may be counterintuitive if you have is_complete<T> followed by a definition of T followed by another is_complete<T>. You may be able to get around this by limiting instantiations to a single TU and/or by using __COUNTER__, but the latter isn't technically conformant and I'm not confident the former is guaranteed to work, especially if present in a header file.

That's part of why I suggested an is_empty flag - to allow magic_enum to distinguish between "this enum is intentionally empty" and "this enum may be empty or forward-declared and I can't tell". Of course, if there is a way to detect a forward-declared enum that solution seems moot.

[Neargye]

@ts826848 @thirtythreeforty Please check this PR for possible solution https://github.com/Neargye/magic_enum/pull/323

[thirtythreeforty]

This partially works for me. It does correctly reject a declaration like

enum class MyEnum;

But, it does not reject a declaration that includes the underlying type:

enum class MyEnum: uint32_t;

[ts826848]

Please check this PR for possible solution https://github.com/Neargye/magic_enum/pull/323

The PR catches the error that I originally ran into, so I think it works for me. I think it should work for most others users as well; the only exception would be if a user intentionally has both empty and non-empty enums but prefers to keep the checks for non-empty enums if possible. I don't know whether that case will come up at all, though, and I think allowing checks to be done on a case-by-case basis should be fairly straightforwards.

I've left a few comments on the PR as well, but they're pretty much nitpicks.

But, it does not reject a declaration that includes the underlying type:

That's strange. The check is just std::is_enum_v<E> & (count_v<E, S> > 0), so it's not immediately obvious to me why the presence/absence of a declared underlying type should matter, especially for a scoped enum. I can't reproduce this behavior using either Clang or GCC as well. My test case, run in a file in the repo root:

#include "include/magic_enum/magic_enum.hpp"

enum class Forward : uint32_t;
auto f(Forward f) { return magic_enum::enum_name(f); }

I get a static_assert whether Forward has a declared type or not.

[thirtythreeforty]

Ah, sorry. You are correct. I was relying on CLion's static analysis rather than running a build. I see your described behavior on Clang 16 and GCC 13.2. So it is working for me! Thank you!