Auto merge of #3648 - phansch:const_fn_lint, r=<try>

Add initial version of const_fn lint

This adds an initial version of a lint that can tell if a function could be `const`.

TODO:

- [x] Finish up the docs
- [ ] Fix the ICE

cc #2440

Author: bors

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

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:
 

clippy_lints/src/lib.rs

@@ -24,6 +24,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;
@@ -478,6 +481,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,
@@ -1017,6 +1021,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,

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.
+///
+/// **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)
+    }
+}
+
+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 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
+            }
+            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
+}

clippy_lints/src/utils/mod.rs

@@ -330,6 +330,19 @@ pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option<Vec<&'a
     Some(matched)
 }
 
+/// Returns true if the function is an entrypoint to a program
+///
+/// This is either the usual `main` function or a custom function with the `#[start]` attribute.
+pub fn is_entrypoint_fn(fn_name: &str, attrs: &[ast::Attribute]) -> 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<Name> {
     let parent_id = cx.tcx.hir().get_parent(expr.id);

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 }

tests/ui/missing_const_for_fn/cant_be_const.stderr

@@ -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 {
+    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: Copy>(t: [T; 1]) -> T {
+    t[0]
+}
+
+// Should not be const
+fn main() {}

tests/ui/missing_const_for_fn/could_be_const.rs

@@ -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) -> T {
+LL | |     t
+LL | | }
+   | |_^
+
+error: aborting due to 5 previous errors
+