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 missing_inline lint #2895

Merged
merged 4 commits into from Jul 5, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -744,6 +744,7 @@ All notable changes to this project will be documented in this file.
[`misaligned_transmute`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#misaligned_transmute
[`misrefactored_assign_op`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#misrefactored_assign_op
[`missing_docs_in_private_items`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#missing_docs_in_private_items
[`missing_inline_in_public_items`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#missing_inline_in_public_items
[`mixed_case_hex_literals`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mixed_case_hex_literals
[`module_inception`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#module_inception
[`modulo_one`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#modulo_one
Expand Down
2 changes: 1 addition & 1 deletion README.md
Expand Up @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 272 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
[There are 273 lints included in this crate!](https://rust-lang-nursery.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:

Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Expand Up @@ -134,6 +134,7 @@ pub mod minmax;
pub mod misc;
pub mod misc_early;
pub mod missing_doc;
pub mod missing_inline;
pub mod multiple_crate_versions;
pub mod mut_mut;
pub mod mut_reference;
Expand Down Expand Up @@ -364,6 +365,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_late_lint_pass(box let_if_seq::LetIfSeq);
reg.register_late_lint_pass(box eval_order_dependence::EvalOrderDependence);
reg.register_late_lint_pass(box missing_doc::MissingDoc::new());
reg.register_late_lint_pass(box missing_inline::MissingInline::new());
reg.register_late_lint_pass(box ok_if_let::Pass);
reg.register_late_lint_pass(box if_let_redundant_pattern_matching::Pass);
reg.register_late_lint_pass(box partialeq_ne_impl::Pass);
Expand Down Expand Up @@ -422,6 +424,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
methods::WRONG_PUB_SELF_CONVENTION,
misc::FLOAT_CMP_CONST,
missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS,
missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS,
panic_unimplemented::UNIMPLEMENTED,
shadow::SHADOW_REUSE,
shadow::SHADOW_SAME,
Expand Down
171 changes: 171 additions & 0 deletions clippy_lints/src/missing_inline.rs
@@ -0,0 +1,171 @@
// Copyright 2012-2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
//

use rustc::hir;
use rustc::lint::*;
use syntax::ast;
use syntax::codemap::Span;

/// **What it does:** it lints if an exported function, method, trait method with default impl,
/// or trait method impl is not `#[inline]`.
///
/// **Why is this bad?** In general, it is not. Functions can be inlined across
/// crates when that's profitable as long as any form of LTO is used. When LTO is disabled,
/// functions that are not `#[inline]` cannot be inlined across crates. Certain types of crates
/// might intend for most of the methods in their public API to be able to be inlined across
/// crates even when LTO is disabled. For these types of crates, enabling this lint might make sense.
/// It allows the crate to require all exported methods to be `#[inline]` by default, and then opt
/// out for specific methods where this might not make sense.
///
/// **Known problems:** None.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even when the example is trivial there should be an example in the documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example of a lint that does this? I based this lint on missing_docs, which doesn't provide anything :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some examples.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more than enough examples! Thanks!

You're right the missing_docs lint is missing an example. Every other lint does have one AFAIK. Even when it is obvious what the lint does: https://rust-lang-nursery.github.io/rust-clippy/current/index.html#assign_ops

declare_clippy_lint! {
pub MISSING_INLINE_IN_PUBLIC_ITEMS,
restriction,
"detects missing #[inline] attribute for public callables (functions, trait methods, methods...)"
}

pub struct MissingInline {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just make it a pub struct MissingInline;, saving you the Default impl and new method


impl ::std::default::Default for MissingInline {
fn default() -> Self {
Self::new()
}
}

impl MissingInline {
pub fn new() -> Self {
Self {}
}

fn check_missing_inline_attrs(&self, cx: &LateContext,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be a function instead of a method. It does not use &self at all.

attrs: &[ast::Attribute], sp: Span, desc: &'static str) {
// If we're building a test harness, FIXME: is this relevant?
// if cx.sess().opts.test {
// return;
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea whether this is needed for something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not. Just don't turn it on in tests, and most test stuff isn't pub


let has_inline = attrs
.iter()
.any(|a| a.name() == "inline" );
if !has_inline {
cx.span_lint(
MISSING_INLINE_IN_PUBLIC_ITEMS,
sp,
&format!("missing `#[inline]` for {}", desc),
);
}
}
}

impl LintPass for MissingInline {
fn get_lints(&self) -> LintArray {
lint_array![MISSING_INLINE_IN_PUBLIC_ITEMS]
}
}

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingInline {
fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, it: &'tcx hir::Item) {
if !cx.access_levels.is_exported(it.id) {
return;
}
match it.node {
hir::ItemFn(..) => {
// ignore main()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just ignore this lint on binary crates? It's only relevant for libs, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, do you have an example of a lint that does that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope ^^ You can probably ask the session what kind of crate it is.

if it.name == "main" {
let def_id = cx.tcx.hir.local_def_id(it.id);
let def_key = cx.tcx.hir.def_key(def_id);
if def_key.parent == Some(hir::def_id::CRATE_DEF_INDEX) {
return;
}
}
let desc = "a function";
self.check_missing_inline_attrs(cx, &it.attrs, it.span, desc);
},
hir::ItemTrait(ref _is_auto, ref _unsafe, ref _generics,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here that we're not using https://doc.rust-lang.org/nightly/nightly-rustc/rustc/lint/trait.LateLintPass.html#method.check_trait_item because we need to check if the trait is exported

ref _bounds, ref trait_items) => {
for tit in trait_items {
let tit_ = cx.tcx.hir.trait_item(tit.id);
match tit_.node {
hir::TraitItemKind::Const(..) |
hir::TraitItemKind::Type(..) => {},
hir::TraitItemKind::Method(..) => {
if tit.defaultness.has_value() {
// trait method with default body needs inline in case
// an impl is not provided
let desc = "a default trait method";
let item = cx.tcx.hir.expect_trait_item(tit.id.node_id);
self.check_missing_inline_attrs(cx, &item.attrs,
item.span, desc);
}
},
}
}
}
hir::ItemConst(..) |
hir::ItemEnum(..) |
hir::ItemMod(..) |
hir::ItemStatic(..) |
hir::ItemStruct(..) |
hir::ItemTraitAlias(..) |
hir::ItemGlobalAsm(..) |
hir::ItemTy(..) |
hir::ItemUnion(..) |
hir::ItemExistential(..) |
hir::ItemExternCrate(..) |
hir::ItemForeignMod(..) |
hir::ItemImpl(..) |
hir::ItemUse(..) => {},
};
}

fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, impl_item: &'tcx hir::ImplItem) {
use rustc::ty::{TraitContainer, ImplContainer};

// If the item being implemented is not exported, then we don't need #[inline]
if !cx.access_levels.is_exported(impl_item.id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check is doing anything

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it, the check fails for:

// do not need inline because Foo is not exported
impl Foo {
    fn FooImpl() {} // ok
}

return;
}

let def_id = cx.tcx.hir.local_def_id(impl_item.id);
match cx.tcx.associated_item(def_id).container {
TraitContainer(cid) => {
let n = cx.tcx.hir.as_local_node_id(cid);
if n.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with if let Some(n) = n to get rid of the unwrap inside

if !cx.access_levels.is_exported(n.unwrap()) {
// If a trait is being implemented for an item, and the
// trait is not exported, we don't need #[inline]
return;
}
}
},
ImplContainer(cid) => {
if cx.tcx.impl_trait_ref(cid).is_some() {
let trait_ref = cx.tcx.impl_trait_ref(cid).unwrap();
let n = cx.tcx.hir.as_local_node_id(trait_ref.def_id);
if n.is_some() {
if !cx.access_levels.is_exported(n.unwrap()) {
// If a trait is being implemented for an item, and the
// trait is not exported, we don't need #[inline]
return;
}
}
}
},
}

let desc = match impl_item.node {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this up in the function as an early abort (saves unnecessary CPU cycles)

hir::ImplItemKind::Method(..) => "a method",
hir::ImplItemKind::Const(..) |
hir::ImplItemKind::Type(_) => return,
};
self.check_missing_inline_attrs(cx, &impl_item.attrs, impl_item.span, desc);
}
}
72 changes: 72 additions & 0 deletions tests/ui/missing_inline.rs
@@ -0,0 +1,72 @@
/* This file incorporates work covered by the following copyright and
* permission notice:
* Copyright 2013 The Rust Project Developers. See the COPYRIGHT
* file at the top-level directory of this distribution and at
* http://rust-lang.org/COPYRIGHT.
*
* Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
* http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
* <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
* option. This file may not be copied, modified, or distributed
* except according to those terms.
*/
#![warn(missing_inline_in_public_items)]

// When denying at the crate level, be sure to not get random warnings from the
// injected intrinsics by the compiler.
#![allow(dead_code, non_snake_case)]

type Typedef = String;
pub type PubTypedef = String;

struct Foo {} // ok
pub struct PubFoo { } // ok
enum FooE {} // ok
pub enum PubFooE {} // ok

mod module {} // ok
pub mod pub_module {} // ok

fn foo() {}
pub fn pub_foo() {} // missing #[inline]
#[inline] pub fn pub_foo_inline() {} // ok
#[inline(always)] pub fn pub_foo_inline_always() {} // ok

#[allow(missing_inline_in_public_items)]
pub fn pub_foo_no_inline() {}
fn main() {}

trait Bar {
fn Bar_a(); // ok
fn Bar_b() {} // ok
}

pub trait PubBar {
fn PubBar_a(); // ok
fn PubBar_b() {} // missing #[inline]
#[inline] fn PubBar_c() {} // ok
}

// none of these need inline because Foo is not exported
impl PubBar for Foo {
fn PubBar_a() {} // ok
fn PubBar_b() {} // ok
fn PubBar_c() {} // ok
}

// all of these need inline because PubFoo is exported
impl PubBar for PubFoo {
fn PubBar_a() {} // missing #[inline]
fn PubBar_b() {} // missing #[inline]
fn PubBar_c() {} // missing #[inline]
}

// do not need inline because Foo is not exported
impl Foo {
fn FooImpl() {} // ok
}

// need inline because PubFoo is exported
impl PubFoo {
pub fn PubFooImpl() {} // missing #[inline]
}
40 changes: 40 additions & 0 deletions tests/ui/missing_inline.stderr
@@ -0,0 +1,40 @@
error: missing `#[inline]` for a function
--> $DIR/missing_inline.rs:31:1
|
31 | pub fn pub_foo() {} // missing #[inline]
| ^^^^^^^^^^^^^^^^^^^
|
= note: `-D missing-inline-in-public-items` implied by `-D warnings`

error: missing `#[inline]` for a default trait method
--> $DIR/missing_inline.rs:46:5
|
46 | fn PubBar_b() {} // missing #[inline]
| ^^^^^^^^^^^^^^^^

error: missing `#[inline]` for a method
--> $DIR/missing_inline.rs:59:5
|
59 | fn PubBar_a() {} // missing #[inline]
| ^^^^^^^^^^^^^^^^

error: missing `#[inline]` for a method
--> $DIR/missing_inline.rs:60:5
|
60 | fn PubBar_b() {} // missing #[inline]
| ^^^^^^^^^^^^^^^^

error: missing `#[inline]` for a method
--> $DIR/missing_inline.rs:61:5
|
61 | fn PubBar_c() {} // missing #[inline]
| ^^^^^^^^^^^^^^^^

error: missing `#[inline]` for a method
--> $DIR/missing_inline.rs:71:5
|
71 | pub fn PubFooImpl() {} // missing #[inline]
| ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 6 previous errors