Missing documentation for `inline` attribute

[k-sareen]

What it does

We work in a performance sensitive domain and so occasionally require the use of #[inline(always)] and #[inline(never)]. However, we don't want to second-guess the compiler except when absolutely necessary (specially with PGO builds). Ideally we would like to have a lint that forces a doc comment (similar to missing_safety_doc for unsafe functions) explaining why the particular inline attribute is required. Something like an /// Inline: <explanation> would work (or alternatively an /// # Inline header similar to /// # Safety).

I understand this is a non-standard use-case, but we were not able to find a way to add a custom lint to clippy easily. If this is not an acceptable lint, would it be possible to point us to a way we can add a custom lint ourselves?

Lint Name

missing_inline_doc

Category

suspicious, style, perf, pedantic

Advantage

• Better documentation of why the inline attribute is required for a particular function

Drawbacks

Use of non-standard /// Inline: doc comment

Example

#[inline(always)]
fn very_large_hot_function() {
    [ ... ]
}

Could be written as:

/// Inline: This function is very large, so the PGO skips inlining, but we have seen performance improvements if we always inline this function.
/// See performance results at: <link-to-performance-results>
#[inline(always)]
fn very_large_hot_function() {
    [ ... ]
}

[xFrednet]

Hey @k-sareen, you mention that this is not a typical use case, do you know how other projects deal with this kind of problem? So far, I've only seen inline in two other projects, one of them had no reason documented while the other one included an explanation in the doc comment.

we were not able to find a way to add a custom lint to clippy easily. If this is not an acceptable lint, would it be possible to point us to a way we can add a custom lint ourselves?

This is sadly a pain point of Clippy. You can have a look at dylint which provides rustc bindings. However, note that the rustc interface is unstable, and might require maintenance. I'm currently working on a stable linting interface for Rust, sadly it's still in early development and not mature enough to recommend it yet. If you only want to check for the doc comment, it could also be worth to just use a regex.

[k-sareen]

I think projects/developers usually don't document their reasoning for inlining. For example tokio does not document why inline(always) is required for this function or any inline function in that file for that matter. hashbrown doesn't document why they think inline(always) is required for this function, but do for this function. In general, I don't think there is a consensus around how to go about documenting inline attributes.

[xFrednet]

Thank you for the research. Since there appears to be no consensus among projects, I first want to ask the team on their thoughts. I like the idea of the lint and it should be easy to implement. However, I haven't worked in the domain and would therefore like more feedback before labeling it as a good first issue. :)

[k-sareen]

Thank you for the consideration. I will say that it is generally considered bad practice to inline functions unless absolutely necessary as they increase register pressure and the code size. This can lead to unintuitive results such as a performance decrease if the code layout slightly moves around. Unfortunately, I don't think any language has a standardized way of documenting inline attributes and decisions, often leading to developers not documenting their decisions as it is not enforced by style-checks.

EDIT: I should note that I'm referring generally to all variations of the inline attribute (so #[inline], #[inline(always)], and #[inline(never)]) in this issue, not just the inline(always) or inline(never) attribute.

[xFrednet]

This issue was discussed in todays meeting. Generally, we're fine with adding this as a restriction lint. It was noted that the effects on #[inline] will often be minimal. This should probably be mentioned in the lint description.

For the comment position, it's probably the easiest to check for it in doc comments. That part is a good first issue.

@rustbot label +good-first-issue

A good enhancement would be to support non doc comments, as the inline reason is more an implementation detail than part of the method docs. clippy::unnecessary_safety_comment is probably a good reference for implementing that part.

[k-sareen]

It was noted that the effects on #[inline] will often be minimal.

I'm not sure what you mean by "effects [...] will often be minimal"? Do you mean inline attributes don't generally change performance that much?

A good enhancement would be to support non doc comments, as the inline reason is more an implementation detail than part of the method docs.

Yes I agree. That is a good point. A normal comment would indeed be semantically better.

[xFrednet]

#[inline] is not a guarantee that the item will be inlined and if a function is inlined, it might not be that much faster. In your domain and for your use case it could be very important, but in most cases the effect is probably minimal.

A normal comment would indeed be semantically better.

Yeah, agreed, it's just harder to implement. I've removed the good-first-issue label in favor of having checking for normal comments :)

[k-sareen]

[inline] is not a guarantee that the item will be inlined and if a function is inlined, it might not be that much faster. In your domain and for your use case it could be very important, but in most cases the effect is probably minimal.

Yeah I agree in general. Specially if the inline attribute is thrown around the code base a lot.

Yeah, agreed, it's just harder to implement. I've removed the good-first-issue label in favor of having checking for normal comments :)

Makes sense. I could try my hand at it, but I've never worked on clippy before so I would appreciate if you could point me at the right direction and provide some guidance. Was there a particular comment style you had in mind (perhaps copying the // SAFETY: block comment and making it an // INLINE: comment)? I don't think this was exactly discussed in the meeting linked above.

[xFrednet]

Was there a particular comment style you had in mind

When I first read the issue, I thought about a doc comment, which would be simpler to implement. But I agree that a normal comment (// XYZ) would be better. I think // INLINE would be good as it's inline with // SAFETY.

I've never worked on clippy before so I would appreciate if you could point me at the right direction and provide some guidance.

The first step would be to detect all functions and methods with the #[inline] attribute. That should be relatively simple, with the check_fn function.

The second part is a bit harder. Comments are not directly included in rustc's AST. Therefore, we have to search for them manually. For this, I would take a look at the related implementation of undocumented_unsafe_blocks. Maybe we can find a way to make the linked method flexible in a way, that it takes the command which is search for? I'm sadly not an expert on this part of Clippy. @Jarcho has experience with this AFAIK. If you're stuck, I can try to take a detailed look at it, or ask for help :)