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

Cannot use constants as enum values #94

Closed
OpenByteDev opened this issue Jan 10, 2023 · 5 comments · Fixed by #96
Closed

Cannot use constants as enum values #94

OpenByteDev opened this issue Jan 10, 2023 · 5 comments · Fixed by #96

Comments

@OpenByteDev
Copy link

OpenByteDev commented Jan 10, 2023

Since the update from 0.5.7 to 0.5.8 using a constants as the value of an enum variant does not work anymore:

const CONST_ONE: u32 = 1;
#[derive(
    TryFromPrimitive, IntoPrimitive
)]
#[repr(u32)]
pub enum Enum {
    One = CONST_ONE,
}

Produces the following error:

error: Expected literal
  --> src\error.rs:58:11
   |
58 |     One = CONST_ONE,
   |           ^^^^^^^^^
@kevinaboos
Copy link

kevinaboos commented Jan 10, 2023

Confirmed, we're facing the same issue in Theseus OS. Previously we were on 0.5.7, and some of our CI passes randomly started failing within the last 2 hours due to num_enum 0.5.8 breaking this. (Ultimately our mistake for not freezing/locking the lockfile versions used for doc builds)

I think 0.5.8 likely needs to be yanked because it somehow broke semver compatibility guarantees.

@kevinaboos
Copy link

This is likely the culprit:

_ => die!(val_exp => "Expected literal"),

@AlexDBlack
Copy link

AlexDBlack commented Jan 10, 2023

rdkafka also appears to be affected:
https://github.com/fede1024/rust-rdkafka/blob/master/rdkafka-sys/src/bindings.rs#L182

error: Expected literal
   --> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/rdkafka-sys-4.3.0+1.9.2/src/bindings.rs:182:32
    |
182 |     RD_KAFKA_RESP_ERR__BEGIN = -200,
    |                                ^^^^
error[E0277]: the trait bound `rd_kafka_resp_err_t: From<i32>` is not satisfied

We have CI builds failing just now with num_enum 0.5.8 that previously passed with 0.5.7.

Edit: manually pinning the version appears to be a workaround.

num_enum = "=0.5.7"
num_enum_derive = "=0.5.7"

@Dhghomon
Copy link

Yes, looks like this was the change that did it! I also started with a cryptic error in rdkafka which hasn't seen any changes for quite some time and eventually noticed the derive trait their error enum uses which led me to here.

Love the macro name there 😂

@illicitonion
Copy link
Owner

I've yanked 0.5.8 - thanks for the reports, and sorry for the breakage!

illicitonion added a commit that referenced this issue Jan 15, 2023
While preserving favouring literal values where possible.

Fixes #94 

The story here is:
#89 tried to simplify the generated code (which, among other things, made rustc not stack overflow when codegening for large enums).
Unfortunately, it assumed that all explicit discriminants were literals (and non-negative ones, as `-123` parses as a `UnaryExpr` not a `Literal`).
And further unfortunately, because of a typo in a `#[cfg(feature)]` attached to the only tests we had for non-literal enums, we weren't running those tests (this PR re-enables them and adds some which aren't feature gated, and #95 will ensure we don't regress in this way again).

This PR attempts to preserve the "prefer just using literals rather than large chains of wrapping adds" property of #89, while also supporting non-literal cases.
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

Successfully merging a pull request may close this issue.

5 participants