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

Clean up "const_hack" PRs now that const if/match exists #67627

Closed
oli-obk opened this issue Dec 26, 2019 · 7 comments · Fixed by #67657
Closed

Clean up "const_hack" PRs now that const if/match exists #67627

oli-obk opened this issue Dec 26, 2019 · 7 comments · Fixed by #67657
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Dec 26, 2019

Before we had branches in const fn we added a few hacks to some functions by rewriting them in unreadable ways or removing assertions. We should undo all these hacks. The list of PRs with const hacks is https://github.com/rust-lang/rust/issues?q=label%3Aconst-hack+is%3Aclosed

Most of the time you'll want to revert the diff to the body of any const fn but not undo any other changes that these PRs made. Then you'll need to add allow_internal_unstable attributes for the const_if feature gate (or whatever other feature gates the compiler tells you to add) in case the const fn is already stable.

This issue has been assigned to @jumbatm via this comment.

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Dec 26, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 26, 2019

cc @ecstatic-morse @RalfJung

@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 26, 2019
@jumbatm
Copy link
Contributor

jumbatm commented Dec 26, 2019

Hi, may I give this a go?

@rustbot claim

@rustbot rustbot assigned rustbot and unassigned rustbot Dec 26, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 26, 2019

It's all yours! Let me know if you run into any troubles or need more info.

@jumbatm
Copy link
Contributor

jumbatm commented Dec 27, 2019

Hello! I'm having a bit of trouble cleaning up the functions in num. Specifically, I'm getting num::<impl i8>::wrapping_abs is not yet stable as a const fn`, which I'm not sure how to solve, on the changes in this commit here jumbatm@a58d811.

I started by reverting the body of wrapping_abs to the pre-const-hack contents, and then adding the rustc_const_unstable annotation to the function. However, the compiler complained with the following message:

  src/libcore/num/mod.rs|1791 col 18 error 723| can only call other `const fn` within a `const fn`, but `const num::<impl i8>::wrapping_abs` is not stable as `const fn` 

To me, it seemed because overflowing_abs calls wrapping_abs, and I had just made wrapping_abs const unstable, I needed to mark overflowing_abs as const unstable too. So, I did so, but this only changed the error message:

src/libcore/num/mod.rs|1791 col 18 error| `num::<impl i8>::wrapping_abs` is not yet stable as a const fn

At this point, I reverted marking overflowing_abs as unstable, and checked the implementation of the check which was raising the error, and found it in qualify_min_const_fn:333. I saw this calls TyCtxt::is_const_fn(..), so I checked its implementation in src/librustc/ty/constness.rs:42, and saw rustc_const_unstable functions don't need to conform to min_const_fn.

Looking at the codebase, I saw there exists examples of other const unstable function which are able to call other rustc_const_unstable functions without issue -- for example, to_be_bytes (here), and it appears that propagating the rustc_const_unstable annotation to its callers is, indeed, the right thing to do.

So, I tried marking overflowing_abs as const unstable again, and this time looked at the check which was causing the "is not yet stable as a const fn" message, which I found in src/librustc_mir/transform/check_consts/op.rs:115. Due to my annotation, the error message suggests that I add #![feature(const_if_match)] to the crate, but you can see I already added it to libcore/lib.rs.
For some time, I thought FnCallUnstable from the same file was missing an override of fn feature_gate(...), that would look for feature, but realised because feature_gate is an associated function, I wouldn't be able to pass the instance-specific feature in anyway, and because this error isn't appearing for any other const unsafe code, the feature flag must have been handled somewhere earlier. I started looking for this in libsyntax/feature_gate, before realising this is probably a case where I should ask for help.

What's the (likely obvious 😝 ) detail I'm missing here that's causing this issue?

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 27, 2019

What's the (likely obvious stuck_out_tongue_closed_eyes ) detail I'm missing here that's causing this issue?

rustc_const_stable may never be changed to rustc_const_unstable, that would be a breaking change. If you want to use unstable features in a stable const fn, you need to add a new attribute: allow_internal_unstable(feature_gate_name) (this attribute also takes a comma separated list of feature gates if you need multiple ones

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 27, 2019

I adjusted the main post, sorry about that misinformation

@jumbatm
Copy link
Contributor

jumbatm commented Dec 27, 2019

Ahh, you're all good :) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants