Skip to content
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.

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

NonNull methods for pointer offset, add, sub #72429

Closed
bluss opened this issue May 21, 2020 · 6 comments
Closed

NonNull methods for pointer offset, add, sub #72429

bluss opened this issue May 21, 2020 · 6 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@bluss
Copy link
Member

bluss commented May 21, 2020

It would be beneficial to low-level code to add methods like offset, add, sub non NonNull<T>, so that pointer offsetting can be done without converting back and forth to raw pointers. These methods would be unsafe just like the corresponding raw pointer methods, return NonNull<T> and have the same semantics.

These methods will work well because such pointer offsetting is only valid inside the same allocated object; when offsetting it is not allowed to "leave" the current object, it can because of that never result in a null pointer. (For example, LLVM documents that an inbounds GEP on a non-null pointer must not result in a null pointer.)

For this reason, the safety rules that need to be followed for correct use of <NonNull<T>>::offset are the same as for <*mut T>::offset, and the method can be offered on the same terms, as an unsafe method.

This is for the moment implemented in at least one crate - rawpointer and was a necessary feature for using NonNull<T> in ndarray.

Example implementation

impl<T> NonNull<T> {
    /// Use the same documentation as the corresponding raw pointer method
    pub unsafe fn offset(self, i: isize) -> Self {
        // offset must be inside the same allocated object or one past the end, and can that way
        // never result in a null pointer
        NonNull::new_unchecked(self.as_ptr().offset(i))
    }

    pub unsafe fn add(self, i: usize) -> Self {
        NonNull::new_unchecked(self.as_ptr().add(i))
    }

    pub unsafe fn sub(self, i: usize) -> Self {
        NonNull::new_unchecked(self.as_ptr().sub(i))
    }
}

The drawback of these methods is that while raw pointer offset has tricky requirements (offset inside the same allocation) due to the code generation back-end, the new nonnull offset methods will add extra requirements on top of that (Rust-level value validity); that these two restrictions go hand in hand, is just a consequence of the current back-end. Would it be possible to imagine a "nicer" Rust that didn't have these UB traps for pointer offsetting?

@bluss bluss added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels May 21, 2020
@RalfJung
Copy link
Member

RalfJung commented Jun 2, 2020

the new nonnull offset methods will add extra requirements on top of that (Rust-level value validity)

Why would they? Or you you mean non-nullness (but you said that is not a concern)?

@bluss
Copy link
Member Author

bluss commented Jun 2, 2020

I mean non-null-ness yes.

Non-null-ness is not a concern as long as we stick to the rules we have, which are partly based on requirements from the llvm backend. The whole paragraph tries to look beyond just LLVM

@Amanieu
Copy link
Member

Amanieu commented Jun 6, 2020

I'm in favor of adding these methods since it will make NonNull easier to use in unsafe code.

@ChrisDenton
Copy link
Member

I'd like these too. Is anyone willing to get the ball rolling on this?

@eduardosm
Copy link
Contributor

It looks like this has been implemented, tracking issue is #117691

@RalfJung
Copy link
Member

Okay seems like we can close this issue then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants