Add missing_inline lint #2895

gnzlbg · 2018-07-04T11:51:41Z

When turned on, the missing_inline lint warns on all exported functions, methods,
trait methods (default impls, impls), that are not #[inline].

Closes #1503.

When turned on, the lint warns on all exported functions, methods, trait methods (default impls, impls), that are not `#[inline]`. Closes rust-lang#1503.

gnzlbg · 2018-07-04T11:51:51Z

cc @oli-obk

gnzlbg · 2018-07-04T11:53:02Z

clippy_lints/src/missing_inline.rs

+        // If we're building a test harness, FIXME: is this relevant?
+        // if cx.sess().opts.test {
+        //    return;
+        // }


I have no idea whether this is needed for something.

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

flip1995 · 2018-07-04T12:03:04Z

clippy_lints/src/missing_inline.rs

+/// 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.


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

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

I've added some examples.

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

oli-obk · 2018-07-04T11:56:20Z

clippy_lints/src/missing_inline.rs

+    "detects missing #[inline] attribute for public callables (functions, trait methods, methods...)"
+}
+
+pub struct MissingInline {}


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

oli-obk · 2018-07-04T11:58:21Z

clippy_lints/src/missing_inline.rs

+        // If we're building a test harness, FIXME: is this relevant?
+        // if cx.sess().opts.test {
+        //    return;
+        // }


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

oli-obk · 2018-07-04T11:59:26Z

clippy_lints/src/missing_inline.rs

+        }
+        match it.node {
+            hir::ItemFn(..) => {
+                // ignore main()


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

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

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

oli-obk · 2018-07-04T12:01:40Z

clippy_lints/src/missing_inline.rs

+                let desc = "a function";
+                self.check_missing_inline_attrs(cx, &it.attrs, it.span, desc);
+            },
+            hir::ItemTrait(ref _is_auto, ref _unsafe, ref _generics,


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

oli-obk · 2018-07-04T12:04:20Z

clippy_lints/src/missing_inline.rs

+        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) {


I don't think this check is doing anything

Without it, the check fails for:

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

oli-obk · 2018-07-04T12:06:54Z

clippy_lints/src/missing_inline.rs

+        match cx.tcx.associated_item(def_id).container {
+            TraitContainer(cid) => {
+                let n = cx.tcx.hir.as_local_node_id(cid);
+                if n.is_some() {


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

oli-obk · 2018-07-04T12:08:34Z

clippy_lints/src/missing_inline.rs

+            },
+        }
+
+        let desc = match impl_item.node {


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

flip1995 · 2018-07-04T12:13:24Z

dogfood test errors on your implementation:

error: this if statement can be collapsed
   --> clippy_lints/src/missing_inline.rs:141:17
    |
141 | /                 if n.is_some() {
142 | |                     if !cx.access_levels.is_exported(n.unwrap()) {
143 | |                         // If a trait is being implemented for an item, and the
144 | |                         // trait is not exported, we don't need #[inline]
145 | |                         return;
146 | |                     }
147 | |                 }

See travis

Also (unrelated) we should fix the span of the collapsible-if suggestion.. 😄

gnzlbg · 2018-07-04T13:34:35Z

I've addresses all changes (hopefully) except for detecting the type of crate being compiled. I can only get a cx.lint_sess session, and I don't see a way of getting anything about the crate from there :/

oli-obk · 2018-07-04T13:42:45Z

Ah, it's under cx.tcx.sess, and it has a field for this: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/session/struct.Session.html#structfield.crate_types

gnzlbg · 2018-07-04T15:00:18Z

Thanks, i've now fixed that too.

oli-obk

Sorry, overlooked one thing in the reviews

r=me with the nit fixed

oli-obk · 2018-07-04T15:06:13Z

clippy_lints/src/missing_inline.rs

+pub struct MissingInline;
+
+impl MissingInline {
+    fn check_missing_inline_attrs(&self, cx: &LateContext,


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

add missing_inline lint

7c4ec40

When turned on, the lint warns on all exported functions, methods, trait methods (default impls, impls), that are not `#[inline]`. Closes rust-lang#1503.

gnzlbg commented Jul 4, 2018

View reviewed changes

flip1995 reviewed Jul 4, 2018

View reviewed changes

oli-obk suggested changes Jul 4, 2018

View reviewed changes

address reviews

999a00b

do not apply lint to executable crate type

14cbdf2

oli-obk approved these changes Jul 4, 2018

View reviewed changes

oli-obk suggested changes Jul 4, 2018

View reviewed changes

refactor function

3fec3b4

oli-obk approved these changes Jul 5, 2018

View reviewed changes

oli-obk merged commit 6c70013 into rust-lang:master Jul 5, 2018

Add missing_inline lint #2895

Add missing_inline lint #2895

Uh oh!

Conversation

gnzlbg commented Jul 4, 2018

Uh oh!

gnzlbg commented Jul 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Jul 4, 2018

Uh oh!

gnzlbg commented Jul 4, 2018

Uh oh!

oli-obk commented Jul 4, 2018

Uh oh!

gnzlbg commented Jul 4, 2018

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!