Emit unused_attributes for #[inline] on exported functions

[Noratrieb]

I saw someone post a code sample that contained these two attributes, which immediately made me suspicious.
My suspicions were confirmed when I did a small test and checked the compiler source code to confirm that in these cases, #[inline] is indeed ignored (because you can't exactly LocalCopyan unmangled symbol since that would lead to duplicate symbols, and doing a mix of an unmangled GloballyShared and mangled LocalCopy instantiation is too complicated for our current instatiation mode logic, which I don't want to change right now).

So instead, emit the usual unused attribute lint with a message saying that the attribute is ignored in this position.

I think this is not 100% true, since I expect LLVM inlinehint to still be applied to such a function, but that's not why people use this attribute, they use it for the LocalCopy instantiation mode, where it doesn't work.

r? saethlin as the instantiation guy

Procedurally, I think this should be fine to merge without any lang involvement, as this only does a very minor extension to an existing lint.

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

[Noratrieb]

heh, good point test. we don't want to do this for #[inline(never)]. I'll fix it later (an initial review of the idea can be made before that)

[saethlin]

I agree that warning on #[inline] and #[inline(always)] makes sense.

I'm somewhat idly curious to see how much this warning fires in crater. If the motivating example (and/or all we see in the ecosystem) was a mistake by a beginner or just someone not paying attention when writing a contrived example that seems fine, but if this was code that's been checked in for a while that's more concerning.

[jdonszelmann]

As little miss attributes I thought I'd also have a look :3. This looks very reasonable to me :). I happen to have been looking at contains_extern_indicator, and I believe figured out the set is not always entirely complete but I'm working on that separately. It looks like this will catch all cases you're looking to catch. Neat!

After fixing never, r=me as well. I do agree with @saethlin that crater could be interesting but it doesn't look like it'd block merging this since its just a warning.

note: that last statement I'm not 100% sure about, I'm not sure what the precedent is for running crater on lint changes like this

[jieyouxu]

Is this related to #65833?

EDIT: not quite, AFAICT #65294 (comment) is adjacent to this change.

[Noratrieb]

no idea what happened here

[Noratrieb]

i don't think a crater run here is really worth it. i don't think our action would change based on the results, even if it could be interesting to look at.

[saethlin]

I approve of the idea of this PR, but it's mostly touching code I am not familiar with, so for implementation
r? jdonszelmann

[jdonszelmann]

I'll give it a more thorough review tomorrow then :)

[jdonszelmann]

Alright, it wasn't quite tomorrow but I can't find anything wrong with this. @bors r=jdonszelmann,saethlin rollup

[bors]

📌 Commit 1aed58c has been approved by me,saethlin

It is now in the queue for this repository.

[LYF1999]

what should we do if we want add inlinehint in llvm ir and effects the optimization during LTO

[Zalathar]

Bors thinks this is still in the queue:

@bors r-