Auto merge of rust-lang#3648 - phansch:const_fn_lint, r=oli-obk

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
- [x] Fix the ICE

cc rust-lang#2440

Author: bors

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

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:
 

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,

clippy_lints/src/missing_const_for_fn.rs

@@ -0,0 +1,121 @@
+use crate::utils::{is_entrypoint_fn, span_lint};
+use rustc::hir;
+use rustc::hir::intravisit::FnKind;
+use rustc::hir::{Body, Constness, FnDecl};
+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::NodeId;
+use syntax_pos::Span;
+
+/// **What it does:**
+///
+/// Suggests the use of `const` in functions and methods where possible.
+///
+/// **Why is this bad?**
+///
+/// Not having the function const prevents callers of the function from being const as well.
+///
+/// **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);
+
+        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(_, _, header, ..) => {
+                if already_const(header) {
+                    return;
+                }
+            },
+            FnKind::Method(_, sig, ..) => {
+                if already_const(sig.header) {
+                    return;
+                }
+            },
+            _ => return,
+        }
+
+        let mir = cx.tcx.optimized_mir(def_id);
+
+        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");
+        }
+    }
+}
+
+// We don't have to lint on something that's already `const`
+fn already_const(header: hir::FnHeader) -> bool {
+    header.constness == Constness::Const
+}

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, CRATE_DEF_INDEX, LOCAL_CRATE};
 use rustc::hir::intravisit::{NestedVisitorMap, Visitor};
 use rustc::hir::Node;
 use rustc::hir::*;
@@ -350,6 +350,14 @@ pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option<Vec<&'a
     Some(matched)
 }
 
+/// 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.
 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,57 @@
+//! 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
+}
+
+// Don't lint entrypoint functions
+#[start]
+fn init(num: isize, something: *const *const u8) -> isize {
+    1
+}
+
+trait Foo {
+    // This should not be suggested to be made const
+    // (rustc doesn't allow const trait methods)
+    fn f() -> u32;
+
+    // This should not be suggested to be made const either
+    fn g() -> u32 {
+        33
+    }
+}

tests/ui/missing_const_for_fn/cant_be_const.stderr

@@ -0,0 +1,57 @@
+#![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
+}
+
+// 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()
+}
+
+// Could be const
+unsafe fn four() -> i32 {
+    4
+}
+
+// Could also be const
+fn generic<T>(t: T) -> T {
+    t
+}
+
+// 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) }
+}
+
+// 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: 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,53 @@
+error: this could be a const_fn
+  --> $DIR/could_be_const.rs:12:5
+   |
+LL | /     pub fn new() -> Self {
+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:18:1
+   |
+LL | / fn one() -> i32 {
+LL | |     1
+LL | | }
+   | |_^
+
+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
+   |
+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 {
+LL | |     4
+LL | | }
+   | |_^
+
+error: this could be a const_fn
+  --> $DIR/could_be_const.rs:40:1
+   |
+LL | / fn generic<T>(t: T) -> T {
+LL | |     t
+LL | | }
+   | |_^
+
+error: aborting due to 6 previous errors
+