Warn about missing #[inline] on trivial public methods

[emilk]

What it does

Check for trivial functions marked pub that aren't marked #[inline] (or #[inline(always)] or #[inline(never)]).

Related issue:

• Opt-out of lint #[inline] on generic functions #11187

Advantage

Adding #[inline] to trivial methods can have a huge effect on performance when the methods are called cross-crate and LTO is not enabled.

The Rust compilers sometimes auto-inlines some of these methods, but not reliably so (at least not yet).

Drawbacks

Defining what makes for a "trivial" function will not be easy. Suggestion is to only trigger the lint if the function restricts itself to:

• constructs/destruct an object
• gets/sets fields
• does at most one function call
• gets arguments and returns a value

Example

pub struct Foo {
    name: String,
    age: Option<i32>,
}

impl Foo {
    pub fn new(name: String) -> Self {
        Self { name, age: None }
    }
    
    pub fn with_age(mut self, age: i32) -> Self {
        self.age = Some(age);
        self
    }
    
    pub fn name(&self) -> &str {
        self.name.as_str()
    }
}

impl AsRef<str> for Foo {
    fn as_ref(&self) -> &str {
        self.as_str()
    }
}

Could be written as:

pub struct Foo {
    name: String,
    age: Option<i32>,
}

impl Foo {
    #[inline]
    pub fn new(name: String) -> Self {
        Self { name, age: None }
    }
    
    #[inline]
    pub fn with_age(mut self, age: i32) -> Self {
        self.age = Some(age);
        self
    }
    
    #[inline]
    pub fn name(&self) -> &str {
        self.name.as_str()
    }
}

impl AsRef<str> for Foo {
    #[inline]
    fn as_ref(&self) -> &str {
        self.as_str()
    }
}

A lot of people don't realize that #[inline] is also important when implementing simple trait functions like these, so that's why I'm calling it out explicitly.

[GuillaumeGomez]

It could be potentially expanded to handled cases like an if/else condition with one expression inside each branch. For now, I'm going to implement the lint for this simple case.

[y21]

A lot of people don't realize that #[inline] is also important when implementing simple trait functions like these, so that's why I'm calling it out explicitly.

Is this really still true today? As of recently rustc has started automatically adding #[inline] to all "trivial" functions so it also works for non-LTO builds (this also broke compiler-explorer, which now requires #[inline(never)] for functions to actually show up). IMHO any "trivial" function that rustc doesn't inline (and where it would be worth it) seems like a bug in its cost checker and it'd make more sense to me to just fix it in the MIR inliner instead of telling users to pollute their code with #[inline] annotations all over the place

[emilk]

I agree with you in priciple y21, but last I read about the auto-inlining in rustc it was extremely simple. Maybe it has improved though (and maybe I should open an issue on rustc instead).

Does anyone know the unit-tests for this is for rustc, so we can check which cases are actually handled?

[y21]

The PR that improved it is rust-lang/rust#116505. It adds the -Zcross-crate-inline-threshold flag, by default it currently has the value 100.
That value is the max. number of MIR statements to consider "trivial" and to allow when considering if a function should be marked as inlineable. 100 statements should be enough for functions that simply return a field, construct a value, return an argument or do some small transformation to it, so it should indeed at least catch 3/4 cases mentioned.

The exact criteria are:
https://github.com/rust-lang/rust/blob/4a78c00e227124ff9d5ece2d493472e7325c87d3/compiler/rustc_mir_transform/src/cross_crate_inline.rs#L82-L85

Interestingly, that checker.calls == 0 condition does look like it would prevent the "does at most one function call" case from being inlined. I'm not sure if there's some technical reason for it I'm not aware of or if it could be tuned to return true for calls == 1 && statements == 0 specifically.

But anyway, if there are other trivial functions that aren't caught by this I do think an issue on the rust issue tracker would be better. Everyone would benefit from better heuristics in that area.

A clippy lint would duplicate what's already done in rustc but just with different heuristics that may or may not be better. There are some subtleties too that would need to be taken into account, which Rust's inliner does, such as being careful if a field access invokes Deref code, or if any parameters are dropped and expensive Drop code is executed at the end of the scope, ...
If the MIR cost checker something shouldn't be inlined, I don't think that clippy should say it either.

[emilk]

Well put @y21