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

Uncaught Error: None value in Pybind enum makes pyi contain syntax errors #258

Open
parker-research opened this issue Dec 3, 2024 · 3 comments

Comments

@parker-research
Copy link
Contributor

Take, for example, the following Pybind C++ enum:

    py::enum_<EvalFlags>(m, "EvalFlags")
        .value("None", EvalFlags::None)
        .value("IsScript", EvalFlags::IsScript)
        .value("CacheResults", EvalFlags::CacheResults)

The output of the type stub generation is:

import enum

class EvalFlags(enum.Enum):
    None = 0
    IsScript = 1
    CacheResults = 2

This is a syntax error, because None is a protected keyword in Python. Instead, during the stub generation/code parsing, an error should be thrown saying that "None" is not a valid enum key. It could even suggest renaming to None_, as is common.

@mosra
Copy link
Owner

mosra commented Dec 3, 2024

I got bitten by this quite a few times myself as well, one of the cases was using from as a method name.

Adding warnings for names that clash with builtin types is definitely a possibility, but I feel that if it's discovered only during docs / stub generation it's already too late (especially because such generation might happen with a significant delay after the bindings are written), and a better place to tackle this would be in pybind itself? One could of course do things like

getattr(EvalFlags, "None")

to access the attribute, so technically it's fine if pybind produces such a code, but I don't see much of a practical value of that. I didn't manage to find any related issues on the pybind repo and I don't think we're the only ones facing this problem, so maybe it'd be worth to ask their opinion on this?

@parker-research
Copy link
Contributor Author

Discussion on how it may technically be possible to avoid this change, but why it's a horrible idea to hack a workaround - https://stackoverflow.com/questions/38773832/is-it-possible-to-add-a-value-named-none-to-enum-type

It would be nice if the generated stubs were reliably valid syntax. Not urgent, but I'd suggest leaving this issue open so someone can fix it eventually.

@mosra
Copy link
Owner

mosra commented Dec 3, 2024

I get what you're saying, but I really feel that when the documentation generator complains, it's too late, and this would best be handled by pybind11 directly. Best for everyone using pybind11, regardless of whether they deal with m.css or generating stubs, because such names cause the bindings next-to-impossible to use.

On the other hand, the doc/stub generator is already printing warnings if it sees strange C++ types in annotations -- because, somehow, pybind11 is unable to robustly prevent those as well. So this would be another pybind11-specific warning.

Any ideas how to make this robust, without having to duplicate name safety checks to several places? Because such a name can be in a module, in a class, in a variable, in a property, in a function, in a function argument, in an enum, ... It seems that I can use keyword.iskeyword() for checking name validity, so that part is easy at least.

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

2 participants