Skip to content

Relax safety documentation of FnPtr::from_ptr(). #163821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kpreid wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Relax safety documentation of FnPtr::from_ptr(). #163821

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions library/core/src/ops/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,8 @@ pub trait FnPtr: Copy {
self.as_ptr().addr().get()
}

/// Returns the function pointer as a [`NonNull<Code>`].
/// Returns the function pointer as a [`NonNull<Code>`], thus forgetting the signature and
/// everything else about the type except except the fact that it was a function pointer.
Comment on lines +335 to +336

@asquared31415 asquared31415 Oct 5, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we perhaps also link to the docs talking about the hazards of function pointers? In particular, NonNull<Code> implements PartialEq, but it has the same problems as "normal" function pointers fn() with regards to comparisons and identity.

(also the wrong lint fires when you try to equate NonNull<Code>, the wide pointer lint fires instead of the fn pointer comparison lint, but that should be fixed in a different PR)

View changes since the review

@clarfonthey clarfonthey Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems fair. Potentially something like, in general, you should not expect to glean any useful information from this pointer besides converting it back into a function, and to link fn_addr_eq to explain pitfalls.

(whose docs I don't think have been updated yet, but will be)

@RalfJung RalfJung Oct 7, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// everything else about the type except except the fact that it was a function pointer.
/// everything else about the function `self` points to.

NonNull<Code> can be safely constructed to point to non-functions so arguably this forgets everything.

View changes since the review

#[unstable(feature = "fn_static", issue = "148768")]
#[lang = "fn_ptr_as_ptr"]
fn as_ptr(self) -> NonNull<Code>;
Expand All @@ -341,9 +342,9 @@ pub trait FnPtr: Copy {
///
/// # Safety
///
/// The function pointer must have been obtained
/// from an [`FnPtr::as_ptr`] call from a function
/// pointer type that is ABI compatible.
/// `ptr` must have been obtained from calling [`FnPtr::as_ptr`] on a function pointer type

@RalfJung RalfJung Oct 7, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// `ptr` must have been obtained from calling [`FnPtr::as_ptr`] on a function pointer type
/// `ptr` must have been obtained by calling [`FnPtr::as_ptr`] on a function pointer type

View changes since the review

/// that is ABI-compatible, or from an equivalent foreign operation (e.g. requesting a function

@RalfJung RalfJung Oct 7, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ABI-compatible with what?

View changes since the review

@RalfJung RalfJung Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is correct. Self here may be e.g. fn(i32) -> i32, right? In that case the constructor needs to require that this is a safe function with that signature. Being ABI-compatible is not enough.

@clarfonthey clarfonthey Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not entirely sure this is correct either. It feels like we're back into the fn_addr_eq discussion again, where you really can't be sure that functions are compatible because the compiler might insert invisible nonsense in the prelude.

@RalfJung RalfJung Oct 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this has anything to do with fn_addr_eq.

It's just that the safety invariant for fn(i32) -> i32 is "when this function is invoked with an i32, it will run without causing UB and if it returns, the result is an i32". The comment that's here right now is not strong enough to justify this property.

/// pointer, of the appropriate ABI, from a dynamic loader by name).
#[unstable(feature = "fn_static", issue = "148768")]
#[lang = "fn_ptr_from_ptr"]
unsafe fn from_ptr(ptr: NonNull<Code>) -> Self;
Expand Down
Loading