Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial version of const_fn lint #3648

Merged
merged 7 commits into from Jan 29, 2019
Merged
Diff settings

Always

Just for now

Copy path View file
@@ -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
Copy path View file
@@ -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:

Copy path View file
@@ -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,
@@ -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
}
Copy path View file
@@ -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);
@@ -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 conversation was marked as resolved by phansch

This comment has been minimized.

@oli-obk

oli-obk Jan 11, 2019

Collaborator

also add a test with a default body

This comment has been minimized.

@phansch

phansch Jan 14, 2019

Author Collaborator

Done 👍


// This should not be suggested to be made const either
fn g() -> u32 {
33
}
}
@@ -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()
This conversation was marked as resolved by phansch

This comment has been minimized.

@oli-obk

oli-obk Jan 21, 2019

Collaborator

This is a false positive. The is_min_const_fn function has a few flaws, but it's not a problem in the compiler because we also have the qualify_consts pass which runs in addition to the is_min_const_fn checks.

}

// 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) }
This conversation was marked as resolved by phansch

This comment has been minimized.

@oli-obk

oli-obk Jan 21, 2019

Collaborator

transmute needs the const_transmute feature gate

This comment has been minimized.

@phansch

phansch Jan 22, 2019

Author Collaborator

It looks like it also depends on the const_fn feature gate? I wasn't able to make the lint suggest to make this a const fn. I tried enabling both const_transmute and const_fn and it didn't detect this as a const fn. I guess is_min_const_fn doesn't know anything about the const_transmute feature?

This comment has been minimized.

@oli-obk

oli-obk Jan 22, 2019

Collaborator

Yea, and that makes sense imo. We only want to suggest to make things const fn if that would work on stable

}

// 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 {
This conversation was marked as resolved by phansch

This comment has been minimized.

@oli-obk

oli-obk Jan 21, 2019

Collaborator

We don't allow any bounds other than Sized at the moment, so once we actually allow making this function const fn, it will automatically get picked up by clippy

t[0]
}

// Should not be const
fn main() {}
@@ -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

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.