diff --git a/CHANGELOG.md b/CHANGELOG.md index 7acf9bc1eaa8..c1b335223241 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -756,6 +756,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 8ca10da416dd..3a7e9a165c16 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 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 292 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 35c00fb63284..d9432e84e30f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -33,6 +33,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; @@ -153,6 +155,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; @@ -487,6 +490,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box redundant_clone::RedundantClone); reg.register_late_lint_pass(box slow_vector_initialization::Pass); reg.register_late_lint_pass(box types::RefToMut); + reg.register_late_lint_pass(box missing_const_for_fn::MissingConstForFn); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -1026,6 +1030,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 000000000000..a79fbfe78925 --- /dev/null +++ b/clippy_lints/src/missing_const_for_fn.rs @@ -0,0 +1,113 @@ +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. +/// `const fn`s exist +/// TODO: Explain why const is preferable over non-const +/// +/// **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. +/// +/// 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) + } +} + +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 9b5e18413a23..5fb17b427f50 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -339,6 +339,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 000000000000..5f00035b3ad3 --- /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 000000000000..f51936e932e2 --- /dev/null +++ b/tests/ui/missing_const_for_fn/cant_be_const.stderr @@ -0,0 +1,24 @@ +error: extern items cannot be `const` + --> $DIR/cant_be_const.rs:44:5 + | +LL | const fn abc() -> i32 { 32 } + | ^^^^^ help: try using a static value: `static` + +error: expected identifier, found keyword `fn` + --> $DIR/cant_be_const.rs:44:11 + | +LL | const fn abc() -> i32 { 32 } + | ^^ expected identifier, found keyword +help: you can escape reserved keywords to use them as identifiers + | +LL | const r#fn abc() -> i32 { 32 } + | ^^^^ + +error: expected `:`, found `abc` + --> $DIR/cant_be_const.rs:44:14 + | +LL | const fn abc() -> i32 { 32 } + | ^^^ expected `:` + +error: aborting due to 3 previous errors + 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 000000000000..3ba39711e8d6 --- /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 000000000000..4e0cfe9f2471 --- /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:15: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:23:1 + | +LL | fn one() -> i32 { 1 } + | ^^^^^^^^^^^^^^^^^^^^^ + +error: this could be a const_fn + --> $DIR/could_be_const.rs:33:1 + | +LL | / fn string() -> String { +LL | | String::new() +LL | | } + | |_^ + +error: this could be a const_fn + --> $DIR/could_be_const.rs:38:1 + | +LL | unsafe fn four() -> i32 { 4 } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this could be a const_fn + --> $DIR/could_be_const.rs:41:1 + | +LL | / fn generic(t: T) -> T { +LL | | t +LL | | } + | |_^ + +error: aborting due to 5 previous errors +