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

Add iter macro #137725

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 27, 2025

See related discussion in https://rust-lang.zulipchat.com/#narrow/channel/481571-t-lang.2Fgen/topic/iter!.20macro/near/500784563

very little error case testing so far, but the success path works.

There is also no IterFn trait yet, as T-lang didn't consider it something urgently needed I think we can implement it in follow-up PRs.

r? lang for the tests, @compiler-errors for the impl

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 27, 2025
@oli-obk oli-obk force-pushed the i-want-to-move-it-move-it branch from e925043 to b2afc00 Compare February 27, 2025 12:41
@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the i-want-to-move-it-move-it branch from b2afc00 to abd590c Compare February 27, 2025 13:42
@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the i-want-to-move-it-move-it branch from abd590c to 180a69b Compare February 27, 2025 14:11
@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors self-assigned this Feb 27, 2025
@compiler-errors
Copy link
Member

Make sure there is a check-pass test for when the coroutine-closure does capture things but those things don't force it to be lending.

@eholk
Copy link
Contributor

eholk commented Mar 5, 2025

I took a look at the tests and they match the syntax I was expecting.

@traviscross
Copy link
Contributor

cc @eholk @tmandry @traviscross

Comment on lines +7 to +6
fn plain() -> impl Fn() -> impl Iterator<Item = u32> {
iter! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is interesting. I was expecting this case would return impl IntoIterator, not an impl GenFn-equivalent. I don't necessarily mind it though.

I do want to check that we're confident that we can create a blanket impl for impl IntoIterator for GenFn {} (or do something equivalent) so that the following snippet will just work once we add GenFn proper:

let iter = iter! {
    for x in 5..10 {
        yield x * 2;
    }
};

for x in iter { // <- calls `.into_iter` for you
    println!("{x}");
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also would expect this to return impl IntoIterator. Maybe one day impl Iterator, but we can defer that discussion for later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the recent direction has been to prefer functions as the more powerful version of Into*, so this seems in line with that.

That said, I think it's important that the output of the iter! can go directly into a for loop if there is no ||.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess that's the surprising part here, that this example doesn't have ||.

@traviscross traviscross assigned traviscross and unassigned scottmcm Mar 5, 2025
@eholk
Copy link
Contributor

eholk commented Mar 6, 2025

I took a look at the tests and they match the syntax I was expecting.

Okay, I take my comment back. I think the decision was to have two variants of it:

  • iter! { EXPR } which evaluates to a impl Iterator
  • iter! { | ID* | EXPR+ } which evaluates to some find of closure that returns an impl Iterator.

@traviscross
Copy link
Contributor

traviscross commented Mar 6, 2025

The latest resolution was the outcome of the 2025-02-12 meeting, where between @tmandry and I, we agreed to do:

iter!{}.into_iter()
iter!(|x| {})(x)

In the first example, the iter!{} returns a thunk iter closure, so it's exactly equivalent to iter!{|| ()}. Then thunk iter closures implement IntoIterator (we had in mind a blanket impl, but given the next point, implementing IntoIterator directly on such concrete closures I'd expect to be indistinguishable).

The IterFn* traits implied by this design would not be exposed in any manner.

Iter closures return types that implement Iterator.

We agreed we'd later discuss possibly going further on the iter!{} case and implementing Iterator directly as well, but we haven't had that discussion yet.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 6, 2025

Then thunk iter closures implement IntoIterator (we had in mind a blanket impl, but given the next point, implementing IntoIterator directly on such concrete closures I'd expect to be indistinguishable).

That's not how async closures work. We should do both at the same time, but it seems entirely orthogonal

@traviscross
Copy link
Contributor

traviscross commented Mar 6, 2025

That's not how async closures work. We should do both at the same time, but it seems entirely orthogonal.

Here's how we got here, roughly:


Alice: We should ship gen blocks somehow.

Bob: Probably we want to support self borrowing though.

Alice: But that needs a new trait.

Bob: Yes. We do have a design for this that probably works.

Alice: Still, it'd be nice if we could ship something before settling those details.

Bob: Agreed, but we don't want to use the "nice" syntax for that.

Alice: Yes, also we probably want these to be lending.

Bob: Yes; that's even more to work out though, and the trait solver isn't really yet up for that.

Alice: And I'd prefer if we didn't end up with a bunch of different bounds for all these, lending vs non-lending, self-borrowing vs not self-borrowing, etc.

Bob: Yes; that may be hard to avoid though. OK, what if we ship a kind of placeholder for now? We won't expose the traits or the bounds, and we'll use a macro syntax.

Alice: Yes, that could work. Maybe iter! { .. } and it'd work like the iter version of async blocks.

Bob: We probably don't quite want to do that. We know better now that people will immediately want to do || iter! { .. } just like they did for async, and that's a problem that we just dug ourselves out of with async closures.

Alice: Yes, we've already learned that lesson, I suppose there's no point repeating it and having to migrate people off of it. So what does that mean?

Bob: Well, we'll need to put the bars in the macro invocation. Then we can control the expansion.

Alice: So we'd have iter! { .. } that works like async { .. } and iter_fn! {|| .. } that works like async closures?

Bob: Actually, I'm not sure we really need the former. Another thing we've learned is that it might have been better if the "main" trait had been IntoFuture rather than Future -- we might have given the wrong one the shorter name.

Alice: Yes, we recently talked about that during the async closures stabilization. We do prefer seeing the more general trait in bounds.

Bob: Yes, and what we came away with is that, fortunately, this is kind of OK because thunk async closures are a "better" IntoFuture.

Alice: Right, so in that way, we kind of get another shot at this. We can push people toward AsyncFn*() in bounds as the "new" IntoFuture.

Bob: Yes, and by passing around thunk async closures rather than futures, it avoids problems like the one that Yosh wrote about that kicked off that last-minute discussion about the async closures stabilization.

Alice: Ah, I see. If the examples in his post used async || { .. } everywhere rather than async { .. }, then that Send problem goes away. Even if a non-Send thing is held on the stack across an await point, the closure can be passed around freely across threads.

Bob: That's right. This kind of thing was also on our mind when we drafted the gen blocks RFC 3513. All the examples there are written as fn f() -> impl Iterator { gen { .. }.into_iter() } to leave space for the possibility that, contra to how we did async { .. }, we might want gen { .. } blocks to return a type that only implements IntoIterator (or IntoGen or whatnot). Then perhaps we could change async blocks to match in the following edition.

Alice: And since we now know that thunk iter closures are the "better" IntoIterator, we'd actually maybe want gen { .. } blocks to return thunk gen closures.

Bob: That's right. We could even probably blanket implement IntoIterator for thunk iter closures, and IntoFuture for thunk async closures.

Alice: Yes, that makes sense. So what do we do about iter! again?

Bob: We just ship the iter closure form of it, iter!(|..| ..). That may be all we need. And it's aligned with the desirable direction on all of this.

Alice: That makes sense. Stepping back to how we're trying to avoid a migration, people still might write the pseudo iter closure bounds that won't really work, though, since we wouldn't be shipping the bounds, right?

Bob: That's right, exactly as we saw with async. Probably I'd go ahead and just ship the IterFn*() bounds to avoid that, but that would mean we'd end up with more total stable bounds, which we're agreeing to see if we can avoid, so we'll just accept that tradeoff. We're solving the constructor half of the problem here, and it'll still be a win for people using it concretely.

Alice: Makes sense. OK, turning to syntax, maybe it'd be nice if there was a shorthand for when there are no arguments. We could use iter! { .. }. Since iter blocks always return (), this isn't ambiguous, since you couldn't have an iter block returning a closure.

Bob: Requiring the bars there doesn't bother me that much, and it seems maybe a bit ad-hoc to lean on that coincidence about the return type, but we could try that. Maybe it's enough of a win to be worth that. As long as it returns a thunk iter closure, it's still aligned with this model. Sounds like we're pretty close here.

Alice: One other thing. It might feel better to people to write iter! { .. }.into_iter() rather than iter! { .. }(), even though they're semantically-equivalent. So we should do that part too along with this.

Bob: Reasonable enough. We're using a placeholder syntax here anyway. It's probably fine to do this even without having done it for async yet. Let's get all that ready to go.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 6, 2025

Makes sense. Let's treat this PR as an MVP then and figure it out together with async closures

@oli-obk oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2025
@tmandry
Copy link
Member

tmandry commented Mar 6, 2025

In the first example, the iter!{} returns a thunk iter closure, so it's exactly equivalent to iter!{|| ()}. Then thunk iter closures implement IntoIterator

I understand this as a future possibility, not the agreement we reached in the meeting. My understanding is that iter! {} would return something that implements IntoIterator directly, but not something that can be called from day one. This is compatible with making it callable later.

It doesn't make sense to me to make something callable that doesn't use closure syntax – unless we essentially deprecate IntoIterator in favor of closures, which should be a separate discussion.

@traviscross
Copy link
Contributor

traviscross commented Mar 6, 2025

Interesting. Things were a bit scattered at the end of that meeting in trying to work out where we had agreement and didn't and whatnot. For my part, as described in the Alice and Bob story, and in how I similarly framed it in the meeting, due to how we're walking down the "thunk closures are the better Into*" path, I do consider this part and parcel of it. I.e., if this were not true, then it works against pushing people toward using *Fn*() bounds.

Probably we're just confusing everyone at this point, though. Let's talk about it tomorrow.

I think what we can say right now is we have agreement on how iter!(|..| ..) should work.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 6, 2025
Move `yield` expressions behind their own feature gate

In order to make progress with the `iter!` macro (e.g. in rust-lang#137725), we need `yield` expressions to be available without the `coroutines` feature. This PR moves `yield` to be guarded by the `yield_expr` feature so that we can stabilize that independently (or at least, concurrently with the `iter_macro` feature). Note that once `yield` is stable, it will still be an error to use `yield` expressions outside something like a generator or coroutine, and these features remain unstable.

r? `@oli-obk`
@bors
Copy link
Contributor

bors commented Mar 7, 2025

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

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2025
Rollup merge of rust-lang#138081 - eholk:yield-feature, r=oli-obk

Move `yield` expressions behind their own feature gate

In order to make progress with the `iter!` macro (e.g. in rust-lang#137725), we need `yield` expressions to be available without the `coroutines` feature. This PR moves `yield` to be guarded by the `yield_expr` feature so that we can stabilize that independently (or at least, concurrently with the `iter_macro` feature). Note that once `yield` is stable, it will still be an error to use `yield` expressions outside something like a generator or coroutine, and these features remain unstable.

r? `@oli-obk`
@eholk eholk force-pushed the i-want-to-move-it-move-it branch from 180a69b to 0e8d7ea Compare March 7, 2025 21:21
@rust-log-analyzer

This comment has been minimized.

@traviscross
Copy link
Contributor

We -- @tmandry, @eholk, and I -- did talk about this today, and what we'd like to do is just start with the iter!(|..| ..) part of this for now.

@eholk eholk force-pushed the i-want-to-move-it-move-it branch from 0e8d7ea to 51c75e7 Compare March 8, 2025 03:28
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Comment on lines +55 to +77
let lo = parser.token.span.shrink_to_lo();
let block = parser.parse_block_tail(
lo,
ast::BlockCheckMode::Default,
rustc_parse::parser::AttemptLocalParseRecovery::No,
)?;
let fn_decl = cx.fn_decl(Default::default(), ast::FnRetTy::Default(span));
let closure = ast::Closure {
binder: ClosureBinder::NotPresent,
capture_clause: CaptureBy::Ref,
constness: Const::No,
coroutine_kind,
movability: ast::Movability::Movable,
fn_decl,
body: cx.expr_block(block),
fn_decl_span: span,
fn_arg_span: span,
};
if parser.token != token::Eof {
parser.unexpected()?;
}
let span = lo.to(parser.token.span);
Ok(cx.expr(span, ExprKind::Closure(Box::new(closure))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part can be ripped out now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants