Clear constant values generated code, defined behavior with the alternative variants, add error messages. #89
Conversation
…ative values, added errors messages
|
Ah, I just saw in the diff that the |
|
Here an example on how it expands: #[derive(Copy, Clone, Debug, PartialEq, Eq, TryFromPrimitive)]
#[repr(u8)]
pub enum EncodingFormat {
Postcard = 3,
Json,
MessagePack,
#[num_enum(alternatives = [9,55,7])]
#[num_enum(default)]
Pickle,
#[num_enum(alternatives = [1, 2])]
BSON,
CBOR = 200,
CSV,
Bencode,
}Expands to: impl ::num_enum::TryFromPrimitive for EncodingFormat {
type Primitive = u8;
const NAME: &'static str = "EncodingFormat";
fn try_from_primitive(
number: Self::Primitive,
) -> ::core::result::Result<Self, ::num_enum::TryFromPrimitiveError<Self>> {
#![allow(non_upper_case_globals)]
const Postcard__num_enum_0__: u8 = 3;
const Json__num_enum_0__: u8 = 4;
const MessagePack__num_enum_0__: u8 = 5;
const Pickle__num_enum_0__: u8 = 6;
const Pickle__num_enum_1__: u8 = 7;
const Pickle__num_enum_2__: u8 = 9;
const Pickle__num_enum_3__: u8 = 55;
const BSON__num_enum_0__: u8 = 8;
const BSON__num_enum_1__: u8 = 1;
const BSON__num_enum_2__: u8 = 2;
const CBOR__num_enum_0__: u8 = 200;
const CSV__num_enum_0__: u8 = 201;
const Bencode__num_enum_0__: u8 = 202;
#[deny(unreachable_patterns)]
match number {
Postcard__num_enum_0__ => ::core::result::Result::Ok(Self::Postcard),
Json__num_enum_0__ => ::core::result::Result::Ok(Self::Json),
MessagePack__num_enum_0__ => {
::core::result::Result::Ok(Self::MessagePack)
}
Pickle__num_enum_0__
| Pickle__num_enum_1__
| Pickle__num_enum_2__
| Pickle__num_enum_3__ => ::core::result::Result::Ok(Self::Pickle),
BSON__num_enum_0__ | BSON__num_enum_1__ | BSON__num_enum_2__ => {
::core::result::Result::Ok(Self::BSON)
}
CBOR__num_enum_0__ => ::core::result::Result::Ok(Self::CBOR),
CSV__num_enum_0__ => ::core::result::Result::Ok(Self::CSV),
Bencode__num_enum_0__ => ::core::result::Result::Ok(Self::Bencode),
#[allow(unreachable_patterns)]
_ => ::core::result::Result::Ok(EncodingFormat::Pickle),
}
}
} |
There was a problem hiding this comment.
This looks great to me, thanks so much for taking the time!
I just merged #86 which, if you rebase or merge main, should get CI green where it's failing.
There's also a few minor clippy suggestions you can view on the "Files changed" tab.
|
I merged #86 and fixed the code with the clippy suggestion. Now, when I run the tests, all passed except the But those tests are not linked to what I did so I am confuse on why it fails... UpdateIt was from rustc itself ! See in the Reference Custom Discriminant Values for Fieldless Enumerations:
In some of the tests we got I updated my toolchain to 1.66.0 an now it works without the error[E0658] from rustc. So it's all good now.Here's a summary output (in 1.65.0):```bash test tests\try_build\pass\default_and_alternatives.rs ... ok test tests\try_build\compile_fail\catch_all_multiple_fields.rs ... mismatch test tests\try_build\compile_fail\catch_all_non_tuple.rs ... ok test tests\try_build\compile_fail\conflicting_default.rs ... ok test tests\try_build\compile_fail\default_and_catch_all_alt.rs ... mismatch test tests\try_build\compile_fail\default_and_catch_all_same_variant.rs ... mismatch test tests\try_build\compile_fail\default_and_catch_all_same_variant_alt.rs ... mismatch test tests\try_build\compile_fail\exhaustive_enum.rs ... ok test tests\try_build\compile_fail\multiple_defaults.rs ... ok ```Here's the detailed output (in 1.65.0):```bash >cargo test --test try_build Finished test [unoptimized + debuginfo] target(s) in 0.05s Running tests\try_build.rs (target\debug\deps\try_build-6366579ee0618747.exe)running 1 test test tests\try_build\pass\default_and_alternatives.rs ... ok test tests\try_build\compile_fail\catch_all_multiple_fields.rs ... mismatch EXPECTED: ACTUAL OUTPUT: error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants test tests\try_build\compile_fail\catch_all_non_tuple.rs ... ok EXPECTED: ACTUAL OUTPUT: error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants test tests\try_build\compile_fail\conflicting_default.rs ... ok EXPECTED: ACTUAL OUTPUT: error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants test tests\try_build\compile_fail\default_and_catch_all_alt.rs ... mismatch EXPECTED: ACTUAL OUTPUT: error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants test tests\try_build\compile_fail\default_and_catch_all_same_variant.rs ... mismatch EXPECTED: ACTUAL OUTPUT: error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants test tests\try_build\compile_fail\default_and_catch_all_same_variant_alt.rs ... mismatch EXPECTED: ACTUAL OUTPUT: error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants test tests\try_build\compile_fail\exhaustive_enum.rs ... ok EXPECTED: ACTUAL OUTPUT: error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants test tests\try_build\compile_fail\multiple_defaults.rs ... ok test trybuild ... FAILED |
Yeah, this is an unfortunate corner based on how we do CI :( Our test-cases aren't pulled in or used by any callers, so we don't generally need to worry about compatibility as far as users are concerned. As rust evolves, sometimes we need to update our test-cases (e.g. when compiler error text changes, because we'd rather have clear-to-understand-and-debug tests). For this particular breakage, unfortunately, you can't (easily) conditionally enable compiler features, so when a feature stabilised, we altered the tests to be compatible with the current stable (because on CI we test against stable, beta, and nightly). If this was a change that affected our production code compatibility, we probably would've gone to some efforts to make sure the minimum rust version we support still ran the tests (and honestly, maybe we should anyway), but realistically that would mean forking our test-suite for different versions of the compiler, which is probably more effort than it's worth :) |
|
@GilShoshan94 I pushed a test fix and a couple of testing improvements which were missing from the repo before your PR - let me know if you're happy and I'll merge this for you - thanks again for taking the time, and for paying such close attention to everything! |
|
@illicitonion, I looked at your commits on the tests, it's good. Sorry I didn't fix the test first, I had to go somewhere and was eager to push the code already for you to see (I saw just after that one test had a slighlty different error output but had to run). Now it's good. Was great to collaborate with you and to make a contribution :) ! You can merge this (I guess the package version need to be patch as well ?). Will I be added to the contributors automatically once this is merged ? |
|
I've just released 0.5.8 including this change (along with a few other non-user-facing things) - thanks again for your contribution! We don't formally track contributors in a file in the project or anything like that, but you should now have a "Contributor" badge when you comment on issues or PRs in this repo:
Please let me know if there's something else you were asking about! |
|
That's perfect! That's what I wanted, to be a contributor as well (badge) . I didn't have a lot of experience with Github and was wondering how to be added as a contributor. Now I see it is automatically done once a PR is merged. Was a pleasure to collaborate on an open source project, especially one popular in the Rust community! 😁 And I learned a lot about proc macros. |
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.
Hi @illicitonion ,
This time I did a better job I think !
I learn how to write proc macro :).
It passed all the tests excepted this one :
try_build/compile_fail/unreachable_patterns.rs.It's because I added some errors messages myself. In my opinion, my error is more accurate and will provide a better user experience.
I updated the test as well then.
Beside that, here is what I did:
literalfunction as I use it and wanted to cover the widest range of #repr[x].fn expr_to_int(val_exp: &Expr) -> Result<i128>that permits two things:a) To reports errors on expected integer literal. (try to put a float in the alternatives or in the discriminant).
b) To works with integers and keep track of them, and to expand to integers directly after.
val_set).alt_attr_ref), this is just for error reporting to keep the span around.a) Check for collision in the case of specified explicit discriminant.
b) Check if the current discriminant is not in the alternative values.
c) Check the presence of duplicates in the alternative values.
d) Check if those alternative values where already attributed.
val_set.Increment from the last canonical discriminant but automatically skip on the taken values.
That's it.
Let me know what you think.