[Coding Guideline]: Do Not Depend on Function Pointer Identity#256
[Coding Guideline]: Do Not Depend on Function Pointer Identity#256rcseacord wants to merge 13 commits intorustfoundation:mainfrom
Conversation
✅ Deploy Preview for scrc-coding-guidelines ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
||
| - Comparing function pointers for equality (``fn1 == fn2``) | ||
| - Assuming a unique function address | ||
| - Using function pointers as identity keys (e.g., in maps, registries, matchers) |
There was a problem hiding this comment.
you can still use function pointers as keys, they just don't have a one-to-one relationship to function items, so you can't rely on a function item giving the same function pointer or different items giving different pointers. (ignoring #[no_mangle]). so e.g. you can have a HashSet<fn()> and it works just fine as long as you don't assume inserting any particular function item does anything other than guaranteeing calling one or more of the entries is equivalent to calling that function item.
|
@rcseacord you'll want to consider this discussion before the guideline is ready for merge rust-lang/unsafe-code-guidelines#589, since it addresses the same topic |
|
FYI @rcseacord @manhatsu -- I got this building again, aside from the code examples, see: #287 Feel free to just yoink the commit from there and replace the commits on this feature branch (i.e. cherry-pick that commit I linked above onto your local updated That way we keep all the comment history from on here. (The coding examples also need to be updated to compile) |
25b657d to
4398ef0
Compare
src/coding-guidelines/types-and-traits/gui_QbvIknd9qNF6.rst.inc
Outdated
Show resolved
Hide resolved
src/coding-guidelines/types-and-traits/gui_QbvIknd9qNF6.rst.inc
Outdated
Show resolved
Hide resolved
Clarify function pointer stability issues and provide examples of potential problems and compliant solutions.
Added a non-compliant example demonstrating issues with function address stability and uninitialized memory in Rust.
Updated references and clarified text regarding function pointer stability and the implications of using `#[no_mangle]` functions. Added citations for better context and understanding.
Refactor handler registration to pass mutable vector.
Clarify guideline on function pointer comparison and provide examples of indirect comparisons.
59dd2b4 to
93907ab
Compare
|
👋 Hey @vccjgust! You've been assigned to review this coding guideline PR. Your Role as ReviewerAs outlined in our contribution guide, please:
Review Checklist
Bot CommandsIf you need to pass this review:
To assign someone else:
Other commands:
|
|
Hey @vccjgust would you like to claim this review, or let someone else grab it? |
|
@guidelines-bot /claim |
|
Hi @plaindocs, sorry didn't know it needed claiming since it was already assigned to me (or so I thought). I'm taking a look at it as we speak! 😃 |
|
No worries @vccjgust, we're just ironing out some kinks in the process and I wasn't sure you'd received the notification. |
vccjgust
left a comment
There was a problem hiding this comment.
Looks great! I have a few small comments. I'm not sure what to do about many of the non-compliant examples since they actually work fine when run as-is in Rust Playground. It would of course be best if we could come up with demonstrators that actually show case the unexpected behaviour, but I'm not sure how to do that given that it relies on cross-crate linking behaviour etc.
| :status: draft | ||
|
|
||
| In this noncompliant example, the ``write_first`` and ``write_second`` functions each | ||
| initialize one field within a ``MaybeUninit`` and write ``uninit`` to the other. If |
There was a problem hiding this comment.
MaybeUninit -> MyMaybeUninit? It is easy for the reader to confuse this with Rust's own MaybeUninit. Perhaps rename to something a bit more unambiguous?
| #[no_mangle] | ||
| fn write_first(a: &mut MyMaybeUninit) { | ||
| *a = MyMaybeUninit { init: (0, 1, 2, 3) }; | ||
| *a = MyMaybeUninit { uninit: () }; |
There was a problem hiding this comment.
I'm a bit confused what this is intended to show since the uninit field is zero sized, so writing to it won't actually change the bit pattern (i.e. this gets compiled down to nothing)
| function or alter the address at which it is emitted. | ||
|
|
||
| Consequently, the following operations are unreliable for functions which are not | ||
| ``#[no_mangle]``: |
There was a problem hiding this comment.
The non-compliant example with MyMaybeUninit below shows that even #[no_mangle] can get merged into one
| a.init.3 = 3; | ||
| } | ||
|
|
||
| #[no_mangle] |
There was a problem hiding this comment.
the 2024 edition will insist to use unsafe all over the place here. How do we show that this only compiles for <2024 edition?
| #![allow(unpredictable_function_pointer_comparisons)] | ||
|
|
||
| mod crate_a { | ||
| pub fn handler_a() {} | ||
| pub fn handler_b() {} | ||
| } | ||
|
|
||
| fn dispatch(f: fn()) { | ||
| if f == crate_a::handler_a { | ||
| println!("Handled by A"); | ||
| } else if f == crate_a::handler_b { | ||
| println!("Handled by B"); | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| dispatch(crate_a::handler_a); |
There was a problem hiding this comment.
I'm not sure how to demonstrate this issue properly here, but this example as it is currently written, behaves exactly as expected when running in Rust Playground (at least for me)
|
Hey @vccjgust, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
Coding guidelines based on #255