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

Type-check `break value;` even outside of `loop {}`. #43745

Merged
merged 1 commit into from Aug 11, 2017

Conversation

Projects
None yet
9 participants
@kennytm
Member

kennytm commented Aug 8, 2017

Fix #43162, fix #43727.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Aug 8, 2017

Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Aug 8, 2017

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Aug 8, 2017

Member

(The break rust; thing is following #43162 (comment). It can be reverted if considered inappropriate.)

Member

kennytm commented Aug 8, 2017

(The break rust; thing is following #43162 (comment). It can be reverted if considered inappropriate.)

@zackmdavis

This comment has been minimized.

Show comment
Hide comment
@zackmdavis

zackmdavis Aug 8, 2017

Contributor

It can be reverted if considered inappropriate.

I don't think the type-checker of a production compiler is a good place for easter eggs.

(For comparison, Python has import this and from __future__ import braces, but it feels significant that those are tucked away in the standard library, rather than affecting the semantics of the language itself.)

Contributor

zackmdavis commented Aug 8, 2017

It can be reverted if considered inappropriate.

I don't think the type-checker of a production compiler is a good place for easter eggs.

(For comparison, Python has import this and from __future__ import braces, but it feels significant that those are tucked away in the standard library, rather than affecting the semantics of the language itself.)

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Aug 8, 2017

Member

@zachreizner Not saying the feature should stay (I may delete those in 1 or 2 days r+'ed 🎊)... but the change does not affect the semantics of the language itself, since entering that branch already indicates the code is wrong and is going to fail soon anyway (which is also the reason why I've added an extra test case to run-pass to ensure normal code is not affected).

Member

kennytm commented Aug 8, 2017

@zachreizner Not saying the feature should stay (I may delete those in 1 or 2 days r+'ed 🎊)... but the change does not affect the semantics of the language itself, since entering that branch already indicates the code is wrong and is going to fail soon anyway (which is also the reason why I've added an extra test case to run-pass to ensure normal code is not affected).

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Aug 8, 2017

Contributor

Generally r=me, but we need signoff from @aturon for all new features.

Contributor

arielb1 commented Aug 8, 2017

Generally r=me, but we need signoff from @aturon for all new features.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Aug 8, 2017

Contributor

I think we could also go with signoff from our release manager, @brson

Contributor

arielb1 commented Aug 8, 2017

I think we could also go with signoff from our release manager, @brson

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Aug 8, 2017

Member

@bors: r+

for as long as we can get away with it

Member

aturon commented Aug 8, 2017

@bors: r+

for as long as we can get away with it

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 8, 2017

Contributor

📌 Commit 3cb23a7 has been approved by aturon

Contributor

bors commented Aug 8, 2017

📌 Commit 3cb23a7 has been approved by aturon

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Aug 9, 2017

Member

This is awesome. I'll wait for the first nightly that includes this and pin all my projects to it!

Member

killercup commented Aug 9, 2017

This is awesome. I'll wait for the first nightly that includes this and pin all my projects to it!

@jD91mZM2

This comment has been minimized.

Show comment
Hide comment
@jD91mZM2

jD91mZM2 Aug 10, 2017

Contributor

Does this only work outside of loops? Or is it a breaking change?

Contributor

jD91mZM2 commented Aug 10, 2017

Does this only work outside of loops? Or is it a breaking change?

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 11, 2017

Contributor

⌛️ Testing commit 3cb23a7 with merge 9868352...

Contributor

bors commented Aug 11, 2017

⌛️ Testing commit 3cb23a7 with merge 9868352...

bors added a commit that referenced this pull request Aug 11, 2017

Auto merge of #43745 - kennytm:fix-43162, r=aturon
Type-check `break value;` even outside of `loop {}`.

Fix #43162, fix #43727.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 11, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 9868352 to master...

Contributor

bors commented Aug 11, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 9868352 to master...

@bors bors merged commit 3cb23a7 into rust-lang:master Aug 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@est31 est31 referenced this pull request Sep 8, 2017

Closed

Break with Value causes ICE #44416

@kennytm kennytm deleted the kennytm:fix-43162 branch Nov 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment