Remove `qualify_min_const_fn` #76850

ecstatic-morse · 2020-09-17T18:29:23Z

~~Blocked on #76807 (the first six commits).~~

With this PR, all checks in qualify_min_const_fn are replicated in check_consts, and the former is no longer invoked. My goal was to have as few changes to test output as possible, since making sweeping changes to the code while doing big batches of diagnostics updates turned out to be a headache. To this end, there's a few HACKs in check_consts to achieve parity with qualify_min_const_fn.

The new system that replaces is_min_const_fn is referred to as "const-stability" My end goal for the const-stability rules is this:

Const-stability is only applicable to functions defined in staged_api crates.
All functions not marked rustc_const_unstable are considered "const-stable".
- NB. This is currently not implemented. #[unstable] functions are also const-unstable. This causes problems when searching for feature gates.
- All "const-unstable" functions have an associated feature gate
const-stable functions can only call other const-stable functions
- allow_internal_unstable can be used to circumvent this.
All const-stable functions are subject to some additional checks (the ones that were unique to qualify_min_const_fn)

The plan is to remove each HACK individually in subsequent PRs. That way, changes to error message output can be reviewed in isolation.

ecstatic-morse · 2020-09-17T19:50:22Z

compiler/rustc_mir/src/transform/check_consts/validation.rs

+        if self.const_checking_stopped {
+            return;
+        }


We could get a bit closer to the existing behavior by disabling the "stop after a single error" stuff when #![feature(const_fn)] is enabled. However, this is temporary anyway.

ecstatic-morse · 2020-09-17T20:44:35Z

r? @oli-obk

Okay, I think I've done all I can to minimize the diff. After this, qualify_min_const_fn is unused. I can remove it later. There will be quite a few follow-up PRs to this one as I clean up the results of combining the logic from both passes. As always, meant to be reviewed commit-by-commit.

oli-obk

I love it, all the problems now have explicit sites "causing" them (though as you correctly noted, some should just happen in stability checking. I think some of the stability checking here was to prevent any worries about accidental stabilization.

oli-obk · 2020-09-18T09:49:30Z

compiler/rustc_mir/src/transform/check_consts/ops.rs

-/// Emits an error if `op` is not allowed in the given const context.
-pub fn non_const<O: NonConstOp>(ccx: &ConstCx<'_, '_>, op: O, span: Span) {
+/// Emits an error and returns `true` if `op` is not allowed in the given const context.
+pub fn non_const<O: NonConstOp>(ccx: &ConstCx<'_, '_>, op: O, span: Span) -> bool {


a bit too much negation going on for my taste, but inverting that can be delayed to a follow up

Yeah, it's possible that this helper function should folded back into the const-checker, since the return value and (unfortunately) the unstable_in_stable bit are unused in the post-elaboration live drop checker.

compiler/rustc_mir/src/transform/check_consts/validation.rs

This may not be strictly minimal, but all unstable functions also need a `rustc_const_unstable` attribute.

This is a hack for parity with `qualify_min_const_fn`, which only emitted a single error.

ecstatic-morse · 2020-09-22T18:06:19Z

Tests are passing, and Oli has approved it, so I'm gonna add it to the queue.

@bors r=oli-obk

bors · 2020-09-22T18:06:21Z

📌 Commit 186d148 has been approved by oli-obk

…tor, r=oli-obk Remove `qualify_min_const_fn` ~~Blocked on rust-lang#76807 (the first six commits).~~ With this PR, all checks in `qualify_min_const_fn` are replicated in `check_consts`, and the former is no longer invoked. My goal was to have as few changes to test output as possible, since making sweeping changes to the code *while* doing big batches of diagnostics updates turned out to be a headache. To this end, there's a few `HACK`s in `check_consts` to achieve parity with `qualify_min_const_fn`. The new system that replaces `is_min_const_fn` is referred to as "const-stability" My end goal for the const-stability rules is this: * Const-stability is only applicable to functions defined in `staged_api` crates. * All functions not marked `rustc_const_unstable` are considered "const-stable". - NB. This is currently not implemented. `#[unstable]` functions are also const-unstable. This causes problems when searching for feature gates. - All "const-unstable" functions have an associated feature gate * const-stable functions can only call other const-stable functions - `allow_internal_unstable` can be used to circumvent this. * All const-stable functions are subject to some additional checks (the ones that were unique to `qualify_min_const_fn`) The plan is to remove each `HACK` individually in subsequent PRs. That way, changes to error message output can be reviewed in isolation.

bors · 2020-09-23T05:40:41Z

⌛ Testing commit 186d148 with merge f6d5920...

bors · 2020-09-23T08:22:53Z

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing f6d5920 to master...

RalfJung · 2020-09-24T19:57:54Z

This is great. :) @ecstatic-morse do you think this would be worth documenting in https://github.com/rust-lang/const-eval/?
I am particularly curious about what these "additional checks" are:

All const-stable functions are subject to some additional checks (the ones that were unique to qualify_min_const_fn)

ecstatic-morse · 2020-09-24T20:10:07Z

I believe it's as simple as "const-stable functions can only call other const-stable functions and cannot depend on any unstable features to pass const-checking". My parenthetical isn't very helpful, since these were only two of the dozen or so checks that were unique to qualify_min_const_fn.

RalfJung · 2020-09-24T20:26:13Z

Ah I see.

Also, shouldn't a PR with this title actually delete the qualify_min_const_fn.rs file, or is that still doing something?

RalfJung · 2020-09-24T20:28:00Z

Oh, that is still open in #77128. I guess I do not understand what this PR is "removing" then, the description sounds more like it is porting all the min_const_fn checks over to the main checker (but keeps the old checks active).

ecstatic-morse · 2020-09-24T20:30:33Z

qualify_min_const_fn.rs is now only used by clippy. I learned about that dependency when I went to remove it as the last commit.

rust-highfive assigned matthewjasper Sep 17, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2020

rust-lang deleted a comment from rust-highfive Sep 17, 2020

ecstatic-morse unassigned matthewjasper Sep 17, 2020

ecstatic-morse force-pushed the const-checking-refactor branch 3 times, most recently from e957778 to de11a42 Compare September 17, 2020 19:45

ecstatic-morse commented Sep 17, 2020

View reviewed changes

ecstatic-morse force-pushed the const-checking-refactor branch from 13880fc to 3e04245 Compare September 17, 2020 20:09

rust-highfive assigned oli-obk Sep 17, 2020

ecstatic-morse marked this pull request as ready for review September 17, 2020 20:44

jyn514 added A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 17, 2020

oli-obk reviewed Sep 18, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the const-checking-refactor branch from a47e76e to b6fe533 Compare September 22, 2020 17:05

ecstatic-morse added 11 commits September 22, 2020 10:05

Useful derives on ops::Status

3dd28c7

Update library functions with stability attributes

110e59e

This may not be strictly minimal, but all unstable functions also need a `rustc_const_unstable` attribute.

Add const-stability helpers

a173c5c

Return true if check_const emits an error

7fb9587

Allow errors to abort const checking when emitted

bfc10a8

This is a hack for parity with `qualify_min_const_fn`, which only emitted a single error.

Add structured errors for qualify_min_const_fn checks

5ee5429

Update const-checker to replicate qualify_min_const_fn

3569bb6

Bless tests

ef6d427

Bless compile-fail

9be3d10

Use the same name everywhere for is_const_stable_const_fn

d60e204

Replace missing comment

08e3822

ecstatic-morse force-pushed the const-checking-refactor branch from b6fe533 to 08e3822 Compare September 22, 2020 17:06

Add #![feature(const_fn_transmute)] to rustc_ast

6044836

Use correct feature gate for unsizing casts

186d148

oli-obk approved these changes Sep 22, 2020

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 22, 2020

ecstatic-morse mentioned this pull request Sep 22, 2020

Rollup of 13 pull requests #77082

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2020

bors merged commit f6d5920 into rust-lang:master Sep 23, 2020

rustbot added this to the 1.48.0 milestone Sep 23, 2020

This was referenced Sep 23, 2020

Make delegation methods of std::net::IpAddr unstably const #76304

Merged

stabilize const_int_pow #76829

Merged

This was referenced Sep 23, 2020

Add speculative const-checking and remove qualify_min_const_fn.rs #77128

Closed

Suggest const_mut_refs, not const_fn for mutable references in const fn #77136

Merged

#![feature(const_fn)] doesn't work #77134

Closed

josephlr mentioned this pull request Sep 24, 2020

Regression for #![feature(const_in_array_repeat_expressions)] #77138

Closed

ecstatic-morse mentioned this pull request Sep 24, 2020

Suggest const_fn_transmute, not const_fn #77160

Merged

ecstatic-morse mentioned this pull request Sep 25, 2020

Remove qualify_min_const_fn and #![feature(const_fn)] #76618

Closed

2 tasks

tesuji mentioned this pull request Sep 30, 2020

async blocks in a const-context error on nightly, but not on stable #77361

Closed

ecstatic-morse deleted the const-checking-refactor branch October 6, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `qualify_min_const_fn` #76850

Remove `qualify_min_const_fn` #76850

ecstatic-morse commented Sep 17, 2020 •

edited

Loading

ecstatic-morse Sep 17, 2020 •

edited

Loading

ecstatic-morse commented Sep 17, 2020 •

edited

Loading

oli-obk left a comment

oli-obk Sep 18, 2020

ecstatic-morse Sep 18, 2020

This comment has been minimized.

ecstatic-morse commented Sep 22, 2020

bors commented Sep 22, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

RalfJung commented Sep 24, 2020

ecstatic-morse commented Sep 24, 2020

RalfJung commented Sep 24, 2020

RalfJung commented Sep 24, 2020

ecstatic-morse commented Sep 24, 2020 •

edited

Loading

Remove qualify_min_const_fn #76850

Remove qualify_min_const_fn #76850

Conversation

ecstatic-morse commented Sep 17, 2020 • edited Loading

ecstatic-morse Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse commented Sep 17, 2020 • edited Loading

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk Sep 18, 2020

Choose a reason for hiding this comment

ecstatic-morse Sep 18, 2020

Choose a reason for hiding this comment

This comment has been minimized.

ecstatic-morse commented Sep 22, 2020

bors commented Sep 22, 2020

bors commented Sep 23, 2020

bors commented Sep 23, 2020

RalfJung commented Sep 24, 2020

ecstatic-morse commented Sep 24, 2020

RalfJung commented Sep 24, 2020

RalfJung commented Sep 24, 2020

ecstatic-morse commented Sep 24, 2020 • edited Loading

Remove `qualify_min_const_fn` #76850

Remove `qualify_min_const_fn` #76850

ecstatic-morse commented Sep 17, 2020 •

edited

Loading

ecstatic-morse Sep 17, 2020 •

edited

Loading

ecstatic-morse commented Sep 17, 2020 •

edited

Loading

ecstatic-morse commented Sep 24, 2020 •

edited

Loading