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

yield without value from a gen block #123614

Closed
Jules-Bertholet opened this issue Apr 8, 2024 · 17 comments
Closed

yield without value from a gen block #123614

Jules-Bertholet opened this issue Apr 8, 2024 · 17 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. F-gen_blocks `gen {}` expressions that produce `Iterator`s

Comments

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Apr 8, 2024

I tried this code:

// edition 2024
#![feature(gen_blocks)]
fn foo() -> impl Iterator {
    gen {
        yield;
    }
}

I expected to see this happen: The gen blocks RFC does not specify the behavior of bare yield (without an argument). There are three possible choices, each with good reasons for and against:

  • Reject it entirely
  • Return None from Iterator::next()
  • Equivalent to yield ()

Instead, this happened: The current behavior is that of yield ().

Meta

rustc --version:

1.79.0-nightly (2024-04-06 aa1c45908df252a5b0c1)

cc #117078

@rustbot label F-gen_blocks

@Jules-Bertholet Jules-Bertholet added the C-bug Category: This is a bug. label Apr 8, 2024
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. F-gen_blocks `gen {}` expressions that produce `Iterator`s labels Apr 8, 2024
@compiler-errors compiler-errors added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. labels Apr 8, 2024
@compiler-errors
Copy link
Member

I'm not certain why it would make sense to do anything other than yield (). It seems like a total parallel to return () and I don't think it's particularly beneficial to do anything else.

Given that the iterators returned by gen blocks are fused (#122829), it wouldn't make much sense to yield None either, since that makes yield equivalent to return (quit the block and don't come back).

We could perhaps "reserve" the behavior by making it error, but seems unnecessarily cautious imo.

@Jules-Bertholet
Copy link
Contributor Author

Jules-Bertholet commented Apr 8, 2024

Arguments for and against each option:

Reject it entirely

Pros:

  • Forward compatible with either of the other two options
  • Lowest chance of user confusion

Cons:

  • Least useful/permissive choice

Return None from next()

Pros:

  • Allows directly implementing deliberately unfused iterators with gen, which is otherwise impossible
  • Conceptually intuitive: yield /* nothing */ yields nothing, i.e. None

Cons:

  • Inconsistent with break and return
  • Unfused iterators are rare
    • Though not unknown, I implemented one just today and would have liked to be able to use gen for it
  • If used by mistake, the type checker won't catch it

Equivalent to yield ()

Pros:

  • Consistent with break and return

Cons:

  • Iterators returning () are rare
    • Though also far from non-existent

See also discussion on the RFC thread. Personally I lean towards the "return None" option.

@Jules-Bertholet
Copy link
Contributor Author

Given that the iterators returned by gen blocks are fused

The compiler could detect the presence of a bare yield anywhere in the block, and omit the FusedIterator impl only in that case.

@compiler-errors
Copy link
Member

That's a lot of subtlety (and complexity) for a really niche use case, imo, and I think that the user should just use impl Iterator<Item = Option<T>> in that case and explicitly yield None.

@Jules-Bertholet
Copy link
Contributor Author

Jules-Bertholet commented Apr 8, 2024

At least for my one use-case today that motivated me to open this issue, that would have been an ergonomic hit.


The use case: I am consuming an API that give me a stream of data. Sometimes there is new data available, sometimes there isn't yet (but might be in the future). My iterator returns None from next() in the "no new data" case, enabling me to do stuff like this:

loop {
    for new_data in &mut iter {
        process_data(new_data);
    }
    
    do_something_else_for_a_while();
}

The above example could be easily converted to use impl Iterator<Item = Option<T>> and while let Some(Some(new_data)) = iter.next() instead. However, an unfused iterator also lets me do the following, which is not so simply adapted:

let mut all_the_data = vec![];
loop {
    all_the_data.extend(&mut iter);
    do_something_else_for_a_while();
}

@oli-obk
Copy link
Contributor

oli-obk commented Apr 8, 2024

I think code that tries to be clever should be explicit. And using an unfused iterator to only collect up to the first None seems very clever to me.

The behavior was not specified in the RFC, because it seemed obvious to me that we want to be consistent with break and return. I'll amend the RFC.

I'm closing this issue as I don't see T-lang accepting

  • diverging from break and return
  • implicit behaviour that is possibly a footgun
  • for a niche use case that is already supported and could be simplified with a fuse like adaptor.

@oli-obk oli-obk closed this as completed Apr 8, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 8, 2024
@Jules-Bertholet
Copy link
Contributor Author

I think code that tries to be clever should be explicit. And using an unfused iterator to only collect up to the first None seems very clever to me.

Out of curiosity, what would be your preferred "non-clever" version? An iterator over Options combined with take_while()?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2024

A function that explicitly turns an unfused iterator into an iterator of iterators so you can just write two nested for loops.

@Jules-Bertholet
Copy link
Contributor Author

A function that explicitly turns an unfused iterator into an iterator of iterators so you can just write two nested for loops.

That has a lot of hidden complexity. Consider the following:

let mut iterator_of_iterators = unfused_iterator.oli_adapter();

// We never actually iterate over this.
// Does the `Drop` impl advance the inner
// unfused iterator until the next `None`?
// Or does it do nothing,
// and leave the elements we failed to iterate over
// to the `for` loop below?
let _ = iterator_of_iterators.next();

for item in iterator_of_iterators.next().unwrap() {
   // ...
}

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2024

Then maybe just handrolling the while let is actually the best way that does not hide any of the unclear things.

@Jules-Bertholet
Copy link
Contributor Author

You mean, handwrite duplicate implementations of extend et al (forgoing the performance optimizations of the standard library impls)? I don't see how that's supposed to be an improvement…

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2024

I don't know what you mean, and issue format discussions are not a great place to discuss hypotheticals. Also, Aadding special cases to gen blocks won't help you, it just shifts the work to the language/libcore, where we then are unable to make breaking changes. If you have super special requirements, please write a dedicated crate for it, so others can reuse it and upstream optimizations to it.

@Jules-Bertholet
Copy link
Contributor Author

I am asking what you mean when you say "handrolling the while let". I'm not making any proposals—the proposal I did make was rejected, I want to understand your point of view on why you rejected it before I go around making new proposals.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2024

I presumed #123614 (comment) works for you and you just want a more concise version.

If you want to make a proposal, I would recommend writing a crate that does the job for you and then explaining why there are limitations that are only solved by language or standard library support

@Jules-Bertholet
Copy link
Contributor Author

#123614 (comment) is how I use the un-fused iterator. I don't need or want a more concise version of that sort of code, existing language and stdlib affordances meet my needs just fine. What I want is to be able to use gen blocks to produce the non-fused iterators that I later consume in the manner described there.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2024

I don't think we're gonna go out of our way to support unfused iterators. There's no explicit support for them in the standard library functions for them beyond the fact of being sound even in their presence. An iterator is done when it returns None the first time.

We don't have async blocks that you can poll again after returning a value, even though the Future trait allows for it.

@Jules-Bertholet
Copy link
Contributor Author

There's no explicit support for them in the standard library

std::sync::mspc::TryIter is explicitly and intentionally unfused.

(I agree that GitHub is not the best platform for this discussion, which is why I opened a thread on URLO; feel free to lock this issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. F-gen_blocks `gen {}` expressions that produce `Iterator`s
Projects
None yet
Development

No branches or pull requests

5 participants