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

Add missing_inline lint #2895

merged 4 commits into from Jul 5, 2018

Conversation

@gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jul 4, 2018

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 #1503.
@gnzlbg
Copy link
Contributor Author

@gnzlbg gnzlbg commented Jul 4, 2018

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

This comment has been minimized.

@gnzlbg

gnzlbg Jul 4, 2018
Author Contributor

I have no idea whether this is needed for something.

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

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

This comment has been minimized.

@flip1995

flip1995 Jul 4, 2018
Member

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

This comment has been minimized.

@gnzlbg

gnzlbg Jul 4, 2018
Author Contributor

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

This comment has been minimized.

@gnzlbg

gnzlbg Jul 4, 2018
Author Contributor

I've added some examples.

This comment has been minimized.

@flip1995

flip1995 Jul 4, 2018
Member

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

"detects missing #[inline] attribute for public callables (functions, trait methods, methods...)"
}

pub struct MissingInline {}

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

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

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

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

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

This comment has been minimized.

@gnzlbg

gnzlbg Jul 4, 2018
Author Contributor

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

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

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

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

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

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

I don't think this check is doing anything

This comment has been minimized.

@gnzlbg

gnzlbg Jul 4, 2018
Author Contributor

Without it, the check fails for:

// do not need inline because Foo is not exported
impl Foo {
    fn FooImpl() {} // ok
}
match cx.tcx.associated_item(def_id).container {
TraitContainer(cid) => {
let n = cx.tcx.hir.as_local_node_id(cid);
if n.is_some() {

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

},
}

let desc = match impl_item.node {

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

@flip1995
Copy link
Member

@flip1995 flip1995 commented Jul 4, 2018

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
Copy link
Contributor Author

@gnzlbg gnzlbg commented Jul 4, 2018

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
Copy link
Collaborator

@oli-obk oli-obk commented Jul 4, 2018

@oli-obk
oli-obk approved these changes Jul 4, 2018
@gnzlbg
Copy link
Contributor Author

@gnzlbg gnzlbg commented Jul 4, 2018

Thanks, i've now fixed that too.

Copy link
Collaborator

@oli-obk oli-obk left a comment

Sorry, overlooked one thing in the reviews

r=me with the nit fixed

pub struct MissingInline;

impl MissingInline {
fn check_missing_inline_attrs(&self, cx: &LateContext,

This comment has been minimized.

@oli-obk

oli-obk Jul 4, 2018
Collaborator

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

@oli-obk
oli-obk approved these changes Jul 5, 2018
@oli-obk oli-obk merged commit 6c70013 into rust-lang:master Jul 5, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants