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

Tracking issue for RFC 1624: break with values for loop #37339

Closed
4 tasks done
aturon opened this issue Oct 22, 2016 · 44 comments
Closed
4 tasks done

Tracking issue for RFC 1624: break with values for loop #37339

aturon opened this issue Oct 22, 2016 · 44 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Oct 22, 2016

RFC.

Current status:

@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. A-lang labels Oct 22, 2016
@aturon
Copy link
Member Author

aturon commented Oct 22, 2016

Currently awaiting implementation.

cc @dhardy

@eddyb
Copy link
Member

eddyb commented Oct 22, 2016

@aturon How would we go about advertising mentorship? If someone wants to implement this and they're not already familiar with the compiler internals, they can come to #rustc on IRC to discuss it.
This is the case for many such features with a relatively small footprint, we're glad to help.

(Although I may be spreading myself thin a bit too much instead of finishing my own refactors.)

@aturon
Copy link
Member Author

aturon commented Oct 22, 2016

@eddyb An important question we don't have a systematic answer for right now (cc @brson). We should probably move this to an internals thread or something, though.

@dhardy
Copy link
Contributor

dhardy commented Oct 24, 2016

I hear you @aturon but don't have the time just now (and maybe for a few weeks).

goffrie added a commit to goffrie/rust that referenced this issue Nov 22, 2016
This implements RFC 1624, tracking issue rust-lang#37339.

- `FnCtxt` (in typeck) gets a stack of `LoopCtxt`s, which store the
  currently deduced type of that loop, the desired type, and a list of
  break expressions currently seen. `loop` loops get a fresh type
  variable as their initial type (this logic is stolen from that for
  arrays). `while` loops get `()`.
- `break {expr}` looks up the broken loop, and unifies the type of
  `expr` with the type of the loop.
- `break` with no expr unifies the loop's type with `()`.
- When building MIR, `loop` loops no longer construct a `()` value at
  termination of the loop; rather, the `break` expression assigns the
  result of the loop. `while` loops are unchanged.
- `break` respects contexts in which expressions may not end with braced
  blocks. That is, `while break { break-value } { while-body }` is
  illegal; this preserves backwards compatibility.
- The RFC did not make it clear, but I chose to make `break ()` inside
  of a `while` loop illegal, just in case we wanted to do anything with
  that design space in the future.

This is my first time dealing with this part of rustc so I'm sure
there's plenty of problems to pick on here ^_^
bors added a commit that referenced this issue Nov 22, 2016
Implement the `loop_break_value` feature.

This implements RFC 1624, tracking issue #37339.
- `FnCtxt` (in typeck) gets a stack of `LoopCtxt`s, which store the
  currently deduced type of that loop, the desired type, and a list of
  break expressions currently seen. `loop` loops get a fresh type
  variable as their initial type (this logic is stolen from that for
  arrays). `while` loops get `()`.
- `break {expr}` looks up the broken loop, and unifies the type of
  `expr` with the type of the loop.
- `break` with no expr unifies the loop's type with `()`.
- When building MIR, loops no longer construct a `()` value at
  termination of the loop; rather, the `break` expression assigns the
  result of the loop.
- ~~I have also changed the loop scoping in MIR-building so that the test
  of a while loop is not considered to be part of that loop. This makes
  the rules consistent with #37360. The new loop scopes in typeck also
  follow this rule. That means that `loop { while (break) {} }` now
  terminates instead of looping forever. This is technically a breaking
  change.~~
- ~~On that note, expressions like `while break {}` and `if break {}` no
  longer parse because `{}` is interpreted as an expression argument to
  `break`. But no code except compiler test cases should do that anyway
  because it makes no sense.~~
- The RFC did not make it clear, but I chose to make `break ()` inside
  of a `while` loop illegal, just in case we wanted to do anything with
  that design space in the future.

This is my first time dealing with this part of rustc so I'm sure
there's plenty of problems to pick on here ^_^
@glaebhoerl
Copy link
Contributor

Out of curiosity is there anything holding this back from being stabilized?

@aturon aturon added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Mar 10, 2017
@aturon
Copy link
Member Author

aturon commented Mar 10, 2017

@glaebhoerl Not that I'm aware of. More obscure features like this are hard to judge, since we naturally tend to get very little feedback on them.

In any case, it's certainly been around long enough that going to FCP to try to advertise for more feedback seems good:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 10, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@withoutboats
Copy link
Contributor

@rfcbot reviewed

@nrc
Copy link
Member

nrc commented Mar 12, 2017

I guess I don't have any technical reason against stabilising this (I was in favour of the RFC and nothing has changed my mind). But I kind of think that if a feature isn't being used at all then we should consider removing it rather than stabilising it. Is it worth posting in users.r-l.o to see if people are using it and whether there are any problems?

@SimonSapin
Copy link
Contributor

It’s not surprising that a feature isn’t used when it’s not available yet. Much of the ecosystem has moved to the stable channel.

@andrewtj
Copy link

@nrc I played with it, saw that it worked, and then promptly stopped using it because while it's nice (really!) it's not worth jumping off stable for some indeterminate period. So if my experience is a barometer you may not get that much feedback but maybe that's to be expected for this feature.

@Boscop
Copy link

Boscop commented Mar 13, 2017

FWIW, I'm using it, e.g. for things like:

			let (filename, mut file) = loop {
				let uuid = Uuid::new_v4().simple().to_string();
				let filename = format!("{}.{}", uuid, ImageFormat::from_image_format(fmt).as_str());
				let fpath = ::helper::filename_to_local(&filename);
				if let Ok(f) = fs::OpenOptions::new().write(true).create_new(true).open(&fpath) {
					break (filename, f);
				}
			};
			img.save(&mut file, fmt)?;

@withoutboats
Copy link
Contributor

I also wouldn't depend on a nightly feature for this, but would use it if it were available. As it stands I'd tend to just use a temporary variable, but I'd rather not if this were stable.

@glaebhoerl
Copy link
Contributor

(Likewise, I originally popped in here to ask about stabilization because I was going to suggest it to someone on twitter who was lamenting the necessity of the temporary variables, before I realized that it's only available on nightly.)

@crumblingstatue
Copy link
Contributor

crumblingstatue commented Mar 13, 2017

It should take more than just "apparent lack of usage" to remove a feature for which the RFC was accepted, with the motivation clearly understood.

People won't go out of their way to change their existing code just to use this feature, but it offers a better alternative for some cases, doesn't introduce any significant complexity, and makes the syntax more consistent with the expression oriented nature of Rust.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Mar 13, 2017

While I'm personally wildly in favor of this, it is important that features be evaluated before stabilization. There was a thread before on this connundrum, but I don't recall it concluding with a solution either :/.

@dhardy
Copy link
Contributor

dhardy commented Mar 13, 2017

To me the "experimental features are unavailable in stable & beta" decision seems wrong. There are a couple of advantages to using the stable branch and it would be nice to be able to use stable while also trying out probably-to-be-stabilised features with the only caveat being that these features might be removed or significantly altered in a future compiler version.

Being forced to use nightly is less attractive: more likely to find other bugs and possibly having to recommend usage of arbitrary nightly releases with your project instead of just the latest stable release.

None of which relates directly to this RFC.

@rfcbot
Copy link

rfcbot commented Mar 16, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Mar 16, 2017
@rfcbot
Copy link

rfcbot commented Mar 26, 2017

The final comment period is now complete.

@nikomatsakis nikomatsakis added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Apr 11, 2017
@nikomatsakis
Copy link
Contributor

So, we've decided to stabilize this feature. That means we would like some PRs. I'm going to mark this bug as E-easy and E-mentor. The PRs we need are a documentation PR (first) and the stabilization PR (second). I've added some links to instructions into the head comment.

@pietroalbini
Copy link
Member

Pull requests for the documentation sent to the various repositories. Should I wait for the stabilization PR?

@dhardy
Copy link
Contributor

dhardy commented May 17, 2017

I noticed a small inconsistency between documentation and implementation: the RFC claims

break and break 'label become equivalent to break () and break 'label () respectively

but the impl doesn't support break () in while or for loops.

Perhaps this is for the best anyway until "break with value" is supported by for and while.

@dhardy
Copy link
Contributor

dhardy commented May 17, 2017

New reference PR: rust-lang/reference#56

frewsxcv added a commit to frewsxcv/rust that referenced this issue May 23, 2017
…alue, r=nikomatsakis

Stabilize the loop_break_value feature

Tracking issue: rust-lang#37339.

Documentation PRs already sent to the various repositories.
@ciphergoth
Copy link
Contributor

It might make sense to allow break to take a value of type () in while and for loops too, for consistency.

@ciphergoth
Copy link
Contributor

ciphergoth commented Jun 6, 2017

A future RFC could extend this to labelled blocks without loops.

    let result : u64 = 'block: {
        if foo() { break 'block 1 }
        if bar() { break 'block 2 }
        3
    };

This would also give you a way to handle for and while

    let result : u64 = 'block: {
        for e in container.iter() {
            let v = foo(e);
            if v > 0 { break 'block v }
        }
        0
    };

The semantics would be exactly the same as

    let result : u64 = 'block: loop { break {
        for e in container.iter() {
            let v = foo(e);
            if v > 0 { break 'block v }
        }
        0
    }};

EDIT: I made this into a Pre-RFC.

@nikomatsakis
Copy link
Contributor

We specifically decided to limit this to loop loops. In fact, there is a (closed) rfc issue on this topic (rust-lang/rfcs#1767).

@aturon
Copy link
Member Author

aturon commented Jun 7, 2017

@nikomatsakis The proposal here is different: it's talking about jumping out to a labeled block, rather than simply breaking out of a loop. It just happens to combine reasonably well with loops.

@Ericson2314
Copy link
Contributor

Hmm, one extra level of indentation, but way less controversy! That's a pretty neat trick @ciphergoth.

@est31
Copy link
Member

est31 commented Jun 26, 2017

Can this issue be closed? The stabilisation PR #42016 has been merged since.

@Mark-Simulacrum
Copy link
Member

I think so, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests