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.

Sign up for GitHub

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

Jump to bottom

Unconditionally breaking loops not allowed in constants. #62272

Closed
eddyb opened this issue Jul 1, 2019 · 2 comments · Fixed by #62274
Closed

Unconditionally breaking loops not allowed in constants. #62272

eddyb opened this issue Jul 1, 2019 · 2 comments · Fixed by #62274
Assignees
@eddyb
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html regression-from-stable-to-stable Performance or correctness regression from one stable version to another.

Comments

@eddyb
Copy link
Member

eddyb commented Jul 1, 2019

Technically a regression from 1.24 to 1.25, but I don't think anyone used this.
Example code which used to be allowed:

pub const FOO: () = loop { break; };

I'm pretty sure this changed with #47802, which added the FalseUnwind terminator to MIR, but didn't handle it in const-checking (which otherwise ignores cleanup blocks).

cc @matthewjasper @nikomatsakis
@eddyb eddyb added A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html regression-from-stable-to-stable Performance or correctness regression from one stable version to another. A-const-eval Area: constant evaluation (mir interpretation) labels Jul 1, 2019
This was referenced Jul 1, 2019
Merged
Merged
@Centril
Copy link
Contributor

Centril commented Jul 1, 2019

I think this is a technicality and that we can continue to let this be broken since no one said a peep about it since 1.25. The upside is that the syntactic surface of const eval is clearer.

@eddyb eddyb mentioned this issue Jul 1, 2019
Merged
@eddyb eddyb self-assigned this Jul 1, 2019
@eddyb
Copy link
Member Author

eddyb commented Jul 5, 2019

On the PR I explained why I think we should fix this even if we want to ban loops: #62274 (comment)

@Centril Centril mentioned this issue Jul 5, 2019
Open
Centril added a commit to Centril/rust that referenced this issue Jul 12, 2019
@Centril
Rollup merge of rust-lang#62274 - eddyb:const-false-unwind, r=pnkfelix
9ffeb26 
rustc_mir: follow FalseUnwind's real_target edge in qualify_consts.

As far as I can tell, this was accidentally omitted from rust-lang#47802.
Fixes rust-lang#62272.

r? @matthewjasper or @nikomatsakis
@ecstatic-morse ecstatic-morse mentioned this issue Nov 6, 2019
Merged
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this issue Nov 9, 2019
@ecstatic-morse
Bless back-compat breakages
bc5ba5d 
This PR BREAKS CODE THAT WAS ACCEPTED ON STABLE. It's arguably a bug
that this was accepted in the first place, but here we are. See rust-lang#62272
for more info.
bors added a commit that referenced this issue Nov 11, 2019
@bors
Auto merge of #66170 - ecstatic-morse:hir-const-check, r=Centril,oli-obk
47904d2 
Add a HIR pass to check consts for `if`, `loop`, etc.

Resolves #66125.

This PR adds a HIR pass to check for high-level control flow constructs that are forbidden in a const-context. The MIR const-checker is unable to provide good spans for these since they are lowered to control flow primitives (e.g., `Goto` and `SwitchInt`), and these often don't map back to the underlying statement as a whole. This PR is intended only to improve diagnostics once `if` and `match` become commonplace in constants (behind a feature flag). The MIR const-checker will continue to operate unchanged, and will catch anything this check might miss.

In this implementation, the HIR const-checking pass is run much earlier than the MIR one, so it will supersede any errors from the latter. I will need some mentoring if we wish to change this, since I'm not familiar with the diagnostics system. Moving this pass into the same phase as the MIR const-checker could also help keep backwards compatibility for items like `const _: () = loop { break; };`, which are currently (erroneously?) accepted by the MIR const-checker (see #62272).

r? @Centril
cc @eddyb (since they filed #62272)
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this issue Nov 13, 2019
@ecstatic-morse
Bless back-compat breakages
281e898 
This PR BREAKS CODE THAT WAS ACCEPTED ON STABLE. It's arguably a bug
that this was accepted in the first place, but here we are. See rust-lang#62272
for more info.
bors added a commit that referenced this issue Nov 13, 2019
@bors
Auto merge of #66170 - ecstatic-morse:hir-const-check, r=Centril,oli-obk
ded5ee0 
Add a HIR pass to check consts for `if`, `loop`, etc.

Resolves #66125.

This PR adds a HIR pass to check for high-level control flow constructs that are forbidden in a const-context. The MIR const-checker is unable to provide good spans for these since they are lowered to control flow primitives (e.g., `Goto` and `SwitchInt`), and these often don't map back to the underlying statement as a whole. This PR is intended only to improve diagnostics once `if` and `match` become commonplace in constants (behind a feature flag). The MIR const-checker will continue to operate unchanged, and will catch anything this check might miss.

In this implementation, the HIR const-checking pass is run much earlier than the MIR one, so it will supersede any errors from the latter. I will need some mentoring if we wish to change this, since I'm not familiar with the diagnostics system. Moving this pass into the same phase as the MIR const-checker could also help keep backwards compatibility for items like `const _: () = loop { break; };`, which are currently (erroneously?) accepted by the MIR const-checker (see #62272).

r? @Centril
cc @eddyb (since they filed #62272)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eddyb eddyb

Labels
A-const-eval Area: constant evaluation (mir interpretation) A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html regression-from-stable-to-stable Performance or correctness regression from one stable version to another.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

rustc_mir: follow FalseUnwind's real_target edge in qualify_consts. eddyb/rust
2 participants
@eddyb @Centril