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

rustc_mir: follow FalseUnwind's real_target edge in qualify_consts. #62274

Merged
merged 1 commit into from
Jul 13, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jul 1, 2019

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

r? @matthewjasper or @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2019
@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

@eddyb Can you follow up with a PR to the reference as well?

@eddyb
Copy link
Member Author

eddyb commented Jul 5, 2019

The reference? Why? This was an implementation bug, any code that works now used to be allowed, and any assumption that it wasn't supposed to be allowed was incorrect.
Did we document the broken behavior somehow? I kind of doubt it.

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

@eddyb Because the reference says nothing about loops that immediately break being allowed in const contexts, https://doc.rust-lang.org/nightly/reference/const_eval.html; and neither does it say anything about const functions either, https://doc.rust-lang.org/nightly/reference/items/functions.html#const-functions.

Speaking of const functions, can you account for loop { break; } in those as well?

[...], and any assumption that it wasn't supposed to be allowed was incorrect.

(General note: what is "supposed to be" is decided by the language team)

@eddyb
Copy link
Member Author

eddyb commented Jul 5, 2019

@Centril I'm only fixing a bug and bringing the behavior back to its previous state.
If you want to ban syntactic loops in constants, you need to do it on HIR, not MIR.
Feel free to open a separate issue for deciding what to do there.

To be clear: MIR constructs like FalseUnwind only exist for certain subtle borrowck analysis reasons, not to indicate any particular syntactic constructs, and there was no deliberate attempt in #47802 to disallow always-breaking loops in constants, making this a bug that we should fix regardless of syntactic restrictions.

r? @pnkfelix

@eddyb
Copy link
Member Author

eddyb commented Jul 5, 2019

Oh, also, cc @oli-obk this might need qualify_min_consts support as well.

@bors
Copy link
Contributor

bors commented Jul 6, 2019

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

@pnkfelix
Copy link
Member

If you want to ban syntactic loops in constants, you need to do it on HIR, not MIR.
Feel free to open a separate issue for deciding what to do there.

I agree with this.

@pnkfelix
Copy link
Member

r=me once merge conflict is resolved.

@pnkfelix pnkfelix 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 Jul 10, 2019
@pnkfelix
Copy link
Member

@bors rollup

@eddyb
Copy link
Member Author

eddyb commented Jul 11, 2019

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Jul 11, 2019

📌 Commit baa9efb has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 11, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 12, 2019
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
bors added a commit that referenced this pull request Jul 12, 2019
Rollup of 12 pull requests

Successful merges:

 - #61535 (Coherence test when a generic type param has a default value from an associated type)
 - #62274 (rustc_mir: follow FalseUnwind's real_target edge in qualify_consts.)
 - #62431 (Add messages to `Option`'s and `Result`'s `must_use` annotation for `is_*`)
 - #62453 (in which we suggest anonymizing single-use lifetimes in paths )
 - #62568 (Replace unsafe_destructor_blind_to_params with may_dangle)
 - #62578 (Add test for #49919)
 - #62595 (Document that the crate keyword refers to the project root)
 - #62599 (move mem::uninitialized deprecation back by 1 release, to 1.39)
 - #62605 (Emit dropped unemitted errors to aid in ICE debugging)
 - #62607 (Correctly break out of recovery loop)
 - #62608 (`async unsafe fn` tests)
 - #62623 (downgrade indirect_structural_match lint to allow)

Failed merges:

r? @ghost
@bors bors merged commit baa9efb into rust-lang:master Jul 13, 2019
@eddyb eddyb deleted the const-false-unwind branch July 13, 2019 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unconditionally breaking loops not allowed in constants.
6 participants