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

Rewrite most of the crate with updated dependencies #9

Merged
merged 42 commits into from
Jul 18, 2018
Merged

Rewrite most of the crate with updated dependencies #9

merged 42 commits into from
Jul 18, 2018

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Jul 7, 2018

Closes #8

So I'm basically done with rewriting everything. Of course I didn't just rewrite it (just) for fun, but to update syn, quote and proc_macro. We now use the following versions (the newest as of time of writing):

  • proc-macro2 0.4.6
  • quote 0.6.3
  • syn 0.14.4

The new implementation can now compile the compile_test examples (and more). I guess I reached feature parity. I am not aware that I changed anything about how this crate works. The attribute syntax is the same as before, and I generate the same impl blocks as before (again, as far as I am aware).

I think the old and new implementations are somewhat different:

  • The new implementation uses much less own data structures and instead uses syn structures to pass information around.
  • Most functionality in the new version is implemented in free functions which usually receive syn data structures and return a token stream.
  • The new implementation uses proc_macro::TokenStream instead of the proc_macro2 version where possible in a useful way.

I managed to reduce total line count despite my code having more comments and adding two examples. I guess this is simply due to more ergonomic APIs by syn and proc_macro.


As far as I can tell, I improved diagnostics in the new version. Example:

image


Notes for the Review

I know this is a pretty big PR and reviewing it thoroughly might not be possible, because it would take too long. But I'd appreciate it if you could take a quick look and maybe tell me if anything is still unclear. I can always add more explanations in comments if necessary.


Things I plan on doing after this PR has been merged

(I'd create an issue for each of those points)

  • Make this work with in-band lifetimes (there is a TODO comment about this in the code)
  • Improve the test suite, in particular:
    • More unit tests
    • More compile_test tests
    • Add compile-fail tests
    • Make cargo test execute the compile-test and compile-fail tests
  • Create better documentation and update the README, if necessary
  • Improve names for the lifetime and type parameter we introduce (just take T or something simple if it's not taken yet)
  • A subset of the proc-macro API will be stabilized for Rust 2018 (see rustc: Stabilize the proc_macro feature rust-lang/rust#52081). Unfortunately, the API in proc_macro that allows the nice kind of diagnostics shown above won't be stabilized by that PR and will thus probably remain unstable for a while. We probably should talk about whether we want this to work on stable Rust 2018 or not. Of course, the best solution would be to use awesome diagnostics on nightly and fall back to a less nice solution on stable. But I'm not yet sure how difficult it would be to use such a fallback mechanism.
  • Possibly more, but I can't think of anything I missed...

Some tests sadly still fail due to a bug/missing feature of
proc-macro.
It ignores generics of the trait and doesn't generate code for
trait items yet.
This is just for testing to see what approach will result in
better and shorter code.
There are probably still bugs, but it looks good for now. Additionally,
a bunch of documentation was added.
The Fn-type is not really generated yet, but sanity checks
are done.
@KodrAus
Copy link
Member

KodrAus commented Jul 9, 2018

Hi @LukasKalbertodt!

Thanks so much for taking this on! I'll set some time aside this week to go through it all. I'm really keen to see how it all comes out using the newer APIs, I think auto_impl is a pretty nice use-case for showing off proc macros.

The feature `proc_macro` was recently stabilized [1]. This has two effects:
- The `proc_macro` feature gate is replaced by finer grained feature gates
- Using this crate now doesn't need `#![feature(proc_macro)]` but
  `#![feature(use_extern_macros)]`

[1]: rust-lang/rust#52081
@LukasKalbertodt
Copy link
Member Author

@KodrAus I updated the code in response to rust-lang/rust#52081.

After this PR is merged, it's time to talk about what nightly features we really want to use. As far as I understand it, the status from the linked PR is what will "land" in Rust 2018. This excludes nice diagnostics (which I use in this PR) and some Span methods, most importantly def_site() for def site hygiene (which I currently don't use, but I think should be used by this crate).

As I probably already said somewhere, I think it's possible to add a nightly feature to this crate which enables better diagnostics and let's this crate avoid the ugly __AutoImplProxyT hack (which I intend to make less ugly, but ... using proper hygiene is better IMO). Without the nightly feature, this crate should be able to compile on stable. That would be pretty neat, as I agree with you that this crate is a nice example crate to show off proc macros!

@KodrAus
Copy link
Member

KodrAus commented Jul 18, 2018

@LukasKalbertodt Thanks for your patience on this. I just moved house over the weekend and tried to get to this before then, but missed the boat. This is looking nice and clean! You've done an awesome job. I'm happy to get this merged.

As for the nightly feature, I really like the idea of getting this working on stable. I've fallen a bit behind on the state of proc macros recently, where do you expect the seams to be? Would the nightly feature mostly just be adding in support for missing features without deeply affecting the way things are structured?

@KodrAus KodrAus merged commit bd1d966 into auto-impl-rs:master Jul 18, 2018
@LukasKalbertodt
Copy link
Member Author

Awesome, thanks for taking a look and merging!

Would the nightly feature mostly just be adding in support for missing features without deeply affecting the way things are structured?

I think it should be possible to add nightly features on top without restructuring everything, yes. Right now it's mainly Diagnostics anyway. For the stable version we could just define our own Diagnostics type that simply panics to show the error. So yeah, I think it would work fine.


So I'll probably make a plan of things I want to do in the next couple of weeks. I'll create issues for everything that is worth discussing (like the two issues I already opened). I hope you don't mind me creating issues and maybe a milestone on your repo?

@LukasKalbertodt LukasKalbertodt added this to the v0.3 milestone Jul 20, 2018
@KodrAus
Copy link
Member

KodrAus commented Jul 22, 2018

I'll create issues for everything that is worth discussing (like the two issues I already opened). I hope you don't mind me creating issues and maybe a milestone on your repo?

Please go right ahead! At this stage I think it's fair to say you've got a much clearer idea of where this crate should go than I do.

@LukasKalbertodt LukasKalbertodt deleted the rewrite branch July 25, 2018 09:52
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 this pull request may close these issues.

2 participants