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

[BUG]: enum should not be silently converted to float in method call #5020

Open
3 tasks done
codeinred opened this issue Feb 6, 2024 · 2 comments
Open
3 tasks done
Labels
triage New bug, unverified

Comments

@codeinred
Copy link

codeinred commented Feb 6, 2024

Required prerequisites

What version (or hash if on master) of pybind11 are you using?

v2.11.1

Problem description

Enums (including enum class) will be implicitly converted to ints or floats during method calls. This behavior is surprising and can lead to problems where the wrong function is selected during overload resolution.

There should be a way to disable implicit conversion of enums to other types.

Consider the following case:

PYBIND11_MODULE(cmake_example, m) {
    py::enum_<Color>(m, "Color")
        .value("RED", Color::RED)
        .value("GREEN", Color::GREEN)
        .value("BLUE", Color::BLUE);

    m.def("f", [](double, double) { return "f(double, double)"; });
    m.def("f", [](double, Color) { return "f(double, Color)"; });
}

Calling f(0, Color.RED) from python incorrectly calls the f(double, double) overload. If the Color enum were not implicitly convertible, this would not occur.

Backwards Compatibility Concerns

I understand that there may be concerns about backwards compatibility. If these concerns are significant enough, then there should at least be a way to opt out of implicit conversion for enums

Reproducible example code

Full example code (C++):

#include <pybind11/pybind11.h>

namespace py = pybind11;

enum class Color { RED = 0, GREEN = 1, BLUE = 2 };

int add_int(int i, int j) { return i + j; }
double add_float(double x, double y) { return x + y; }

PYBIND11_MODULE(cmake_example, m) {
    m.def("add_int", &add_int);
    m.def("add_float", &add_float);

    py::enum_<Color>(m, "Color")
        .value("RED", Color::RED)
        .value("GREEN", Color::GREEN)
        .value("BLUE", Color::BLUE);

    /// Example of overload failure - calling f(0, Color) in python
    /// returns "f(double, double)" due to implicit conversion
    m.def("f", [](double, double) { return "f(double, double)"; });
    m.def("f", [](double, Color) { return "f(double, Color)"; });
}

Full example code (Python):

import cmake_example as m

# Probable bug - m.Color should _not_ be implicitly convertible to int
# Both of these should throw an invalid argument exception
print(m.add_int(1, m.Color.BLUE))     # Calls add_int, prints 3
print(m.add_float(0.5, m.Color.BLUE)) # Calls add_float, prints 2.5

# Overload case
print(m.f(0.0, m.Color.BLUE)) # Calls f(double, Color) (CORRECT)
print(m.f(0, m.Color.BLUE))   # Calls f(double, double) (INCORRECT)

Output of python:

3
2.5
f(double, Color)
f(double, double)

Is this a regression? Put the last known working version here if it is.

Not a regression

@codeinred codeinred added the triage New bug, unverified label Feb 6, 2024
@codeinred
Copy link
Author

Based on this test code it seems like implicit conversion to int is intended behavior.

def test_enum_to_int():
    m.test_enum_to_int(m.Flags.Read)
    m.test_enum_to_int(m.ClassWithUnscopedEnum.EMode.EFirstMode)
    m.test_enum_to_int(m.ScopedCharEnum.Positive)
    m.test_enum_to_int(m.ScopedBoolEnum.TRUE)
    # ...

This is fine, but

  • Implicit conversion to float is NOT intended behavior,
  • There should be a way to opt out of implicit conversion for an enum altogether (implementing this may be easier than fixing implicit conversion to float)

@codeinred codeinred changed the title [BUG]: enum should NOT be implicitly converted to int, float in method call [BUG]: enum should not be silectly converted to float in method call Feb 14, 2024
@codeinred codeinred changed the title [BUG]: enum should not be silectly converted to float in method call [BUG]: enum should not be silently converted to float in method call Feb 14, 2024
codeinred added a commit to codeinred/pybind11 that referenced this issue Feb 14, 2024
Enums (including enum class) are silently converted to floats during
method calls. This can cause overload resolution to fail.

I believe this behavior is a bug. This commit adds tests for that
behavior. These tests fail right now, but they should pass once the
behavior is fixed.

See: pybind#5020
codeinred added a commit to codeinred/pybind11 that referenced this issue Feb 14, 2024
Enums (including enum class) are silently converted to floats during
method calls. This can cause overload resolution to fail.

I believe this behavior is a bug. This commit adds tests for that
behavior. These tests fail right now, but they should pass once the
behavior is fixed.

See: pybind#5020
@EmilDohne
Copy link

Hey, I just wanted to check if theres an update on this in regards to disabling the implicit conversion from enum to int since I am running into the same problem where I want two separate constructors:

template <typename T>
constructLayer(std::unordered_map<int, py::array_t<T>>& channel_mapping);

template <typename T>
constructLayer(std::unordered_map<Enum::ChannelID, py::array_t<T>>& channel_mapping);

but it appears to always call the first ctor even when calling it with these args from python

# Passing these to the ctor always calls the int overload
image_dict = {}
image_dict[0] = np.full((height, width), 255, np.uint8)
image_dict[1] = np.full((height, width), 0, np.uint8)
image_dict[2] = np.full((height, width), 0, np.uint8)

image_dict_id = {}
image_dict_id[psapi.enum.ChannelID.red] = np.full((height, width), 255, np.uint8)
image_dict_id[psapi.enum.ChannelID.green] = np.full((height, width), 0, np.uint8)
image_dict_id[psapi.enum.ChannelID.blue] = np.full((height, width), 0, np.uint8)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage New bug, unverified
Projects
None yet
Development

No branches or pull requests

2 participants