From 1b185975544fcab53f9bd23a74cb0997e8f40607 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 25 Aug 2019 15:44:22 +0200 Subject: [PATCH 1/2] Fix missing_const_for_fn false positive We don't want to lint if any of the input parameters implement drop. (constant functions cannot evaluate destructors) --- clippy_lints/src/missing_const_for_fn.rs | 20 ++++++++++++++--- .../ui/missing_const_for_fn/cant_be_const.rs | 22 +++++++++++++++++++ .../ui/missing_const_for_fn/could_be_const.rs | 15 +++++++++++++ 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 89398f82c9e..2f20aa9c683 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -1,10 +1,11 @@ -use crate::utils::{is_entrypoint_fn, span_lint, trait_ref_of_method}; +use crate::utils::{has_drop, is_entrypoint_fn, span_lint, trait_ref_of_method}; use rustc::hir; use rustc::hir::intravisit::FnKind; -use rustc::hir::{Body, Constness, FnDecl, HirId}; +use rustc::hir::{Body, Constness, FnDecl, HirId, HirVec}; use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintPass}; use rustc::{declare_lint_pass, declare_tool_lint}; use rustc_mir::transform::qualify_min_const_fn::is_min_const_fn; +use rustc_typeck::hir_ty_to_ty; use syntax_pos::Span; declare_clippy_lint! { @@ -94,7 +95,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { } }, FnKind::Method(_, sig, ..) => { - if trait_ref_of_method(cx, hir_id).is_some() || already_const(sig.header) { + if trait_ref_of_method(cx, hir_id).is_some() + || already_const(sig.header) + || method_accepts_dropable(cx, &sig.decl.inputs) + { return; } }, @@ -113,6 +117,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { } } +/// 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: &HirVec) -> bool { + // If any of the params are dropable, 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` fn already_const(header: hir::FnHeader) -> bool { header.constness == Constness::Const diff --git a/tests/ui/missing_const_for_fn/cant_be_const.rs b/tests/ui/missing_const_for_fn/cant_be_const.rs index 115cc954dc7..f367279906f 100644 --- a/tests/ui/missing_const_for_fn/cant_be_const.rs +++ b/tests/ui/missing_const_for_fn/cant_be_const.rs @@ -68,3 +68,25 @@ impl std::ops::Add for Point { Point(self.0 + other.0, self.1 + other.1) } } + +mod with_drop { + pub struct A; + pub struct B; + impl Drop for A { + fn drop(&mut self) {} + } + + impl A { + // This can not be const because the type implements `Drop`. + pub fn a(self) -> B { + B + } + } + + impl B { + // This can not be const because `a` implements `Drop`. + pub fn a(self, a: A) -> B { + B + } + } +} diff --git a/tests/ui/missing_const_for_fn/could_be_const.rs b/tests/ui/missing_const_for_fn/could_be_const.rs index 139e64de1ff..9109d255ca7 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.rs +++ b/tests/ui/missing_const_for_fn/could_be_const.rs @@ -53,5 +53,20 @@ fn generic_arr(t: [T; 1]) -> T { t[0] } +mod with_drop { + pub struct A; + pub struct B; + impl Drop for A { + fn drop(&mut self) {} + } + + impl B { + // This can be const, because `a` is passed by reference + pub fn b(self, a: &A) -> B { + B + } + } +} + // Should not be const fn main() {} From 5e1fdf9ae6e4b66083e420e14a3151171a2b868d Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 29 Aug 2019 18:37:43 +0200 Subject: [PATCH 2/2] Add missing UI test change --- tests/ui/missing_const_for_fn/could_be_const.stderr | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/ui/missing_const_for_fn/could_be_const.stderr b/tests/ui/missing_const_for_fn/could_be_const.stderr index 22ea852905d..0c0775764b7 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.stderr +++ b/tests/ui/missing_const_for_fn/could_be_const.stderr @@ -49,5 +49,13 @@ LL | | t LL | | } | |_^ -error: aborting due to 6 previous errors +error: this could be a const_fn + --> $DIR/could_be_const.rs:65:9 + | +LL | / pub fn b(self, a: &A) -> B { +LL | | B +LL | | } + | |_________^ + +error: aborting due to 7 previous errors