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

Get rid of "const_fn" feature gate #84510

Closed
RalfJung opened this issue Apr 24, 2021 · 13 comments · Fixed by #85109
Closed

Get rid of "const_fn" feature gate #84510

RalfJung opened this issue Apr 24, 2021 · 13 comments · Fixed by #85109
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@RalfJung
Copy link
Member

With #84310, the const-checking analysis in compiler/rustc_mir/src/transform/check_consts no longer cares about the const_fn feature gate. So it's time we remove it entirely. That, however, turns out to be non-trivial -- I tried.

  • Some external creates need to be patched first to not use const_fn any more:
error[E0557]: feature has been removed
  --> /cargo/registry/src/github.com-1ecc6299db9ec823/lock_api-0.4.1/src/lib.rs:91:42
   |
91 | #![cfg_attr(feature = "nightly", feature(const_fn))]
   |                                          ^^^^^^^^ feature has been removed
   |
   = note: replaced by finer-grained feature flags
  • Something very strange is going on with the "const fn in trait" errors. In my attempt I made this a new feature gate, which however just lead to tons of duplicate errors.
  • Something else but equally strange is going on with some cases of having trait bounds on const fn, leading to errors disappearing in the min_const_fn test.

Cc @rust-lang/wg-const-eval

@RalfJung
Copy link
Member Author

Regarding "const fn in trait", I think what happens is that currently, parsing this is gated on const_fn but then still gets rejected later. So maybe the easiest way forward here would be to just reject this in the parser always, even under const_fn -- what do you think @oli-obk?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 24, 2021

How can we reject it in the parser? We need to know about bounds and similar which we usually check on the HIR I think?

@RalfJung
Copy link
Member Author

I mean this:

trait Foo {
  const fn bar();
}

I think this is not currently allowed under any conditions, is it? We accept it in the parser if const_fn is set, only to later reject it somewhere.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 24, 2021

I mean this:

trait Foo {
  const fn bar();
}

I think this is not currently allowed under any conditions, is it? We accept it in the parser if const_fn is set, only to later reject it somewhere.

oh... yea, just forbid that without any way to enable it

@RalfJung
Copy link
Member Author

Turns out we already forbid it:

self.check_trait_fn_not_const(sig.header.constness);

So, I think we can just remove the feature gate check.

@RalfJung
Copy link
Member Author

All right, opened #84544 for that.

What is still confusing me a lot is this one:


This is inside is_min_const_fn. What does that even mean? In the past I think we had two const-checkers, and is_min_const_fn decided whether the "minimal" or the "full" checker was used. Nowadays, we only have one checker, right? So should is_min_const_fn even still exist?
There is exactly one call site:
(tcx.is_const_fn_raw(def.did.to_def_id()), is_min_const_fn(tcx, def.did.to_def_id()))

The result is then passed to the unsafety checker where it has exactly one consequence:

I don't think whether something is "min" or "not-min" const fn should affect unsafety checking. Do you think we can remove this code (and just treat all functions like min_const_fn as far as unsafety checking is concerned)?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 25, 2021

yea, all mentions of "min_const_fn" should disappear and instead just be checks for whether the function is_const_fn_raw

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 25, 2021
Always reject `const fn` in `trait` during parsing.

'const fn' in trait are rejected in the AST:
https://github.com/rust-lang/rust/blob/b78c0d8a4d5af91a4a55d029293e3ecb879ec142/compiler/rustc_ast_passes/src/ast_validation.rs#L1411
So this feature gate check is a NOP and we can just remove it.

The src/test/ui/feature-gates/feature-gate-min_const_fn.rs and src/test/ui/feature-gates/feature-gate-const_fn.rs tests ensure that we still reject `const fn` in `trait`

Cc rust-lang#84510
r? `@oli-obk`
@RalfJung
Copy link
Member Author

RalfJung commented Apr 25, 2021

All right, #84547 is underway doing that.

This means there is just one reference to the const_fn feature gate left:

} else if !tcx.features().const_fn

This is probably what caused the strange side-effects last time...
since this is about trait bounds, maybe the most sensible thing to do is to use the const_fn_trait_bound feature gate instead of the const_fn feature gate?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 25, 2021

Yea, that sounds like the best thing to do here

@oli-obk
Copy link
Contributor

oli-obk commented Apr 25, 2021

Though I'm moderately confused about the inversion of the check

@oli-obk
Copy link
Contributor

oli-obk commented Apr 25, 2021

I would have thought that means we can just scrap the check entirely instead of using a different feature gate

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 25, 2021
Always reject `const fn` in `trait` during parsing.

'const fn' in trait are rejected in the AST:
https://github.com/rust-lang/rust/blob/b78c0d8a4d5af91a4a55d029293e3ecb879ec142/compiler/rustc_ast_passes/src/ast_validation.rs#L1411
So this feature gate check is a NOP and we can just remove it.

The src/test/ui/feature-gates/feature-gate-min_const_fn.rs and src/test/ui/feature-gates/feature-gate-const_fn.rs tests ensure that we still reject `const fn` in `trait`

Cc rust-lang#84510
r? ``@oli-obk``
@RalfJung
Copy link
Member Author

All right, with #84556 the last effect of const_fn is gone. :)
Once that lands, we can start removing const_fn from crates (in particular, lock_api) and tests, and then eventually mark the feature gate as "removed".

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 25, 2021
Get rid of is_min_const_fn

This removes the last trace of the min_const_fn mechanism by making the unsafety checker agnostic about whether something is a min or "non-min" const fn. It seems this distinction was used to disallow some features inside `const fn`, but that is the responsibility of the const checker, not of the unsafety checker. No test seems to even notice this change in the unsafety checker so I guess we are good...

r? `@oli-obk`
Cc rust-lang#84510
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 26, 2021
Always reject `const fn` in `trait` during parsing.

'const fn' in trait are rejected in the AST:
https://github.com/rust-lang/rust/blob/b78c0d8a4d5af91a4a55d029293e3ecb879ec142/compiler/rustc_ast_passes/src/ast_validation.rs#L1411
So this feature gate check is a NOP and we can just remove it.

The src/test/ui/feature-gates/feature-gate-min_const_fn.rs and src/test/ui/feature-gates/feature-gate-const_fn.rs tests ensure that we still reject `const fn` in `trait`

Cc rust-lang#84510
r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 29, 2021
use correct feature flag for impl-block-level trait bounds on const fn

I am not sure what that special hack was needed for, but it doesn't seem needed any more...

This removes the last use of the `const_fn` feature flag -- Cc rust-lang#84510
r? `@oli-obk`
@RalfJung
Copy link
Member Author

With #84556 having landed, the const_fn feature gate is now dead code. We can start removing it everywhere, and then eventually mark the feature gate itself as removed in the compiler (so it is rejected during the build).

@camelid camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. F-const_fn A-const-eval Area: constant evaluation (mir interpretation) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 6, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 9, 2021
@bors bors closed this as completed in 506e75c May 11, 2021
staktrace added a commit to staktrace/extprim that referenced this issue May 15, 2021
This feature gate was removed from rust in
rust-lang/rust#84510 and so
no longer compiles in rust nightly.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue May 20, 2021
bors bot added a commit to crossbeam-rs/crossbeam that referenced this issue May 30, 2021
707: Prepare for the next release r=taiki-e a=taiki-e

- crossbeam-epoch 0.9.4 -> 0.9.5
  - Fix UB in `Pointable` impl of `[MaybeUninit<T>]` (#694)
  - Support targets that do not have atomic CAS on stable Rust (#698)
  - Fix breakage with nightly feature due to rust-lang/rust#84510 (#692)
- crossbeam-queue 0.3.1 -> 0.3.2
  - Support targets that do not have atomic CAS on stable Rust (#698)
- crossbeam-utils 0.8.4 -> 0.8.5
  - Add `AtomicCell::fetch_update` (#704)
  - Support targets that do not have atomic CAS on stable Rust (#698)
- crossbeam 0.8.0 -> 0.8.1
  - Support targets that do not have atomic CAS on stable Rust (#698)

Closes #702 

Co-authored-by: Taiki Endo <te316e89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants