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

add wrapping_offset_from #244

Closed
RalfJung opened this issue Jun 26, 2023 · 4 comments
Closed

add wrapping_offset_from #244

RalfJung opened this issue Jun 26, 2023 · 4 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@RalfJung
Copy link
Member

Proposal

Problem statement

Using wrapping_offset it is possible (even at const-time) to create pointers that are still associated with the original allocation they came from, but may point out-of-bounds of that allocation. One fairly common use of such an operation is for iterators over slices of ZST, where the begin and end pointers are re-interpreted as "the difference between them (in bytes) is the remaining number of elements" but where they actually point is irrelevant (since ZST values do not carry any data, only their count is relevant). This need an operation to compute the distance between two such pointers created by wrapping_offset. Currently, we do not offer such an operation: at runtime it is possible to cast the pointers to integers and do the subtraction there, but at compile time that will not work -- and also it seems preferable to avoid ptr-to-int casts when they are not actually needed.

Motivating examples or use cases

I mentioned the ZST iterator above. Also see rust-lang/rust#92512.

Solution sketch

Add a wrapping_offset_from that allows subtracting pointers even if they are out-of-bounds or have wrapped the address space, as long as they originate from the same allocation.

See rust-lang/rust#112837 for an implementation of this.

Alternatives

If we do nothing, implementing iterators in this way in const fn will remain impossible.

We could weaken the offset_from requirements to allow out-of-bounds and wrapping difference. However that would destroy the symmetry indicating by the naming of offset and offset_from.

Links and related work

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2023

After looking at the slice iterator code again, actually the ZST code path does not do pointer subtraction any more. So I guess that concrete motivation for wrapping_offset_from does not hold up.

I still think it would make sense to have a function that mirrors wrapping_offset, but given that wrapping_offset is rarely required, it is hard to come up with concrete motivations for both wrapping_offset and wrapping_offset_from.

@thomcc
Copy link
Member

thomcc commented Jul 4, 2023

So, I've wanted a wrapping equivalent to offset_from often, but had always hoped it (like wrapping_offset) would be a safe function.

In the PR your version is unsafe because it requires that the pointers be from the same object (and the pointers be an exact multiple of T apart, which is indeed tricky), but this requirement is just for const support:

The requirement for pointers to be derived from the same allocated object is primarily needed for const-compatibility: at compile-time, pointers into different allocated object do not have a known distance to each other.

This is somewhat surprising, since I think it'd be reasonable for users to expect the operation to behave as wrapping arithmetic on the addresses.

IMO it's also not worth it -- offset_from supports const, and the users who can't or don't want to use that can either separately track the offset, or do something like what core does for slice iterators of ZST (as you mention).


That said, the non-const case seems a lot more useful... if the function were safe. While it's true that wrapping_(add|offset|sub) is rarely needed, it's actually semi-common to see these used in the wild, since they're safe (and thus easier to review, and less likely to cause major regret in the future).

I think this would be true for wrapping_offset_from, since the returned value can be used (safely) in a bounds/range check, as an index, as a count, and various other things which are not that likely to be require other unsafe code.

I've found this to be the case in my own projects, which often end up with some utility function like the following (typed from memory):

// in a utils.rs somewhere...
pub(crate) fn ptr_byte_diff<T>(a: *const T, b: *const T) -> isize {
	a.addr().wrapping_sub(b.addr()) as isize
}

Which is basically something like a (safe) x.wrapping_byte_offset_from(y)... or something along those lines.

Sadly, I have no idea what the non _byte_ version of this should do if the pointers are not a multiple of size_of::<T> apart. All the options seem bad; "panic", "round up/down", "abort", "UB"... Oof. That said, maybe we have existing design precedent for the behavior here in some other API, IDK.

I also don't know if this is sufficiently useful to justify adding (especially if it would just be the version for the byte offset). I've found it useful, but perhaps it's too niche?

(Not to mention, it can't support const in any way I can see. I don't think this is that bad (as mentioned) but it was the goal of your proposal...)

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2023

IMO it's also not worth it -- offset_from supports const, and the users who can't or don't want to use that can either separately track the offset, or do something like what core does for slice iterators of ZST (as you mention).

I'm doubtful that will always work satisfyingly. But I don't have a counterexample.

That said, the non-const case seems a lot more useful... if the function were safe.

A safe version of this can't be the exact dual to wrapping_offset though, in the sense that it is valid to call if and only if
self could have been computed as origin.wrapping_offset(n) for some n, and it will then return that n. Maybe that's not a desirable property, but it is at least a design principle.

And indeed, it cannot be const (except maybe via "just halt evaluation if the pointers are not derived from the same allocated object").

Note that a safe wrapping_offset_from existed for a short time but got removed again (rust-lang/rust#73580).

@the8472
Copy link
Member

the8472 commented Jul 11, 2023

We discussed this in the libs-meeting today. With rust-lang/rust#92512 (comment) the primary motivation seems to be gone.

And as thomcc explains other use-cases many want this to be a safe function but then we can't make it const.

So we're closing this based on the given motivation. If there's a better one, maybe with different tradeoffs you can make an updated ACP.

@the8472 the8472 closed this as completed Jul 11, 2023
@dtolnay dtolnay closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants