Fix `num-macros` `FromPrimitive` implementation by hauleth · Pull Request #228 · rust-num/num

hauleth · 2016-09-18T18:58:37Z

Current solution follow syntax proposed in rust-lang/rfcs#1681.

Close #227

Code broken after change:

hauleth · 2016-09-18T19:09:07Z

Unfortunately due to bug dtolnay/syn#12 this syntax will currently fail:

#[derive(FromPrimitive)]
enum Foo {
    A = 1,
    B
}

hauleth · 2016-09-18T19:16:25Z

cc @cuviper

cuviper · 2016-09-18T19:53:42Z

Does the existing code work with that problem case? (Apart from the current nightly break.). If so, I think we should try to fix that break and add a test like that. Then when syn releases a fix for that bug, we can land this PR.

hauleth · 2016-09-18T19:55:55Z

Existing code doesn't work at all as there was breaking change in compiler plugins (again) and that made num-macros uncompilable. Current version compile and works except that one case.

cuviper · 2016-09-18T19:57:58Z

That's what I meant by the current nightly break. Can we fix that without this whole rewrite, then switch when syn is ready?

hauleth · 2016-09-18T19:59:51Z

I haven't written compiler plugin even once so I am not sure if I can fix this. About this issue I can use syntex crate but this will significantly increase compile times for num-macros.

bluss · 2016-09-18T20:02:42Z

Syntax extensions are annoying for now that they break so often, but the fixes are usually very simple to make.

hauleth · 2016-09-18T20:06:59Z

@bluss probably it is just fixing the name, but since rust-lang/rfcs#1681 was accepted I think we should jump into that train and use Macros 1.1 syntax. As you can see in the PR it makes whole code much simpler. The only problem is that syn crate need a little fix (which I cannot make because I am not the nom wizard and I always forget how to write parsers in it. However it should be also quite simple task.

hauleth · 2016-09-18T20:08:23Z

We can always drop syn and use syntex instead, but AFAIK syntex compile times aren't the best.

bluss · 2016-09-18T20:21:12Z

Not disagreeing about the plan for the future, but fixing the num-macros nightly issue seems like the reasonable first thing to do right now.

bluss · 2016-09-18T20:25:54Z

#229 is the fix

hauleth · 2016-09-18T20:33:00Z

So in sake of future changes:

this PR rename #[derive(NumFromPrimitive)] to more natural #[derive(FromPrimitive)]
user should use #![feature(rustc_macro)] instead of #![feature(custom_derive, plugin)]
user should use #[macro_use] extern crate num_macros; instead of #![plugin(num_macros)]

In future it should also be allowed to use #[macro_use] extern crate num; and also be able to use custom derives.

homu · 2016-09-18T20:34:18Z

☔ The latest upstream changes (presumably #229) made this pull request unmergeable. Please resolve the merge conflicts.

Current solution follow syntax proposed in rust-lang/rfcs#1681. Tracking issue rust-lang/rust#35900 Close #227

cuviper · 2016-09-19T16:36:20Z

BTW, how did you get that "code broken after change" list? Is crater available to us now?

hauleth · 2016-09-19T17:00:29Z

@cuviper no, just GitHub search and copy&paste 😄

dtolnay · 2016-09-24T02:42:25Z

I fixed the syn enum parsing issue in 0.6.0. Sorry for the slow response - I was away without internet access for a week.

bluss · 2016-09-26T06:50:27Z

cargo-crusader was kind of a bare bones "crater" for rdeps, but it errors when I try to use it nowadays. Not sure if there was a crates.io api change or what happened.

cuviper

Is your list of breakages solved by the new syn, or are there still other problems?

We should do better about rejecting enums with data in them.

The old code also rejected empty enums, which are weird but sometimes found in FFI for opaque structs.

cuviper · 2016-09-27T08:06:42Z

+            if let Some(val) = variant.discriminant {
+                idx = val.value;
+            }
+            let tt = quote!(#idx => Some(#name::#ident));


Remember that enums can have data too, but FromPrimitive only makes sense for integral enums. It looks like that's where Variant.data is VariantData::Unit. If any variant is something else, we need to reject the whole thing.

For instance, try adding something like Other(u8, u8, u8) to one of the Color tests. It does get an error, but it's a very confusing one:

error[E0308]: mismatched types --> tests/trivial.rs:18:1 | 18 | enum Color { | ^ expected enum `Color`, found fn item | = note: expected type `Color` = note: found type `fn(u8, u8, u8) -> Color {Color::Other}`

The derivation tried to output Some(Color::Other) here, but that's actually a type constructor, not a value in itself, which is why the error is talking about a fn. Similarly if it were Other{ r:u8, g:u8, b:u8 } you get the error struct called like a function. We should catch this up front to identify the root problem.

We could use compiletest-rs to have some negative tests.

Oh yeah, I have forgotten about that.

Added better error message and compiletest-rs crate to test against invalid code.

cuviper · 2016-09-27T08:11:53Z

-plugin = true
+[dependencies]
+quote = "0.1.3"
+syn = "0.6.0"


There's even 0.7 now. Don't know if it matters to us, but we probably should try to keep up.

Don't bother trying to keep up for now. I am sprinting to get serde codegen working with syn (serde-rs/serde#548) so there may be another break coming up. When serde is done I will make a PR here bumping the version and fixing anything I broke (although I doubt there would be anything affecting the code in this PR). Also starting with the next release I will start doing proper release notes explaining the changes.

cuviper · 2016-09-27T08:37:15Z

OK, thanks for telling us!

dtolnay · 2016-09-27T08:37:46Z

+[lib]
+crate-type = ["rustc-macro"]
+name = "num_macros"
+rustc-macro = true


From what I have seen in serde, this line is sufficient. You don't need crate-type = ["rustc-macro"] and #![crate_type = "rustc-macro"]. Is there a reason to have those?

None, but it isn't harmful in any way and this helps with visibility (you do not need to check Cargo.toml for crate type, it is already there).

hauleth · 2016-09-27T12:27:00Z

@cuviper all done.

dtolnay · 2016-09-27T15:15:51Z

I haven't reviewed carefully but I just want to say what a massive improvement in readability of lib.rs, nicely done. Once this merges I will link to this from the syn and quote readmes as an example of how to do things right.

cuviper · 2016-09-27T18:38:17Z

You didn't answer my question about the "Code broken after change" status, but I suppose they must still be broken since you changed the derivation name. Let's back that change out and save it for 0.2, so we can merge the syn rewrite now.

cuviper · 2016-09-27T18:41:51Z

Or if you really want the new name now, add a derivation by the old name too for compatibility.

hauleth · 2016-09-27T18:42:38Z

@cuviper it is and will be broken even if I keep the name as "requiring" syntax has changed.

Ex.

Old

#![features(plugin, custom_derive)]
#![plugin(num_macros)]

extern crate num;

#[derive(FromPrimitive)]
enum Foo { Bar }

// ...

New

#![features(rustc_macro)]

#[use_macros] extern crate num_macros;
extern crate num;

#[derive(FromPrimitive)]
enum Foo { Bar }

So either way it is still breaking change.

dtolnay · 2016-09-27T18:53:33Z

For serde we decided to keep "serde_macros" as the feature(plugin) one and use "serde_derive" as the feature(rustc_macro) one. I think "derive" is a lot more evocative in terms of how the crate is used, and "macros" just happens to be an implementation detail of Rust.

cuviper · 2016-09-27T19:13:54Z

OK, I'd be fine with forking num_macros into a syn-based num_derive crate, and leave the former alone. That would also make it easier to show side-by-side for your syn case study. :)

hauleth · 2016-09-28T11:34:06Z

Ok. I have moved it to new crate num-derived and restored old num-macros.

cuviper · 2016-09-30T05:58:34Z

@homu r+ b7e6407

Fix `num-macros` `FromPrimitive` implementation Current solution follow syntax proposed in rust-lang/rfcs#1681. Tracking issue rust-lang/rust#35900 Close #227 Code broken after change: - [ ] https://github.com/kdar/gameboy-rs - [ ] https://github.com/lemonrock/mbedtls - [ ] https://github.com/timorantalaiho/kelaa - [ ] https://github.com/dylanede/cef-rs - [ ] https://github.com/klutzy/aheui-llvm - [ ] https://github.com/pcwalton/rust-media - [ ] https://github.com/PistonDevelopers/hematite_server

homu · 2016-09-30T05:58:53Z

⚡ Test exempted - status

Add a short example to `<Value as Index>::index`

hauleth added the breaking change label Sep 18, 2016

hauleth added 2 commits September 18, 2016 22:43

Fix num-macros FromPrimitive implementation

11f8289

Current solution follow syntax proposed in rust-lang/rfcs#1681. Tracking issue rust-lang/rust#35900 Close #227

Add test case for which syn currently fails

43cfa25

hauleth force-pushed the fix/num-macros branch from 39a9b55 to 43cfa25 Compare September 18, 2016 20:43

Rebase leftovers

e9768a0

hauleth modified the milestone: v0.2.0 Sep 18, 2016

hauleth added 2 commits September 26, 2016 20:32

Update to syn 0.6.0

ada17a1

Merge remote-tracking branch 'origin/master' into fix/num-macros

b038b79

cuviper requested changes Sep 27, 2016

View reviewed changes

dtolnay reviewed Sep 27, 2016

View reviewed changes

Add compiletest crate to test against invalid derivations

0c89b89

Add new crate num-derive as an new replacement for num-macro

b7e6407

cuviper approved these changes Sep 30, 2016

View reviewed changes

cuviper removed the breaking change label Sep 30, 2016

homu merged commit b7e6407 into master Sep 30, 2016

hauleth deleted the fix/num-macros branch October 1, 2016 19:16

remexre pushed a commit to remexre/num that referenced this pull request Jun 1, 2017

Merge pull request rust-num#228 from killercup/docs/index-examples

f5f27bf

Add a short example to `<Value as Index>::index`

Conversation

hauleth commented Sep 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hauleth commented Sep 18, 2016

Uh oh!

hauleth commented Sep 18, 2016

Uh oh!

cuviper commented Sep 18, 2016

Uh oh!

hauleth commented Sep 18, 2016

Uh oh!

cuviper commented Sep 18, 2016

Uh oh!

hauleth commented Sep 18, 2016

Uh oh!

bluss commented Sep 18, 2016

Uh oh!

hauleth commented Sep 18, 2016

Uh oh!

hauleth commented Sep 18, 2016

Uh oh!

bluss commented Sep 18, 2016

Uh oh!

bluss commented Sep 18, 2016

Uh oh!

hauleth commented Sep 18, 2016

Uh oh!

homu commented Sep 18, 2016

Uh oh!

cuviper commented Sep 19, 2016

Uh oh!

hauleth commented Sep 19, 2016

Uh oh!

dtolnay commented Sep 24, 2016

Uh oh!

bluss commented Sep 26, 2016

Uh oh!

cuviper left a comment

Choose a reason for hiding this comment

Uh oh!

cuviper Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hauleth Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

hauleth Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

cuviper Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

dtolnay Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

cuviper commented Sep 27, 2016 via email

Uh oh!

dtolnay Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

hauleth Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

hauleth commented Sep 27, 2016

Uh oh!

dtolnay commented Sep 27, 2016

Uh oh!

cuviper commented Sep 27, 2016

Uh oh!

cuviper commented Sep 27, 2016

Uh oh!

hauleth commented Sep 27, 2016

Ex.

Old

New

Uh oh!

dtolnay commented Sep 27, 2016

Uh oh!

hauleth commented Sep 18, 2016 •

edited

Loading

cuviper Sep 27, 2016 •

edited

Loading