-
Notifications
You must be signed in to change notification settings - Fork 30
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
init WIP add proc_macro generator generator (issue #2) #5
Conversation
src/lib.rs
Outdated
#![cfg_attr( | ||
feature = "nightly", | ||
feature(async_await, async_closure, proc_macro_hygiene) | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the proc_macro_hygiene
for the proc-macros to be used on closures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be needed if the closure macro is changed to a function-like macro
This is basically done a few cleanup things and documenting them fully. I'm not familiar with the github actions you have for testing if you could point me at what I should do to get them to pass. I added very basic tests for each
or any other ideas changes I'm all for it! |
Thanks for putting this together! Sorry I've been unresponsive – I was traveling for the holidays, but I'm back! Real quickly addressing your comments -
I'm out of time for tonight but I'll give the actual code a proper look-over tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That about covers it! Most of the comments are smaller nits, but the one about the visitor pattern might require reworking that part of the code a bit.
I ran out of time to look at GitHub actions tonight – I'll do that tomorrow.
Also one other comment about closures+stable, I realized my example above was totally wrong because it doesn't specify the type. This is viable (I believe), but also very ugly:
let closure = yielder_cls!(u8 in || {
yield_!(5);
});
Do you have any better ideas? I'm really bummed that expression attributes are not stable yet. Nothing I come up with looks anywhere near as good as your original design.
src/lib.rs
Outdated
#![cfg_attr( | ||
feature = "nightly", | ||
feature(async_await, async_closure, proc_macro_hygiene) | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be needed if the closure macro is changed to a function-like macro
I fixed most of the issues that are not related to or not solved by using Thanks for all the feed back I'm really excited about the |
Don't mention it, thanks for spending time on this! I like how it's shaping up. btw I think I fixed CI—if you rebase this PR onto master, it should start running the tests, fingers crossed. |
src/rc/mod.rs
Outdated
A macro attribute can be used for functions `rc_yield_fn`, and a function like macro | ||
for closures `rc_yield_cls`. These are meant to be used with the `yield_` macro for | ||
easy definition of generators. The crate must be compiled with the `proc_macro` | ||
feature for these to be enabled. | ||
```toml | ||
syn = {version = "0.2", features = ["proc_macro"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an example of possible documentation in one of the mods let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty good. I would take the prose and move it to the main module (so you're not copy/pasting text across all three modules) and leave just minimal code examples in the examples sections.
If you want, I'd be happy to handle writing the docs for this after we get this merged so you don't have to worry about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that'd be nice, keep it one consistent voice and way of explaining things. Cool that'd be nice thanks.
Sorry about all the pushes. The Again sorry about the mess I was fighting with the doc tests hopefully I got it! |
@@ -0,0 +1,81 @@ | |||
// Remove to test otherwise CI fails because of the async closures | |||
#[cfg(feature = "proc_macro_example")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs a feature gate. Since it's an example it doesn't matter if it fails to compile without the right features. (Actually, it might be worse if it compiles but does nothing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I couldn't get this file to even run without moving it up a level to the root of the repo. And it seems like there's a lot going on here, maybe this would be better off as a bunch of tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was planning on probably deleting it, distributing out some test would be even better.
genawaiter-proc-macro/src/common.rs
Outdated
} | ||
|
||
impl VisitMut for YieldMatchMacro { | ||
fn visit_stmt_mut(&mut self, node: &mut Stmt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big improvement over the last implementation! Unfortunately there are still more cases it doesn't cover, such as calling a method on the resume argument:
Details
#[test]
fn sync_proc_macro_fn() {
#[genawaiter::sync::sync_yield_fn(u8)]
async fn odds() {
for n in (1_u8..).step_by(2).take_while(|&n| n < 10) {
let cloned_resume_arg = genawaiter::yield_!(n).clone();
}
}
let gen = Gen::new(odds);
let res = gen.into_iter().collect::<Vec<_>>();
assert_eq!(vec![1, 3, 5, 7, 9], res)
}
I think to cover all your bases you'll need to think in terms of expressions (Expr
) instead of Stmt
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got to remove even more code sweet. That worked really well.
examples/await.rs
Outdated
// // Remove to test otherwise CI fails because of the async closures | ||
// #![feature(async_closure)] | ||
// #[cfg(feature = "proc_macro_example")] | ||
// mod mac { | ||
// fn stack_yield_fn() { | ||
// #[genawaiter::stack::stack_yield_fn(u8)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it in for now but I will delete it when everything looks good. Its kinda to bad it's the easiest way to test and debug, i could remove it then gitignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably remove it, since I can't even figure out how to get it to run 🙂 I tried uncommenting everything, and I get compile errors.
Side note about dev workflow – when I made this I found running the tests to be the easiest way to work. You can just run cargo test --features proc_macro and that's about as easy as it gets. Plus, tests have the ongoing benefit of making sure the code stays working.
genawaiter-proc-macro/Cargo.toml
Outdated
"printing", | ||
"extra-traits", | ||
"full", | ||
"visit", | ||
"visit-mut", | ||
"clone-impls" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly a note to myself to remove the unused features.
genawaiter-proc-macro/src/common.rs
Outdated
if let Expr::Macro(m) = expr { | ||
if m.mac.path.segments.iter().any(|seg| seg.ident == "yield_") { | ||
let tkns: TokenStream2 = | ||
syn::parse2(m.mac.tokens.clone()).expect("parse of TokensStream failed"); | ||
let ident = quote! { #tkns }; | ||
let co_call = quote! { co.yield_(#ident).await }; | ||
let cc: Expr = parse2(co_call).expect("parse of Expr failed"); | ||
*expr = cc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now handles any Expr
's no more collecting and comparing!
Is this ready for review or still wip? You were right that visit_mut will let you remove a ton of code, but there's still a ton left that can be deleted 🙂 |
…isit_mut, closure is still a function moved to lib
I'm ready for review now hopefully 🤞. I removed a few unneeded match arms, each mod file is now gone I kinda like the way that worked out. For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those changes, this is really coming together! A couple points:
Naming
Naming is hard, but also permanent, so I want us to try to get it right! A while ago you asked for ideas and I suggested calling the macros "producer". Did you miss this or explicitly decide not to do it?
I suggest also getting rid of the _cls
suffix from the closure macros, since its similarity to "class" still bothers me. And a simpler name might nudge people towards them as the "primary" ones to use since they (could) support type inference and closing over variables. So we'd end up with:
stack_producer
stack_producer_fn
rc_producer
rc_producer_fn
sync_producer
sync_producer_fn
What do you think about those names?
Closure macro syntax
I know the current syntax yielder_cls!(u8 in || async move { yield!_(...) })
is a direct result of my off-the-cuff suggestion from a few weeks ago. But man, is it a mouthful. I actually think it should be possible to get rid of the requirement to specify the type at all. You can already use type inference with the non-macro interface:
Type inference (co: Co<_>
)
#[test]
fn test_type_inference() {
let mut gen = Gen::new(|co: Co<_>| async move {
for n in (1..).step_by(2).take_while(|&n| n < 10) {
co.yield_(n).await;
}
});
gen.resume();
}
It should be possible to emit that same Co<_>
syntax from the proc macro, to avoid the awkward syntax. In addition, the async
and the move
could be implied and added by the macro, since not including them always results in an error anyway. With that, you'd just be left with:
sync_producer!(|| yield!_(...); })
Real-world example
let gen = Gen::new(sync_producer!(|| {
let mut n = 1;
while n < 10 {
yield_!(n);
n += 2;
}
}));
That's a much simpler syntax, and one I'd be more comfortable with committing to long-term stability for. Do you see any downsides?
I know it's another change and I'm sorry for the churn! I'm aware we've been going back and forth on changes for a while. If you want, we could try to break this into two PRs: temporarily stop exporting the closure macros, try to land just the *_fn!
ones sooner (since there are no open design questions for them), and address the closure ones in a later PR. I don't want you to feel like this will never get merged! It's totally up to you though—if you want to keep going in one PR, either way is fine with me 😃 Thanks again for putting the time in!
tests/stack.rs
Outdated
#[cfg(feature = "proc_macro")] | ||
#[test] | ||
fn stack_proc_macro_with_co_arg() { | ||
#[genawaiter::stack::stack_yield_fn(u8)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing you added this test to handle the weird case where someone both uses the proc macro, and adds an explicit co
param. It's a great idea for a test, but I'm not sure I agree with the outcome.
I think it would make more sense if the proc macro flat-out disallowed any parameters, since:
- if the user can name the
co
argument, they can technically cause unsafety (as per docs forstack::Gen::new
). If they can't name it, that safety hole is plugged. - any extra arguments would eventually cause a compile error in
stack::Gen::new
anyway. Better to catch that mistake sooner rather than later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh that's cool using the macro to keep it safe.
error: arguments are not allowed when using proc_macro
--> tests/stack.rs:48:5
|
48 | async fn odds(co: genawaiter::stack::Co<'_, u8>) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
tests/rc.rs
Outdated
use genawaiter::rc_yield_cls; | ||
|
||
let gen = Gen::new(rc_yield_cls!( | ||
u8 in async move || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test wouldn't require nightly if you used faux-closure syntax (change async move || {}
to || async move {}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool everything works on stable now!
tests/rc.rs
Outdated
#[cfg(feature = "proc_macro")] | ||
#[test] | ||
fn rc_proc_macro_fn() { | ||
#[genawaiter::rc::rc_yield_fn(u8)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rc::rc
jumped out at me (the clippy stutter lint). I know we can't avoid the proc_macro_hack problems for the closure macros, but we can avoid the stutter for the others. Can you rename this to rc::yield_fn
for rc, sync, and stack?
You could just rename the re-exports in rc/mod.rs
and friends:
pub use genawaiter_proc_macro::rc_yield_fn as yield_fn;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also changed to producer_fn
so no more stutter!
tests/rc.rs
Outdated
#[genawaiter::rc::rc_yield_fn(u8)] | ||
async fn odds() { | ||
for n in (1_u8..).step_by(2).take_while(|&n| n < 10) { | ||
genawaiter::yield_!(n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a style point. I'd like the tests to match the way users use the library. People shouldn't be writing genawaiter::yield_!()
everywhere, instead they'll import it:
use genawaiter::yield_;
// ...
yield!(...);
Can we mirror that in the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I agree I did change it all then realized the reason I did it was the strict
lint fails because when the proc_macro
expands it warns that yield_!
is not used.
Don't worry I love the back and forth, I always learn a lot and respect your commitment to keeping a solid crate/api, I'm learning good habits. Everything seems to be in order the I hope you had a good holidays! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a good holiday, thanks for asking! Spent some time in the town I grew up. I hope yours was good as well!
I have to admit, I did not anticipate that unused_imports lint issue. It's a sneaky one, but I have an idea to solve it:
-
Redefine the macro_rules! yield macro to look something like this:
macro_rules! yield_ { ($value) => { comple_error!("can only be used via proc macro"); }; (@emit => $co:expr, $value:expr) => { $co.yield_($value).await }; }
-
Modify the visitor, so instead of replacing the macro invocation wholesale, it just changes the arguments:
// User's code, before proc macro: yield_!(2); // Output of the proc macro: yield_!(@emit => co, 2);
This way the macro is still considered used, and the lint will not trigger.
@@ -350,3 +355,18 @@ mod doctests { | |||
*/ | |||
fn co_is_not_static_baseline() {} | |||
} | |||
#[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't compile_fail
ing for the reason I think you think it is. I think this test should actually be testing that you can't name the co
variable. For example, this test passes, but in order to uphold safety, it should fail to compile:
#[test]
fn blah() {
use genawaiter::stack::producer_fn;
#[producer_fn(u8)]
async fn prod() {
drop(co);
}
}
If you can drop co
, you can do other nefarious things, like move it to a 'static
location and write to the pointer once it's dangling. This would violate memory safety.
Rust's solution for "protecting" identifiers is hygeine.
Example
macro_rules! define_co {
() => { let co = "some value"; };
}
#[test]
fn tst() {
define_co!();
drop(co); // <-- Fails to compile. This is what we want.
}
That's using macro_rules, but there should also be a way to set a separate syntax context using proc macros. Can you investigate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm well one of the ideas from stack overflow was to just plop a uuid in front of it, this would work but it seems unfortunate (the answer is for current rust).
Another option would be to enforce this using the proc_macro
fn visit_path_mut(&mut self, path: &mut syn::Path) {
if let Some(n) = path.get_ident() {
if n == "co" { abort!(path.span(), "you are not able alter the Co<...>") }
}
visit_mut::visit_path_mut(self, path)
}
here is the error it produces
error: you are not able alter the Co<...>
--> tests/stack.rs:50:54
|
50 | let x: Co<'static, u8> = std::mem::transmute(co);
| ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great SO answer (and almost suspiciously recent too!)
I don't like UUID since it's non-deterministic. Here's another idea – can you create an identifier with invalid characters, so normal Rust code can't name it? e.g., call it #co
. Failing that, we should probably go for the super ugly name option.
I suspect your last option could be broken with another macro. Untested example:
macro_rules! grab_the_co {
() => { co };
}
#[producer]
async fn blah() {
// Genawaiter proc macro won't see the `co` expression, but post-macro expansion Rust will.
grab_the_co!().yield_(1).await;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the last option has way to many edge cases it was more a first idea that popped into my head kinda thing. #co
won't work it errors thinking its an attribute, __private_co_arg__: Co<...>
sound good?
I just reviewed this, and it looks great! In the interest of having the best possible API, I'd like to make one last tweak (sorry, but APIs are forever, as they say!) I think we should remove the empty argument list from the macro syntax. Since those closures can never have user-specified arguments, the pipes are redundant and just line noise. // change from this:
rc_producer!(|| { ... })
// to this:
rc_producer!({ ... }) Hopefully this just means changing Once that's ready, I'll merge and push a release! |
Yea this actually ended up allowing me to remove even more from the |
Looks good, consider yourself merged! I know this was a lot of work and a lot of changes. I'm very happy with the final result. Thanks for sticking it through to the end, and welcome to collaborators 🙂 Thanks for the offer – I can't think of anything else I have going on, but if I think of something, I'll let you know. |
@DevinR528 Since you asked, let me know if you're interested in working on a PR for #7. I know your offer was technically for "other projects" and you may be exhausted by this crate. But I promise that will be a much simpler PR than this one was! |
No I rather enjoy this one haha, I'd love to keep working on this. What would your git flow be if you were me, or just delete and re |
This should grab github's merge commit. Then:
and start plugging away If that doesn't work, it's tough for me to know what's going on without having access to type commands, but a delete/re-clone is a guaranteed way of getting to a clean slate. |
ref issue #2
This is what I have so far. I will start to switch it over to use
yield_!
it shouldn't be to hard. EachCo<...>
will have twomacro_attribute
's one forfn()
's and one for closures. Any use with closures will have to be done onnightly
as#![feature(proc_macro_hygiene)]
is needed (bummer 👎 ).