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.

Sign up for GitHub

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

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

Closed
wants to merge 7 commits into from
+127 −555
Closed

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

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e900f1f
Remove `qualify_min_const_fn.rs`
ecstatic-morse Sep 23, 2020
e89e935
Remove `non_const_op` helper
ecstatic-morse Sep 23, 2020
2ca256a
Record drops that would be live without `const_precise_live_drops`
ecstatic-morse Sep 23, 2020
8095303
Expose speculative const-checking for `clippy`
ecstatic-morse Sep 23, 2020
70e70de
Update clippy to use speculative const-checking
ecstatic-morse Sep 23, 2020
34b821f
Run const-checking on optimized MIR in clippy
ecstatic-morse Sep 24, 2020
5b51326
Work around clippy bug in const-checking
ecstatic-morse Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 4 additions & 20 deletions src/tools/clippy/clippy_lints/src/missing_const_for_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
use crate::utils::{fn_has_unsatisfiable_preds, has_drop, is_entrypoint_fn, span_lint, trait_ref_of_method};
use crate::utils::{fn_has_unsatisfiable_preds, is_entrypoint_fn, span_lint, trait_ref_of_method};
use rustc_hir as hir;
use rustc_hir::intravisit::FnKind;
use rustc_hir::{Body, Constness, FnDecl, GenericParamKind, HirId};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::lint::in_external_macro;
use rustc_mir::transform::qualify_min_const_fn::is_min_const_fn;
use rustc_middle::ty::WithOptConstParam;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::Span;
use rustc_typeck::hir_ty_to_ty;

declare_clippy_lint! {
/// **What it does:**
Expand Down Expand Up @@ -108,36 +107,21 @@ impl<'tcx> LateLintPass<'tcx> for MissingConstForFn {
FnKind::Method(_, sig, ..) => {
if trait_ref_of_method(cx, hir_id).is_some()
|| already_const(sig.header)
|| method_accepts_dropable(cx, sig.decl.inputs)
{
return;
}
},
FnKind::Closure(..) => return,
}

let mir = cx.tcx.optimized_mir(def_id);
let mir = cx.tcx.mir_const(WithOptConstParam::unknown(def_id)).borrow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if let Err((span, err)) = is_min_const_fn(cx.tcx, def_id.to_def_id(), &mir) {
if rustc_mir::const_eval::is_min_const_fn(cx.tcx, def_id.to_def_id()) {
cx.tcx.sess.span_err(span, &err);
}
} else {
if rustc_mir::transform::check_consts::non_const_fn_could_be_made_stable_const_fn(cx.tcx, def_id, &mir) {
span_lint(cx, MISSING_CONST_FOR_FN, span, "this could be a `const fn`");
}
}
}

/// Returns true if any of the method parameters is a type that implements `Drop`. The method
/// can't be made const then, because `drop` can't be const-evaluated.
fn method_accepts_dropable(cx: &LateContext<'_>, param_tys: &[hir::Ty<'_>]) -> bool {
// If any of the params are droppable, return true
param_tys.iter().any(|hir_ty| {
let ty_ty = hir_ty_to_ty(cx.tcx, hir_ty);
has_drop(cx, ty_ty)
})
}

// We don't have to lint on something that's already `const`
#[must_use]
fn already_const(header: hir::FnHeader) -> bool {
Expand Down