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

ecstatic-morse · 2020-09-23T23:49:37Z

This removes qualify_min_const_fn.rs entirely. It remained after #76850 because it was still used for a clippy lint. To continue to support clippy, I added a speculative mode to the const-checker, which is exposed via the non_const_fn_could_be_made_stable_const_fn method.

r? @oli-obk

Most of the extra logic was unused in the post-drop-elaboration checker.

ecstatic-morse · 2020-09-23T23:52:57Z

I haven't run the clippy tests locally (I'm not sure how), so this probably won't pass CI. I'm working on it.

Also, you might have noticed that I use early returns instead of else-if whenever possible. Usually I find this much more readable, but that might not be the case here.

Also removes some checks that were overzealous.

oli-obk · 2020-09-24T15:32:46Z

src/tools/clippy/clippy_lints/src/missing_const_for_fn.rs

                {
                    return;
                }
            },
            FnKind::Closure(..) => return,
        }

-        let mir = cx.tcx.optimized_mir(def_id);
+        let mir = cx.tcx.mir_const(WithOptConstParam::unknown(def_id)).borrow();


whoa, how does this work? Couldn't it already have been stolen?

Clippy tests don't run on PR CI, and I still can't run them locally due to some error about "two copies of the regex crate".

I don't know if clippy has a mechanism for ordering lints. It's not correct to const-check optimized MIR, since optimizations (especially drop elaboration) could have removed invalid operations. This was true for qualify_min_const_fn as well. Can someone from @rust-lang/clippy weigh in here?

Okay, ./x.py clean seemed to work. Besides being incorrect, running const-checking on optimized MIR causes a bunch of assertions to fire, since we don't reason about promoteds inside const-checking. I guess we'll have to disable the lint until someone tells us how to order the execution of clippy lints.

clippy could override the query that steals mir_const and just not steal it. Right now we're doing a hacky thing (

rust/src/tools/clippy/src/driver.rs

Line 86 in 87d262a

// FIXME: #4825; This is required, because Clippy lints that are based on MIR have to be

) but maybe we could just inject a different query.

cc @rust-lang/clippy are you ok with turning off the "this could be const fn" lint until we figure out how to get it to use exactly the same analysis as the one used by rustc?

This passes clippy tests locally now that I've commented out a single assertion. It was just the one ICE in every test. I think we could live with it (that assertion was more of a sanity check), but it doesn't feel great.

Clippy is tested by running ./x.py test src/tools/clippy

If it works like Miri, test --stage 0 should also work (and be much faster as rustc is built only once).

I tried started with stage 0 but got the bug about two rlibs for regex. Can anyone reproduce? Doing a stage 1 build just to run clippy tests is pretty painful, especially since they don't run on CI.

the regex thing is a longstanding problem with compiletest-rs that we've not really been able to fix. I don't know if https://crates.io/crates/trybuild fares better, maybe we should try it out

Miri also uses compiletest and its stage 0 tests work fine...

in miri we don't use any crates in our unit tests I think

I really hate having to do this, since I don't think unstable compiler APIs should have to work around misuse by external crates. However, this is the only way this change will pass CI without a ton of modifications to clippy and/or involvement of the clippy team.

src/tools/clippy/clippy_lints/src/missing_const_for_fn.rs

RalfJung · 2020-09-25T06:27:02Z

Cc @rust-lang/wg-const-eval

ecstatic-morse · 2020-09-25T17:32:57Z

Okay, so I'm gonna close this for now. I'll let someone from clippy move qualify_min_const_fn into tree and/or disable the lint, since I don't really know what's involved. When clippy figures out how to work around stolen queries, we can reopen it.

Also Clippy has set the mir-opt-flag to 0, so no MIR optimizations are run for Clippy lints.

@flip1995 This statement is misleading, and the misunderstanding underlying it will likely cause issues in the future, definitely with this lint and maybe with others. At mir-opt-level=0, we still run all the post_borrowck_cleanup passes, which remove dead code (SimplifyBranches, SimplifyCfg) and do drop elaboration, along with some other nontrivial stuff. We also do constant propagation at mir-opt-level=0, although this is more like a bug in rustc.

RalfJung · 2020-09-25T21:22:26Z

Should we have an issue to track this pending cleanup?

ecstatic-morse · 2020-09-25T23:06:35Z

Added a note to #76618 (comment) and rewrote rust-lang/rust-clippy#6080.

ecstatic-morse added 4 commits September 23, 2020 13:14

Remove qualify_min_const_fn.rs

e900f1f

Remove non_const_op helper

e89e935

Most of the extra logic was unused in the post-drop-elaboration checker.

Record drops that would be live without const_precise_live_drops

2ca256a

Expose speculative const-checking for clippy

8095303

rust-highfive assigned oli-obk Sep 23, 2020

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

Update clippy to use speculative const-checking

70e70de

Also removes some checks that were overzealous.

ecstatic-morse force-pushed the remove-qualify-min-const-fn branch from a141a97 to 70e70de Compare September 24, 2020 00:03

oli-obk reviewed Sep 24, 2020

View reviewed changes

ecstatic-morse added 2 commits September 24, 2020 11:03

Run const-checking on optimized MIR in clippy

34b821f

ecstatic-morse force-pushed the remove-qualify-min-const-fn branch from 3d76006 to 5b51326 Compare September 24, 2020 18:11

RalfJung mentioned this pull request Sep 24, 2020

Remove qualify_min_const_fn #76850

Merged

phansch reviewed Sep 25, 2020

View reviewed changes

src/tools/clippy/clippy_lints/src/missing_const_for_fn.rs Show resolved Hide resolved

ecstatic-morse closed this Sep 25, 2020

ecstatic-morse mentioned this pull request Sep 25, 2020

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

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ecstatic-morse commented Sep 23, 2020 •

edited

Loading

ecstatic-morse commented Sep 23, 2020 •

edited

Loading

oli-obk Sep 24, 2020

ecstatic-morse Sep 24, 2020

ecstatic-morse Sep 24, 2020 •

edited

Loading

oli-obk Sep 24, 2020

ecstatic-morse Sep 24, 2020

RalfJung Sep 25, 2020

ecstatic-morse Sep 25, 2020

oli-obk Sep 25, 2020

RalfJung Sep 25, 2020

oli-obk Sep 25, 2020

RalfJung commented Sep 25, 2020

ecstatic-morse commented Sep 25, 2020 •

edited

Loading

RalfJung commented Sep 25, 2020

ecstatic-morse commented Sep 25, 2020 •

edited

Loading

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

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

Conversation

ecstatic-morse commented Sep 23, 2020 • edited Loading

ecstatic-morse commented Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Sep 25, 2020

ecstatic-morse commented Sep 25, 2020 • edited Loading

RalfJung commented Sep 25, 2020

ecstatic-morse commented Sep 25, 2020 • edited Loading

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

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

ecstatic-morse commented Sep 23, 2020 •

edited

Loading

ecstatic-morse commented Sep 23, 2020 •

edited

Loading

ecstatic-morse Sep 24, 2020 •

edited

Loading

ecstatic-morse commented Sep 25, 2020 •

edited

Loading

ecstatic-morse commented Sep 25, 2020 •

edited

Loading