Emit unused_attributes for #[inline] on exported functions
#138842
Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
heh, good point test. we don't want to do this for |
|
I agree that warning on 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. |
|
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 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 |
|
Is this related to #65833? EDIT: not quite, AFAICT #65294 (comment) is adjacent to this change. |
saethlin
added
S-waiting-on-author
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 `LocalCopy`an 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.
Noratrieb
force-pushed
the
inline-exported
branch
from
c1aa984 to
1aed58c
Compare
There was a problem hiding this comment.
no idea what happened here
|
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. |
|
I approve of the idea of this PR, but it's mostly touching code I am not familiar with, so for implementation |
|
I'll give it a more thorough review tomorrow then :) |
|
Alright, it wasn't quite tomorrow but I can't find anything wrong with this. @bors r=jdonszelmann,saethlin rollup |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#138176 (Prefer built-in sized impls (and only sized impls) for rigid types always) - rust-lang#138749 (Fix closure recovery for missing block when return type is specified) - rust-lang#138842 (Emit `unused_attributes` for `#[inline]` on exported functions) - rust-lang#139153 (Encode synthetic by-move coroutine body with a different `DefPathData`) - rust-lang#139157 (Remove mention of `exhaustive_patterns` from `never` docs) - rust-lang#139167 (Remove Amanieu from the libs review rotation) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#138842 - Noratrieb:inline-exported, r=me,saethlin Emit `unused_attributes` for `#[inline]` on exported functions 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 `LocalCopy`an 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.
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 exactlyLocalCopyan unmangled symbol since that would lead to duplicate symbols, and doing a mix of an unmangledGloballySharedand mangledLocalCopyinstantiation 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
inlinehintto still be applied to such a function, but that's not why people use this attribute, they use it for theLocalCopyinstantiation 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.