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

Explain one-past-the-end pointer in std library #138976

Merged
merged 1 commit into from
Mar 29, 2025

Conversation

xizheyin
Copy link
Contributor

@xizheyin xizheyin commented Mar 26, 2025

Fixes #138969

r? libs

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 26, 2025
@jieyouxu
Copy link
Member

(Not a libs reviewer, just a drive-by remark)

@lolbinarycat
Copy link
Contributor

I think this is the wrong place to add this remark.

I think the existing docs are correct, just confusingly worded, specifically this part on offset:

If the computed offset is non-zero, then self must be derived from a pointer to some allocated object, and the entire memory range between self and the result must be in bounds of that allocated object. In particular, this range must not “wrap around” the edge of the address space.

I believe that "the range between self and the result" is meant to be a half-open range, so all the bytes up to, but not including the result must be within the bounds of the allocated object.

@xizheyin
Copy link
Contributor Author

The original documentation was correct, it just wasn't clear about this boundary case, so that caused the confusion. There seems to be ambiguity in what different people understand. The additions were just to add explanations to alleviate the confusion. : )

Do you have any good suggestions for modifications? @lolbinarycat

@lolbinarycat
Copy link
Contributor

For one, I don't think this should be addressed under allocated object, since by my reading this has nothing to do with the definition of an allocated object. Additionally, calling a pointer valid has a very specific meaning, which has to do with if you can dereference it, so "valid, but you can't dereference it" doesn't really make sense.

I think something like this would be good (additions italicized):

If the computed offset is non-zero, then self must be [derived from][crate::ptr#provenance] a pointer to some [allocated object], and the entire memory range between self and the result (not including the result itself) must be in bounds of that allocated object. In particular, this range must not "wrap around" the edge of the address space.

We can then clarify the generality of the example later on:

This means implies, for instance, that offsetting to one past the end of an allocation is always safe, such as in vec.as_ptr().add(vec.capacity()) (for vec: Vec<T>) is always safe.

I'm not on t-libs, so ultimately I have no authority on this (and maybe we should wait to see what someone from t-libs thinks about this), but I did rewrite the "Pointer to reference conversion" section, so I have a bit of experience with pointer semantics.

@lolbinarycat
Copy link
Contributor

I'm pretty sure t-libs is actually the wrong team for this.

@rust-lang/opsem

@@ -388,6 +388,12 @@ impl<T: ?Sized> *const T {
/// bounds of that allocated object. In particular, this range must not "wrap around" the edge
/// of the address space.
///
/// * Note that the special case of "one-past-the-end" pointers is explicitly allowed: a pointer
Copy link
Member

Choose a reason for hiding this comment

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

We are not using "one-past-the-end" terminology in Rust (I think it's terrible terminology).

Also, as far as I know all the existing docs already handle this properly.

@RalfJung
Copy link
Member

RalfJung commented Mar 27, 2025

I believe that "the range between self and the result" is meant to be a half-open range, so all the bytes up to, but not including the result must be within the bounds of the allocated object.

Correct. Happy for suggestions for how to clarify this, but I think calling this "past the end" is needlessly confusing. The pointer is not "past" the end, it is at the end.

For one, I don't think this should be addressed under allocated object,

👍 , the confusion is entirely around the offset docs, so only those docs should be change (if at all).

@lolbinarycat
Copy link
Contributor

@xizheyin sounds like the only change that is needed is the "(not including the result itself)" line I mentioned earlier.

@xizheyin
Copy link
Contributor Author

Make sense! Thank you for your help. I will revise this PR. @lolbinarycat @RalfJung

@RalfJung
Copy link
Member

RalfJung commented Mar 27, 2025

@xizheyin sounds like the only change that is needed is the "(not including the result itself)" line I mentioned earlier.

I don't think that's better than what we have now.

I'd rather state explicitly that when we talk about a "range" (as we already do), we mean a a..b range, i.e. left-inclusive right-exclusive -- as usual in Rust.

@lolbinarycat
Copy link
Contributor

Yeah, that might make more sense, since the allocated object docs are also in terms of ranges.

@xizheyin
Copy link
Contributor Author

I'd rather state explicitly that when we talk about a "range" (as we already do), we mean a a..b range, i.e. left-inclusive right-exclusive -- as usual in Rust.

Thank you. I've revised this submission to add a sentence of explanation. That doesn't seem to be causing any confusion. Does that make sense? @lolbinarycat @RalfJung

Signed-off-by: xizheyin <xizheyin@smail.nju.edu.cn>
@RalfJung
Copy link
Member

LGTM, thanks. :)

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 28, 2025

📌 Commit 861f425 has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2025
@xizheyin
Copy link
Contributor Author

OH, I just squashed the commits, will it matter?

@RalfJung
Copy link
Member

It's fine, this just kicked the PR out of the queue so I have to put it back in. :)

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 28, 2025

📌 Commit 074edbd has been approved by RalfJung

It is now in the queue for this repository.

@xizheyin
Copy link
Contributor Author

Thanks, I learned a lot. 😊

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#138976 (Explain one-past-the-end pointer in std library)
 - rust-lang#139052 (Put pin!() tests in the right file.)
 - rust-lang#139058 (Fix formatting nit in process.rs)
 - rust-lang#139063 (Fix TAIT & ATPIT feature gating in the presence of anon consts)
 - rust-lang#139065 (Miri subtree update)
 - rust-lang#139069 (`io::Take`: avoid new `BorrowedBuf` creation in some case)
 - rust-lang#139075 (Do not treat lifetimes from parent items as influencing child items)
 - rust-lang#139079 (tracking autodiff files via triagebot.toml)

Failed merges:

 - rust-lang#139044 (bootstrap: Avoid cloning `change-id` list)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e82557e into rust-lang:master Mar 29, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 29, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 29, 2025
Rollup merge of rust-lang#138976 - xizheyin:issue-138969, r=RalfJung

Explain one-past-the-end pointer in std library

Closing rust-lang#138969

r? libs
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 2, 2025
Explain one-past-the-end pointer in std library

Closing rust-lang#138969

r? libs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear is offsetting one element past array length is allowed
7 participants