From c3980bf0bc1641717d3253f9168dec07b52e0e4f Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 9 Jan 2019 20:11:37 +0100 Subject: [PATCH 1/7] Add initial version of const_fn lint --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 5 + clippy_lints/src/missing_const_for_fn.rs | 115 ++++++++++++++++++ clippy_lints/src/utils/mod.rs | 13 ++ .../ui/missing_const_for_fn/cant_be_const.rs | 50 ++++++++ .../missing_const_for_fn/cant_be_const.stderr | 0 .../ui/missing_const_for_fn/could_be_const.rs | 53 ++++++++ .../could_be_const.stderr | 42 +++++++ 9 files changed, 280 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/missing_const_for_fn.rs create mode 100644 tests/ui/missing_const_for_fn/cant_be_const.rs create mode 100644 tests/ui/missing_const_for_fn/cant_be_const.stderr create mode 100644 tests/ui/missing_const_for_fn/could_be_const.rs create mode 100644 tests/ui/missing_const_for_fn/could_be_const.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index b773422b01507..c0679d280c266 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -878,6 +878,7 @@ All notable changes to this project will be documented in this file. [`min_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_max [`misaligned_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#misaligned_transmute [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op +[`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes diff --git a/README.md b/README.md index 6473b8efc5480..dad18ef756901 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 292 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 293 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4683d353ccf87..3483aae0ca3bc 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -23,6 +23,8 @@ extern crate rustc_data_structures; #[allow(unused_extern_crates)] extern crate rustc_errors; #[allow(unused_extern_crates)] +extern crate rustc_mir; +#[allow(unused_extern_crates)] extern crate rustc_plugin; #[allow(unused_extern_crates)] extern crate rustc_target; @@ -144,6 +146,7 @@ pub mod methods; pub mod minmax; pub mod misc; pub mod misc_early; +pub mod missing_const_for_fn; pub mod missing_doc; pub mod missing_inline; pub mod multiple_crate_versions; @@ -486,6 +489,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box slow_vector_initialization::Pass); reg.register_late_lint_pass(box types::RefToMut); reg.register_late_lint_pass(box assertions_on_constants::AssertionsOnConstants); + reg.register_late_lint_pass(box missing_const_for_fn::MissingConstForFn); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -1027,6 +1031,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::nursery", Some("clippy_nursery"), vec![ attrs::EMPTY_LINE_AFTER_OUTER_ATTR, fallible_impl_from::FALLIBLE_IMPL_FROM, + missing_const_for_fn::MISSING_CONST_FOR_FN, mutex_atomic::MUTEX_INTEGER, needless_borrow::NEEDLESS_BORROW, redundant_clone::REDUNDANT_CLONE, diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs new file mode 100644 index 0000000000000..4751a538ebd6c --- /dev/null +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -0,0 +1,115 @@ +use rustc::hir; +use rustc::hir::{Body, FnDecl, Constness}; +use rustc::hir::intravisit::FnKind; +// use rustc::mir::*; +use syntax::ast::{NodeId, Attribute}; +use syntax_pos::Span; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::{declare_tool_lint, lint_array}; +use rustc_mir::transform::qualify_min_const_fn::is_min_const_fn; +use crate::utils::{span_lint, is_entrypoint_fn}; + +/// **What it does:** +/// +/// Suggests the use of `const` in functions and methods where possible +/// +/// **Why is this bad?** +/// Not using `const` is a missed optimization. Instead of having the function execute at runtime, +/// when using `const`, it's evaluated at compiletime. +/// +/// **Known problems:** +/// +/// Const functions are currently still being worked on, with some features only being available +/// on nightly. This lint does not consider all edge cases currently and the suggestions may be +/// incorrect if you are using this lint on stable. +/// +/// Also, the lint only runs one pass over the code. Consider these two non-const functions: +/// +/// ```rust +/// fn a() -> i32 { 0 } +/// fn b() -> i32 { a() } +/// ``` +/// +/// When running Clippy, the lint will only suggest to make `a` const, because `b` at this time +/// can't be const as it calls a non-const function. Making `a` const and running Clippy again, +/// will suggest to make `b` const, too. +/// +/// **Example:** +/// +/// ```rust +/// fn new() -> Self { +/// Self { +/// random_number: 42 +/// } +/// } +/// ``` +/// +/// Could be a const fn: +/// +/// ```rust +/// const fn new() -> Self { +/// Self { +/// random_number: 42 +/// } +/// } +/// ``` +declare_clippy_lint! { + pub MISSING_CONST_FOR_FN, + nursery, + "Lint functions definitions that could be made `const fn`" +} + +#[derive(Clone)] +pub struct MissingConstForFn; + +impl LintPass for MissingConstForFn { + fn get_lints(&self) -> LintArray { + lint_array!(MISSING_CONST_FOR_FN) + } + + fn name(&self) -> &'static str { + "MissingConstForFn" + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { + fn check_fn( + &mut self, + cx: &LateContext<'_, '_>, + kind: FnKind<'_>, + _: &FnDecl, + _: &Body, + span: Span, + node_id: NodeId + ) { + let def_id = cx.tcx.hir().local_def_id(node_id); + let mir = cx.tcx.optimized_mir(def_id); + if let Ok(_) = is_min_const_fn(cx.tcx, def_id, &mir) { + match kind { + FnKind::ItemFn(name, _generics, header, _vis, attrs) => { + if !can_be_const_fn(&name.as_str(), header, attrs) { + return; + } + }, + FnKind::Method(ident, sig, _vis, attrs) => { + let header = sig.header; + let name = ident.name.as_str(); + if !can_be_const_fn(&name, header, attrs) { + return; + } + }, + _ => return + } + span_lint(cx, MISSING_CONST_FOR_FN, span, "this could be a const_fn"); + } + } +} + +fn can_be_const_fn(name: &str, header: hir::FnHeader, attrs: &[Attribute]) -> bool { + // Main and custom entrypoints can't be `const` + if is_entrypoint_fn(name, attrs) { return false } + + // We don't have to lint on something that's already `const` + if header.constness == Constness::Const { return false } + true +} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 2b2b3e8b2f6a5..f06a257b5ca48 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -350,6 +350,19 @@ pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option bool { + + let is_custom_entrypoint = attrs.iter().any(|attr| { + attr.path.segments.len() == 1 + && attr.path.segments[0].ident.to_string() == "start" + }); + + is_custom_entrypoint || fn_name == "main" +} + /// Get the name of the item the expression is in, if available. pub fn get_item_name(cx: &LateContext<'_, '_>, expr: &Expr) -> Option { let parent_id = cx.tcx.hir().get_parent(expr.id); diff --git a/tests/ui/missing_const_for_fn/cant_be_const.rs b/tests/ui/missing_const_for_fn/cant_be_const.rs new file mode 100644 index 0000000000000..5f00035b3ad36 --- /dev/null +++ b/tests/ui/missing_const_for_fn/cant_be_const.rs @@ -0,0 +1,50 @@ +//! False-positive tests to ensure we don't suggest `const` for things where it would cause a +//! compilation error. +//! The .stderr output of this test should be empty. Otherwise it's a bug somewhere. + +#![warn(clippy::missing_const_for_fn)] +#![feature(start)] + +struct Game; + +// This should not be linted because it's already const +const fn already_const() -> i32 { 32 } + +impl Game { + // This should not be linted because it's already const + pub const fn already_const() -> i32 { 32 } +} + +// Allowing on this function, because it would lint, which we don't want in this case. +#[allow(clippy::missing_const_for_fn)] +fn random() -> u32 { 42 } + +// We should not suggest to make this function `const` because `random()` is non-const +fn random_caller() -> u32 { + random() +} + +static Y: u32 = 0; + +// We should not suggest to make this function `const` because const functions are not allowed to +// refer to a static variable +fn get_y() -> u32 { + Y + //~^ ERROR E0013 +} + +// Also main should not be suggested to be made const +fn main() { + // We should also be sure to not lint on closures + let add_one_v2 = |x: u32| -> u32 { x + 1 }; +} + +trait Foo { + // This should not be suggested to be made const + // (rustc restriction) + fn f() -> u32; +} + +// Don't lint custom entrypoints either +#[start] +fn init(num: isize, something: *const *const u8) -> isize { 1 } diff --git a/tests/ui/missing_const_for_fn/cant_be_const.stderr b/tests/ui/missing_const_for_fn/cant_be_const.stderr new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/ui/missing_const_for_fn/could_be_const.rs b/tests/ui/missing_const_for_fn/could_be_const.rs new file mode 100644 index 0000000000000..3ba39711e8d6a --- /dev/null +++ b/tests/ui/missing_const_for_fn/could_be_const.rs @@ -0,0 +1,53 @@ +#![warn(clippy::missing_const_for_fn)] +#![allow(clippy::let_and_return)] + +use std::mem::transmute; + +struct Game { + guess: i32, +} + +impl Game { + // Could be const + pub fn new() -> Self { + Self { + guess: 42, + } + } +} + +// Could be const +fn one() -> i32 { 1 } + +// Could also be const +fn two() -> i32 { + let abc = 2; + abc +} + +// TODO: Why can this be const? because it's a zero sized type? +// There is the `const_string_new` feature, but it seems that this already works in const fns? +fn string() -> String { + String::new() +} + +// Could be const +unsafe fn four() -> i32 { 4 } + +// Could also be const +fn generic(t: T) -> T { + t +} + +// FIXME: This could be const but is currently not linted +fn sub(x: u32) -> usize { + unsafe { transmute(&x) } +} + +// FIXME: This could be const but is currently not linted +fn generic_arr(t: [T; 1]) -> T { + t[0] +} + +// 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 new file mode 100644 index 0000000000000..09350572e9934 --- /dev/null +++ b/tests/ui/missing_const_for_fn/could_be_const.stderr @@ -0,0 +1,42 @@ +error: this could be a const_fn + --> $DIR/could_be_const.rs:12:5 + | +LL | / pub fn new() -> Self { +LL | | Self { +LL | | guess: 42, +LL | | } +LL | | } + | |_____^ + | + = note: `-D clippy::missing-const-for-fn` implied by `-D warnings` + +error: this could be a const_fn + --> $DIR/could_be_const.rs:20:1 + | +LL | fn one() -> i32 { 1 } + | ^^^^^^^^^^^^^^^^^^^^^ + +error: this could be a const_fn + --> $DIR/could_be_const.rs:30:1 + | +LL | / fn string() -> String { +LL | | String::new() +LL | | } + | |_^ + +error: this could be a const_fn + --> $DIR/could_be_const.rs:35:1 + | +LL | unsafe fn four() -> i32 { 4 } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this could be a const_fn + --> $DIR/could_be_const.rs:38:1 + | +LL | / fn generic(t: T) -> T { +LL | | t +LL | | } + | |_^ + +error: aborting due to 5 previous errors + From 68cc4df551ea289e542910226cd1429ff2310dfa Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 10 Jan 2019 20:33:24 +0100 Subject: [PATCH 2/7] Maybe fix ICE? --- clippy_lints/src/missing_const_for_fn.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 4751a538ebd6c..caa516acd24d1 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -84,7 +84,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { ) { let def_id = cx.tcx.hir().local_def_id(node_id); let mir = cx.tcx.optimized_mir(def_id); - if let Ok(_) = is_min_const_fn(cx.tcx, def_id, &mir) { + if let Err((span, err) = is_min_const_fn(cx.tcx, def_id, &mir) { + cx.tcx.sess.span_err(span, &err); + } else { match kind { FnKind::ItemFn(name, _generics, header, _vis, attrs) => { if !can_be_const_fn(&name.as_str(), header, attrs) { From f9d65b6356abfc0503046232776cf6fdc43fb578 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 14 Jan 2019 16:44:10 +0100 Subject: [PATCH 3/7] Reorganize conditionals: Run faster checks first --- clippy_lints/src/missing_const_for_fn.rs | 40 +++++++++++-------- .../ui/missing_const_for_fn/cant_be_const.rs | 7 +++- .../could_be_const.stderr | 11 ++++- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index caa516acd24d1..3e5c81484d542 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -82,26 +82,32 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { span: Span, node_id: NodeId ) { + // Perform some preliminary checks that rule out constness on the Clippy side. This way we + // can skip the actual const check and return early. + match kind { + FnKind::ItemFn(name, _generics, header, _vis, attrs) => { + if !can_be_const_fn(&name.as_str(), header, attrs) { + return; + } + }, + FnKind::Method(ident, sig, _vis, attrs) => { + let header = sig.header; + let name = ident.name.as_str(); + if !can_be_const_fn(&name, header, attrs) { + return; + } + }, + _ => return + } + let def_id = cx.tcx.hir().local_def_id(node_id); let mir = cx.tcx.optimized_mir(def_id); - if let Err((span, err) = is_min_const_fn(cx.tcx, def_id, &mir) { - cx.tcx.sess.span_err(span, &err); - } else { - match kind { - FnKind::ItemFn(name, _generics, header, _vis, attrs) => { - if !can_be_const_fn(&name.as_str(), header, attrs) { - return; - } - }, - FnKind::Method(ident, sig, _vis, attrs) => { - let header = sig.header; - let name = ident.name.as_str(); - if !can_be_const_fn(&name, header, attrs) { - return; - } - }, - _ => return + + if let Err((span, err)) = is_min_const_fn(cx.tcx, def_id, &mir) { + if cx.tcx.is_min_const_fn(def_id) { + cx.tcx.sess.span_err(span, &err); } + } else { span_lint(cx, MISSING_CONST_FOR_FN, span, "this could be a 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 5f00035b3ad36..cfaf01cf3c119 100644 --- a/tests/ui/missing_const_for_fn/cant_be_const.rs +++ b/tests/ui/missing_const_for_fn/cant_be_const.rs @@ -41,8 +41,13 @@ fn main() { trait Foo { // This should not be suggested to be made const - // (rustc restriction) + // (rustc doesn't allow const trait methods) fn f() -> u32; + + // This should not be suggested to be made const either + fn g() -> u32 { + 33 + } } // Don't lint custom entrypoints either 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 09350572e9934..593f9cf810ac6 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.stderr +++ b/tests/ui/missing_const_for_fn/could_be_const.stderr @@ -16,6 +16,15 @@ error: this could be a const_fn LL | fn one() -> i32 { 1 } | ^^^^^^^^^^^^^^^^^^^^^ +error: this could be a const_fn + --> $DIR/could_be_const.rs:23:1 + | +LL | / fn two() -> i32 { +LL | | let abc = 2; +LL | | abc +LL | | } + | |_^ + error: this could be a const_fn --> $DIR/could_be_const.rs:30:1 | @@ -38,5 +47,5 @@ LL | | t LL | | } | |_^ -error: aborting due to 5 previous errors +error: aborting due to 6 previous errors From c0a02691d87559cc1f8ff577f6bdb140a619ee7f Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 21 Jan 2019 07:54:05 +0100 Subject: [PATCH 4/7] cargo fmt --- clippy_lints/src/missing_const_for_fn.rs | 36 ++++++++++--------- clippy_lints/src/utils/mod.rs | 8 ++--- .../ui/missing_const_for_fn/cant_be_const.rs | 18 +++++++--- .../ui/missing_const_for_fn/could_be_const.rs | 12 ++++--- .../could_be_const.stderr | 20 ++++++----- 5 files changed, 54 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 3e5c81484d542..4e85d13332c16 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -1,13 +1,13 @@ use rustc::hir; -use rustc::hir::{Body, FnDecl, Constness}; use rustc::hir::intravisit::FnKind; +use rustc::hir::{Body, Constness, FnDecl}; // use rustc::mir::*; -use syntax::ast::{NodeId, Attribute}; -use syntax_pos::Span; +use crate::utils::{is_entrypoint_fn, span_lint}; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::{declare_tool_lint, lint_array}; use rustc_mir::transform::qualify_min_const_fn::is_min_const_fn; -use crate::utils::{span_lint, is_entrypoint_fn}; +use syntax::ast::{Attribute, NodeId}; +use syntax_pos::Span; /// **What it does:** /// @@ -26,8 +26,12 @@ use crate::utils::{span_lint, is_entrypoint_fn}; /// Also, the lint only runs one pass over the code. Consider these two non-const functions: /// /// ```rust -/// fn a() -> i32 { 0 } -/// fn b() -> i32 { a() } +/// fn a() -> i32 { +/// 0 +/// } +/// fn b() -> i32 { +/// a() +/// } /// ``` /// /// When running Clippy, the lint will only suggest to make `a` const, because `b` at this time @@ -38,9 +42,7 @@ use crate::utils::{span_lint, is_entrypoint_fn}; /// /// ```rust /// fn new() -> Self { -/// Self { -/// random_number: 42 -/// } +/// Self { random_number: 42 } /// } /// ``` /// @@ -48,9 +50,7 @@ use crate::utils::{span_lint, is_entrypoint_fn}; /// /// ```rust /// const fn new() -> Self { -/// Self { -/// random_number: 42 -/// } +/// Self { random_number: 42 } /// } /// ``` declare_clippy_lint! { @@ -80,7 +80,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { _: &FnDecl, _: &Body, span: Span, - node_id: NodeId + node_id: NodeId, ) { // Perform some preliminary checks that rule out constness on the Clippy side. This way we // can skip the actual const check and return early. @@ -97,7 +97,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { return; } }, - _ => return + _ => return, } let def_id = cx.tcx.hir().local_def_id(node_id); @@ -115,9 +115,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { fn can_be_const_fn(name: &str, header: hir::FnHeader, attrs: &[Attribute]) -> bool { // Main and custom entrypoints can't be `const` - if is_entrypoint_fn(name, attrs) { return false } + if is_entrypoint_fn(name, attrs) { + return false; + } // We don't have to lint on something that's already `const` - if header.constness == Constness::Const { return false } + if header.constness == Constness::Const { + return false; + } true } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index f06a257b5ca48..32fe5e22dd545 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -354,11 +354,9 @@ pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option bool { - - let is_custom_entrypoint = attrs.iter().any(|attr| { - attr.path.segments.len() == 1 - && attr.path.segments[0].ident.to_string() == "start" - }); + let is_custom_entrypoint = attrs + .iter() + .any(|attr| attr.path.segments.len() == 1 && attr.path.segments[0].ident.to_string() == "start"); is_custom_entrypoint || fn_name == "main" } 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 cfaf01cf3c119..ede3724cc6b0d 100644 --- a/tests/ui/missing_const_for_fn/cant_be_const.rs +++ b/tests/ui/missing_const_for_fn/cant_be_const.rs @@ -8,16 +8,22 @@ struct Game; // This should not be linted because it's already const -const fn already_const() -> i32 { 32 } +const fn already_const() -> i32 { + 32 +} impl Game { // This should not be linted because it's already const - pub const fn already_const() -> i32 { 32 } + pub const fn already_const() -> i32 { + 32 + } } // Allowing on this function, because it would lint, which we don't want in this case. #[allow(clippy::missing_const_for_fn)] -fn random() -> u32 { 42 } +fn random() -> u32 { + 42 +} // We should not suggest to make this function `const` because `random()` is non-const fn random_caller() -> u32 { @@ -30,7 +36,7 @@ static Y: u32 = 0; // refer to a static variable fn get_y() -> u32 { Y - //~^ ERROR E0013 + //~^ ERROR E0013 } // Also main should not be suggested to be made const @@ -52,4 +58,6 @@ trait Foo { // Don't lint custom entrypoints either #[start] -fn init(num: isize, something: *const *const u8) -> isize { 1 } +fn init(num: isize, something: *const *const u8) -> isize { + 1 +} 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 3ba39711e8d6a..2c0a8e7a3c11f 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.rs +++ b/tests/ui/missing_const_for_fn/could_be_const.rs @@ -10,14 +10,14 @@ struct Game { impl Game { // Could be const pub fn new() -> Self { - Self { - guess: 42, - } + Self { guess: 42 } } } // Could be const -fn one() -> i32 { 1 } +fn one() -> i32 { + 1 +} // Could also be const fn two() -> i32 { @@ -32,7 +32,9 @@ fn string() -> String { } // Could be const -unsafe fn four() -> i32 { 4 } +unsafe fn four() -> i32 { + 4 +} // Could also be const fn generic(t: T) -> T { 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 593f9cf810ac6..22ea852905dac 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.stderr +++ b/tests/ui/missing_const_for_fn/could_be_const.stderr @@ -2,19 +2,19 @@ error: this could be a const_fn --> $DIR/could_be_const.rs:12:5 | LL | / pub fn new() -> Self { -LL | | Self { -LL | | guess: 42, -LL | | } +LL | | Self { guess: 42 } LL | | } | |_____^ | = note: `-D clippy::missing-const-for-fn` implied by `-D warnings` error: this could be a const_fn - --> $DIR/could_be_const.rs:20:1 + --> $DIR/could_be_const.rs:18:1 | -LL | fn one() -> i32 { 1 } - | ^^^^^^^^^^^^^^^^^^^^^ +LL | / fn one() -> i32 { +LL | | 1 +LL | | } + | |_^ error: this could be a const_fn --> $DIR/could_be_const.rs:23:1 @@ -36,11 +36,13 @@ LL | | } error: this could be a const_fn --> $DIR/could_be_const.rs:35:1 | -LL | unsafe fn four() -> i32 { 4 } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | / unsafe fn four() -> i32 { +LL | | 4 +LL | | } + | |_^ error: this could be a const_fn - --> $DIR/could_be_const.rs:38:1 + --> $DIR/could_be_const.rs:40:1 | LL | / fn generic(t: T) -> T { LL | | t From 0c6bdda562a0b703214657d92dcfa85b78537daf Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 22 Jan 2019 07:36:15 +0100 Subject: [PATCH 5/7] Use built-in entry_fn detection over self-built --- clippy_lints/src/missing_const_for_fn.rs | 33 ++++++++----------- clippy_lints/src/utils/mod.rs | 17 ++++------ .../ui/missing_const_for_fn/cant_be_const.rs | 14 +++----- 3 files changed, 25 insertions(+), 39 deletions(-) diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 4e85d13332c16..2afde0c931567 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -6,7 +6,7 @@ use crate::utils::{is_entrypoint_fn, span_lint}; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::{declare_tool_lint, lint_array}; use rustc_mir::transform::qualify_min_const_fn::is_min_const_fn; -use syntax::ast::{Attribute, NodeId}; +use syntax::ast::NodeId; use syntax_pos::Span; /// **What it does:** @@ -82,25 +82,28 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { span: Span, node_id: NodeId, ) { + let def_id = cx.tcx.hir().local_def_id(node_id); + + if is_entrypoint_fn(cx, def_id) { + return; + } + // Perform some preliminary checks that rule out constness on the Clippy side. This way we // can skip the actual const check and return early. match kind { - FnKind::ItemFn(name, _generics, header, _vis, attrs) => { - if !can_be_const_fn(&name.as_str(), header, attrs) { + FnKind::ItemFn(_, _, header, ..) => { + if already_const(header) { return; } }, - FnKind::Method(ident, sig, _vis, attrs) => { - let header = sig.header; - let name = ident.name.as_str(); - if !can_be_const_fn(&name, header, attrs) { + FnKind::Method(_, sig, ..) => { + if already_const(sig.header) { return; } }, _ => return, } - let def_id = cx.tcx.hir().local_def_id(node_id); let mir = cx.tcx.optimized_mir(def_id); if let Err((span, err)) = is_min_const_fn(cx.tcx, def_id, &mir) { @@ -113,15 +116,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn { } } -fn can_be_const_fn(name: &str, header: hir::FnHeader, attrs: &[Attribute]) -> bool { - // Main and custom entrypoints can't be `const` - if is_entrypoint_fn(name, attrs) { - return false; - } - - // We don't have to lint on something that's already `const` - if header.constness == Constness::Const { - return false; - } - true +// 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/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 32fe5e22dd545..8ce28bfed1ff2 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -3,7 +3,7 @@ use if_chain::if_chain; use matches::matches; use rustc::hir; use rustc::hir::def::Def; -use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX}; +use rustc::hir::def_id::{DefId, LOCAL_CRATE, CRATE_DEF_INDEX}; use rustc::hir::intravisit::{NestedVisitorMap, Visitor}; use rustc::hir::Node; use rustc::hir::*; @@ -350,15 +350,12 @@ pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option bool { - let is_custom_entrypoint = attrs - .iter() - .any(|attr| attr.path.segments.len() == 1 && attr.path.segments[0].ident.to_string() == "start"); - - is_custom_entrypoint || fn_name == "main" +/// Returns true if the provided `def_id` is an entrypoint to a program +pub fn is_entrypoint_fn(cx: &LateContext<'_, '_>, def_id: DefId) -> bool { + if let Some((entry_fn_def_id, _)) = cx.tcx.entry_fn(LOCAL_CRATE) { + return def_id == entry_fn_def_id + } + false } /// Get the name of the item the expression is in, if available. 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 ede3724cc6b0d..36efe16b84f0f 100644 --- a/tests/ui/missing_const_for_fn/cant_be_const.rs +++ b/tests/ui/missing_const_for_fn/cant_be_const.rs @@ -39,10 +39,10 @@ fn get_y() -> u32 { //~^ ERROR E0013 } -// Also main should not be suggested to be made const -fn main() { - // We should also be sure to not lint on closures - let add_one_v2 = |x: u32| -> u32 { x + 1 }; +// Don't lint entrypoint functions +#[start] +fn init(num: isize, something: *const *const u8) -> isize { + 1 } trait Foo { @@ -55,9 +55,3 @@ trait Foo { 33 } } - -// Don't lint custom entrypoints either -#[start] -fn init(num: isize, something: *const *const u8) -> isize { - 1 -} From aed001b8d4b318dab34882dafa27511e21bfa58a Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 22 Jan 2019 07:59:09 +0100 Subject: [PATCH 6/7] Update various docs * `const_transmute` currently also seems to depend on the `const_fn` feature. * Only `Sized` is currently allowed as a bound, not Copy. --- clippy_lints/src/missing_const_for_fn.rs | 6 +++--- tests/ui/missing_const_for_fn/could_be_const.rs | 10 ++++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 2afde0c931567..9c9029a500b63 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -11,11 +11,11 @@ use syntax_pos::Span; /// **What it does:** /// -/// Suggests the use of `const` in functions and methods where possible +/// Suggests the use of `const` in functions and methods where possible. /// /// **Why is this bad?** -/// Not using `const` is a missed optimization. Instead of having the function execute at runtime, -/// when using `const`, it's evaluated at compiletime. +/// +/// Not having the function const prevents callers of the function from being const as well. /// /// **Known problems:** /// 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 2c0a8e7a3c11f..139e64de1ff5d 100644 --- a/tests/ui/missing_const_for_fn/could_be_const.rs +++ b/tests/ui/missing_const_for_fn/could_be_const.rs @@ -25,8 +25,8 @@ fn two() -> i32 { abc } -// TODO: Why can this be const? because it's a zero sized type? -// There is the `const_string_new` feature, but it seems that this already works in const fns? +// FIXME: This is a false positive in the `is_min_const_fn` function. +// At least until the `const_string_new` feature is stabilzed. fn string() -> String { String::new() } @@ -41,12 +41,14 @@ fn generic(t: T) -> T { t } -// FIXME: This could be const but is currently not linted +// FIXME: Depends on the `const_transmute` and `const_fn` feature gates. +// In the future Clippy should be able to suggest this as const, too. fn sub(x: u32) -> usize { unsafe { transmute(&x) } } -// FIXME: This could be const but is currently not linted +// NOTE: This is currently not yet allowed to be const +// Once implemented, Clippy should be able to suggest this as const, too. fn generic_arr(t: [T; 1]) -> T { t[0] } From d0d7c5e92271c40b74e796bcf71758348a748553 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 23 Jan 2019 07:32:58 +0100 Subject: [PATCH 7/7] cargo fmt --- clippy_lints/src/missing_const_for_fn.rs | 3 +-- clippy_lints/src/utils/mod.rs | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/missing_const_for_fn.rs b/clippy_lints/src/missing_const_for_fn.rs index 9c9029a500b63..9228c586bbfd5 100644 --- a/clippy_lints/src/missing_const_for_fn.rs +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -1,8 +1,7 @@ +use crate::utils::{is_entrypoint_fn, span_lint}; use rustc::hir; use rustc::hir::intravisit::FnKind; use rustc::hir::{Body, Constness, FnDecl}; -// use rustc::mir::*; -use crate::utils::{is_entrypoint_fn, span_lint}; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::{declare_tool_lint, lint_array}; use rustc_mir::transform::qualify_min_const_fn::is_min_const_fn; diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 8ce28bfed1ff2..ee3356fdc8234 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -3,7 +3,7 @@ use if_chain::if_chain; use matches::matches; use rustc::hir; use rustc::hir::def::Def; -use rustc::hir::def_id::{DefId, LOCAL_CRATE, CRATE_DEF_INDEX}; +use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX, LOCAL_CRATE}; use rustc::hir::intravisit::{NestedVisitorMap, Visitor}; use rustc::hir::Node; use rustc::hir::*; @@ -353,7 +353,7 @@ pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option, def_id: DefId) -> bool { if let Some((entry_fn_def_id, _)) = cx.tcx.entry_fn(LOCAL_CRATE) { - return def_id == entry_fn_def_id + return def_id == entry_fn_def_id; } false }