From 26a1989041393116c001f17d0323d6b4fc989dd9 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Tue, 13 Apr 2021 17:13:49 +1200 Subject: [PATCH] Fix a FP in `missing_const_for_fn` where a function that calls a standard library function whose constness is unstable is considered as being able to be a const function --- clippy_lints/src/missing_const_for_fn.rs | 2 +- clippy_utils/src/lib.rs | 1 + clippy_utils/src/qualify_min_const_fn.rs | 40 +++++++++++++++++-- .../missing_const_for_fn/auxiliary/helper.rs | 8 ++++ .../ui/missing_const_for_fn/cant_be_const.rs | 19 +++++++++ .../ui/missing_const_for_fn/could_be_const.rs | 10 +++++ .../could_be_const.stderr | 26 +++++++----- 7 files changed, 92 insertions(+), 14 deletions(-) create mode 100644 tests/ui/missing_const_for_fn/auxiliary/helper.rs diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 0dc02431ad5..93b7a897405 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -138,7 +138,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingConstForFn { let mir = cx.tcx.optimized_mir(def_id); - if let Err((span, err)) = is_min_const_fn(cx.tcx, mir) { + if let Err((span, err)) = is_min_const_fn(cx.tcx, mir, self.msrv.as_ref()) { if rustc_mir::const_eval::is_min_const_fn(cx.tcx, def_id.to_def_id()) { cx.tcx.sess.span_err(span, &err); } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index e99ffbe269b..8c20f8a0fbc 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -9,6 +9,7 @@ // (Currently there is no way to opt into sysroot crates without `extern crate`.) extern crate rustc_ast; extern crate rustc_ast_pretty; +extern crate rustc_attr; extern crate rustc_data_structures; extern crate rustc_errors; extern crate rustc_hir; diff --git a/clippy_utils/src/qualify_min_const_fn.rs b/clippy_utils/src/qualify_min_const_fn.rs index b52cbf31e35..b2ce58b597b 100644 --- a/clippy_utils/src/qualify_min_const_fn.rs +++ b/clippy_utils/src/qualify_min_const_fn.rs @@ -1,3 +1,8 @@ +// This code used to be a part of `rustc` but moved to Clippy as a result of +// https://github.com/rust-lang/rust/issues/76618. Because of that, it contains unused code and some +// of terminologies might not be relevant in the context of Clippy. Note that its behavior might +// differ from the time of `rustc` even if the name stays the same. + use rustc_hir as hir; use rustc_hir::def_id::DefId; use rustc_middle::mir::{ @@ -6,6 +11,7 @@ use rustc_middle::mir::{ }; use rustc_middle::ty::subst::GenericArgKind; use rustc_middle::ty::{self, adjustment::PointerCast, Ty, TyCtxt}; +use rustc_semver::RustcVersion; use rustc_span::symbol::sym; use rustc_span::Span; use rustc_target::spec::abi::Abi::RustIntrinsic; @@ -13,7 +19,7 @@ use std::borrow::Cow; type McfResult = Result<(), (Span, Cow<'static, str>)>; -pub fn is_min_const_fn(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>) -> McfResult { +pub fn is_min_const_fn(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, msrv: Option<&RustcVersion>) -> McfResult { let def_id = body.source.def_id(); let mut current = def_id; loop { @@ -70,7 +76,7 @@ pub fn is_min_const_fn(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>) -> McfResult { )?; for bb in body.basic_blocks() { - check_terminator(tcx, body, bb.terminator())?; + check_terminator(tcx, body, bb.terminator(), msrv)?; for stmt in &bb.statements { check_statement(tcx, body, def_id, stmt)?; } @@ -268,7 +274,12 @@ fn check_place(tcx: TyCtxt<'tcx>, place: Place<'tcx>, span: Span, body: &Body<'t Ok(()) } -fn check_terminator(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, terminator: &Terminator<'tcx>) -> McfResult { +fn check_terminator( + tcx: TyCtxt<'tcx>, + body: &'a Body<'tcx>, + terminator: &Terminator<'tcx>, + msrv: Option<&RustcVersion>, +) -> McfResult { let span = terminator.source_info.span; match &terminator.kind { TerminatorKind::FalseEdge { .. } @@ -305,7 +316,7 @@ fn check_terminator(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, terminator: &Termin } => { let fn_ty = func.ty(body, tcx); if let ty::FnDef(fn_def_id, _) = *fn_ty.kind() { - if !rustc_mir::const_eval::is_min_const_fn(tcx, fn_def_id) { + if !is_const_fn(tcx, fn_def_id, msrv) { return Err(( span, format!( @@ -350,3 +361,24 @@ fn check_terminator(tcx: TyCtxt<'tcx>, body: &'a Body<'tcx>, terminator: &Termin TerminatorKind::InlineAsm { .. } => Err((span, "cannot use inline assembly in const fn".into())), } } + +fn is_const_fn(tcx: TyCtxt<'_>, def_id: DefId, msrv: Option<&RustcVersion>) -> bool { + rustc_mir::const_eval::is_const_fn(tcx, def_id) + && if let Some(const_stab) = tcx.lookup_const_stability(def_id) { + if let rustc_attr::StabilityLevel::Stable { since } = const_stab.level { + // Checking MSRV is manually necessary because `rustc` has no such concept. This entire + // function could be removed if `rustc` provided a MSRV-aware version of `is_const_fn`. + // as a part of an unimplemented MSRV check https://github.com/rust-lang/rust/issues/65262. + crate::meets_msrv( + msrv, + &RustcVersion::parse(&since.as_str()) + .expect("`rustc_attr::StabilityLevel::Stable::since` is ill-formatted"), + ) + } else { + // `rustc_mir::const_eval::is_const_fn` should return false for unstably const functions. + unreachable!(); + } + } else { + true + } +} diff --git a/tests/ui/missing_const_for_fn/auxiliary/helper.rs b/tests/ui/missing_const_for_fn/auxiliary/helper.rs new file mode 100644 index 00000000000..7b9dc76b8f1 --- /dev/null +++ b/tests/ui/missing_const_for_fn/auxiliary/helper.rs @@ -0,0 +1,8 @@ +// This file provides a const function that is unstably const forever. + +#![feature(staged_api)] +#![stable(feature = "1", since = "1.0.0")] + +#[stable(feature = "1", since = "1.0.0")] +#[rustc_const_unstable(feature = "foo", issue = "none")] +pub const fn unstably_const_fn() {} 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 ba352ef9ee9..7cda1aaa3c2 100644 --- a/tests/ui/missing_const_for_fn/cant_be_const.rs +++ b/tests/ui/missing_const_for_fn/cant_be_const.rs @@ -2,9 +2,14 @@ //! compilation error. //! The .stderr output of this test should be empty. Otherwise it's a bug somewhere. +// aux-build:helper.rs + #![warn(clippy::missing_const_for_fn)] #![allow(incomplete_features)] #![feature(start, const_generics)] +#![feature(custom_inner_attributes)] + +extern crate helper; struct Game; @@ -101,3 +106,17 @@ fn const_generic_return(t: &[T]) -> &[T; N] { unsafe { &*p } } + +// Do not lint this because it calls a function whose constness is unstable. +fn unstably_const_fn() { + helper::unstably_const_fn() +} + +mod const_fn_stabilized_after_msrv { + #![clippy::msrv = "1.46.0"] + + // Do not lint this because `u8::is_ascii_digit` is stabilized as a const function in 1.47.0. + fn const_fn_stabilized_after_msrv(byte: u8) { + byte.is_ascii_digit(); + } +} 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 c6f44b7daa3..0accb516f5f 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.rs +++ b/tests/ui/missing_const_for_fn/could_be_const.rs @@ -1,6 +1,7 @@ #![warn(clippy::missing_const_for_fn)] #![allow(incomplete_features, clippy::let_and_return)] #![feature(const_generics)] +#![feature(custom_inner_attributes)] use std::mem::transmute; @@ -70,5 +71,14 @@ mod with_drop { } } +mod const_fn_stabilized_before_msrv { + #![clippy::msrv = "1.47.0"] + + // This could be const because `u8::is_ascii_digit` is a stable const function in 1.47. + fn const_fn_stabilized_before_msrv(byte: u8) { + byte.is_ascii_digit(); + } +} + // Should not be const fn main() {} 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 74d32b8a1aa..63c211f39fa 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.stderr +++ b/tests/ui/missing_const_for_fn/could_be_const.stderr @@ -1,5 +1,5 @@ error: this could be a `const fn` - --> $DIR/could_be_const.rs:13:5 + --> $DIR/could_be_const.rs:14:5 | LL | / pub fn new() -> Self { LL | | Self { guess: 42 } @@ -9,7 +9,7 @@ LL | | } = note: `-D clippy::missing-const-for-fn` implied by `-D warnings` error: this could be a `const fn` - --> $DIR/could_be_const.rs:17:5 + --> $DIR/could_be_const.rs:18:5 | LL | / fn const_generic_params<'a, T, const N: usize>(&self, b: &'a [T; N]) -> &'a [T; N] { LL | | b @@ -17,7 +17,7 @@ LL | | } | |_____^ error: this could be a `const fn` - --> $DIR/could_be_const.rs:23:1 + --> $DIR/could_be_const.rs:24:1 | LL | / fn one() -> i32 { LL | | 1 @@ -25,7 +25,7 @@ LL | | } | |_^ error: this could be a `const fn` - --> $DIR/could_be_const.rs:28:1 + --> $DIR/could_be_const.rs:29:1 | LL | / fn two() -> i32 { LL | | let abc = 2; @@ -34,7 +34,7 @@ LL | | } | |_^ error: this could be a `const fn` - --> $DIR/could_be_const.rs:34:1 + --> $DIR/could_be_const.rs:35:1 | LL | / fn string() -> String { LL | | String::new() @@ -42,7 +42,7 @@ LL | | } | |_^ error: this could be a `const fn` - --> $DIR/could_be_const.rs:39:1 + --> $DIR/could_be_const.rs:40:1 | LL | / unsafe fn four() -> i32 { LL | | 4 @@ -50,7 +50,7 @@ LL | | } | |_^ error: this could be a `const fn` - --> $DIR/could_be_const.rs:44:1 + --> $DIR/could_be_const.rs:45:1 | LL | / fn generic(t: T) -> T { LL | | t @@ -58,12 +58,20 @@ LL | | } | |_^ error: this could be a `const fn` - --> $DIR/could_be_const.rs:67:9 + --> $DIR/could_be_const.rs:68:9 | LL | / pub fn b(self, a: &A) -> B { LL | | B LL | | } | |_________^ -error: aborting due to 8 previous errors +error: this could be a `const fn` + --> $DIR/could_be_const.rs:78:5 + | +LL | / fn const_fn_stabilized_before_msrv(byte: u8) { +LL | | byte.is_ascii_digit(); +LL | | } + | |_____^ + +error: aborting due to 9 previous errors