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

Make NonNull::as_ref (and friends) return refs with unbound lifetimes #80771

Merged
merged 1 commit into from Mar 22, 2021

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Jan 7, 2021

Rationale:

  1. The documentation for all of these functions claims that this is what the functions already do, as they all come with this comment:

    You must enforce Rust's aliasing rules, since the returned lifetime 'a is arbitrarily chosen and does not necessarily reflect the actual lifetime of the data...

    So I think it's just a bug that they weren't this way already. Note that had it not been for this part, I wouldn't be making this PR, so if we decide we won't take this change, I'll follow it up with a docs PR to fix this.

  2. This is how the equivalent raw pointer functions behave.

    They also take self and not &self/&mut self, but that can't be changed compatibly at this point. This is the next best thing.

  3. Without this fix, often code that uses these methods will find it has to expand the lifetime of the result.

    (I can't speak for others but even in unsafe-heavy code, needing to do this unexpectedly is a huge red flag -- if Rust thinks something should have a specific lifetime, I assume it's for a reason)

Can this cause existing code to be unsound?

I'm confident this can't cause new unsoundness since the reference exists for at most its lifetime, but you get a borrow checker error if you do something that would require/allow the reference to exist past its lifetime.

Additionally, the aliasing rules of a reference only applies while the reference exists.

This must be the case, as it is required by the rules used by safe code. (That said, the documentation in this file sort of contradicts it, but I think it's just ambiguity between the lifetime 'a in &'a T and lifetime of the &'a T reference itself...)

We are increasing the lifetime of these references, but they should already have hard bounds on that lifetime, or they'd have borrow checker errors.

(CC @RalfJung because I have gone and done the mistake where I say something definitive about aliasing in Rust which is honestly outside the group of things I should make definitive comments about).

Caveats

  1. This is insta-stable (except for on the unstable functions ofc). I don't think there's any other alternative.

  2. I don't believe this is a breaking change in practice. In theory someone could be assigning NonNull::as_ref to a function pointer of type fn(&NonNull<T>) -> &T. Now they'd need to use a slightly different function pointer type which is (probably) incompatible. This seems pathological, but I guess crater could be used if there are concerns.

  3. This has no tests. The old version didn't either that I saw. I could add some stuff that fails to compile without it, if that would be useful.

  4. Sometimes the NLL borrow checker gives up and decides lifetimes live till the end of the scope, as opposed to the range where they're used. If this change can cause this to happen more, then my soundness rationale is wrong, and it's likely breaking.

    In practice this seems super unlikely.

Anyway. That was a lot of typing.

Fixes #80183

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2021
@RalfJung
Copy link
Member

RalfJung commented Jan 7, 2021

I agree with the soundness analysis; I cannot imagine any way in which this makes existing code unsound.

Regarding the change itself, I seem to recall one case where I was hit by this as well. My conclusion back then was that this was meant as a kind of safety net (note how getting a mutable reference also requires &mut NonNull). I am not sure if that safety net is really helping anyone, though.

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 7, 2021
@camelid camelid added the A-raw-pointers Area: raw pointers, MaybeUninit, NonNull label Jan 22, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2021
@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 5, 2021
@JohnTitor
Copy link
Member

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Mar 18, 2021

as_ref on raw pointers already returns an unbounded lifetime, so I think this change is fine.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 18, 2021

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 18, 2021
@rfcbot
Copy link

rfcbot commented Mar 19, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@dtolnay
Copy link
Member

dtolnay commented Mar 21, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2021

📌 Commit cce258b has been approved by dtolnay

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 21, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 21, 2021
Make NonNull::as_ref (and friends) return refs with unbound lifetimes

# Rationale:

1. The documentation for all of these functions claims that this is what the functions already do, as they all come with this comment:

    > You must enforce Rust's aliasing rules, *since the returned lifetime 'a is arbitrarily chosen* and does not necessarily reflect the actual lifetime of the data...

    So I think it's just a bug that they weren't this way already. Note that had it not been for this part, I wouldn't be making this PR, so if we decide we won't take this change, I'll follow it up with a docs PR to fix this.

2. This is how the equivalent raw pointer functions behave.

    They also take `self` and not `&self`/`&mut self`, but that can't be changed compatibly at this point. This is the next best thing.

3. Without this fix, often code that uses these methods will find it has to expand the lifetime of the result.

    (I can't speak for others but even in unsafe-heavy code, needing to do this unexpectedly is a huge red flag -- if Rust thinks something should have a specific lifetime, I assume it's for a reason)

### Can this cause existing code to be unsound?

I'm confident this can't cause new unsoundness since the reference exists for at most its lifetime, but you get a borrow checker error if you do something that would require/allow the reference to exist past its lifetime.

Additionally, the aliasing rules of a reference only applies while the reference exists.

This *must* be the case, as it is required by the rules used by safe code. (That said, the documentation in this file sort of contradicts it, but I think it's just ambiguity between the lifetime `'a` in `&'a T` and lifetime of the `&'a T` reference itself...)

We are increasing the lifetime of these references, but they should already have hard bounds on that lifetime, or they'd have borrow checker errors.

(CC `@RalfJung` because I have gone and done the mistake where I say something definitive about aliasing in Rust which is honestly outside the group of things I should make definitive comments about).

# Caveats

1. This is insta-stable (except for on the unstable functions ofc). I don't think there's any other alternative.

2. I don't believe this is a breaking change in practice. In theory someone could be assigning `NonNull::as_ref` to a function pointer of type `fn(&NonNull<T>) -> &T`. Now they'd need to use a slightly different function pointer type which is (probably) incompatible. This seems pathological, but I guess crater could be used if there are concerns.

3. This has no tests. The old version didn't either that I saw. I could add some stuff that fails to compile without it, if that would be useful.

4. Sometimes the NLL borrow checker gives up and decides lifetimes live till the end of the scope, as opposed to the range where they're used. If this change can cause this to happen more, then my soundness rationale is wrong, and it's likely breaking.

    In practice this seems super unlikely.

Anyway. That was a lot of typing.

Fixes rust-lang#80183
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#80193 (stabilize `feature(osstring_ascii)`)
 - rust-lang#80771 (Make NonNull::as_ref (and friends) return refs with unbound lifetimes)
 - rust-lang#81607 (Implement TrustedLen and TrustedRandomAccess for Range<integer>, array::IntoIter, VecDequeue's iterators)
 - rust-lang#82554 (Fix invalid slice access in String::retain)
 - rust-lang#82686 (Move `std::sys::unix::platform` to `std::sys::unix::ext`)
 - rust-lang#82771 (slice: Stabilize IterMut::as_slice.)
 - rust-lang#83329 (Cleanup LLVM debuginfo module docs)
 - rust-lang#83336 (Fix ICE with `use clippy::a::b;`)
 - rust-lang#83350 (Download a more recent LLVM version if `src/version` is modified)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ad8aa18 into rust-lang:master Mar 22, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 22, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 29, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 4, 2021
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NonNull's as_ref / as_mut don't have arbitrary lifetimes as stated