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

Implement ptr_as_ref_unchecked #122492

Merged
merged 1 commit into from
May 3, 2024

Conversation

GrigorenkoPV
Copy link
Contributor

Implementation of #122034.

Prefixed the feature name with ptr_ for clarity.

Linked const-unstability to #91822, so the post there should probably be updated to mentions the 3 new methods when/if this PR is merged.

@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 14, 2024
@GrigorenkoPV
Copy link
Contributor Author

Did not replace any ptr-as-ref casts with these shiny new methods just yet, because I was planning to go on a big crusade against as-casts of pointers overall, similarly to #121556 but with things like cast, cast_{mut,const}, etc. and that will probably be a separate PR overall.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

The implementation per se is fine, but the docs on pointer fn are always in need of more love than they actually get, so I must ask some questions nevertheless.

Comment on lines +384 to +385
/// In particular, while this reference exists, the memory the pointer points to must
/// not get mutated (except inside `UnsafeCell`).
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly it is still illegal to mutate &'a Inner if I use this to obtain &'a Inner directly, instead of &'a UnsafeCell<Inner>. It might be better to describe this as "except via pointers obtained by UnsafeCell::get and other functions on UnsafeCell." Again, I understand this has already been copied a zillion times and you didn't originate this.

Comment on lines +382 to +383
/// * 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.
Copy link
Member

Choose a reason for hiding this comment

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

I realize this is copied from elsewhere but it has been copied so often with subtle changes of the context around it that by being a copy of a copy of a copy it reads fairly jarringly in this new case. I would prefer it was more explicit to explain that you, the caller of this unsafe fn, define the 'a lifetime by calling this (you may pass an explicit generic via turbofish, or it will be inferred). You are making a claim that you will uphold it for that long, and "arbitrarily chosen" makes it sound more... impersonal.

All of those requirements, except mutation (which is stated in this clause) are stated previously, so perhaps it would be better to move the non-mutation clause into its own bullet. Then it can simply say "all of the previous rules".

Copy link
Member

@RalfJung RalfJung Aug 22, 2024

Choose a reason for hiding this comment

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

In practice this is almost always chosen by inference, which makes it equivalent to 'static, so "arbitrarily chosen" is fairly close to the truth. Whatever new wording we use here should preserve that sentiment.

library/core/src/ptr/const_ptr.rs Show resolved Hide resolved
Comment on lines +375 to +416
/// Returns a shared reference to the value behind the pointer.
/// If the pointer may be null or the value may be uninitialized, [`as_uninit_ref`] must be used instead.
/// If the pointer may be null, but the value is known to have been initialized, [`as_ref`] must be used instead.
///
/// For the mutable counterpart see [`as_mut_unchecked`].
///
/// [`as_ref`]: #method.as_ref
/// [`as_uninit_ref`]: #method.as_uninit_ref
/// [`as_mut_unchecked`]: #method.as_mut_unchecked
///
/// # Safety
///
/// When calling this method, you have to ensure that all of the following is true:
///
/// * The pointer must be properly aligned.
///
/// * It must be "dereferenceable" in the sense defined in [the module documentation].
///
/// * The pointer must point to an initialized instance of `T`.
///
/// * 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.
/// In particular, while this reference exists, the memory the pointer points to must
/// not get mutated (except inside `UnsafeCell`).
///
/// This applies even if the result of this method is unused!
/// (The part about being initialized is not yet fully decided, but until
/// it is, the only safe approach is to ensure that they are indeed initialized.)
///
/// [the module documentation]: crate::ptr#safety
///
/// # Examples
///
/// ```
/// #![feature(ptr_as_ref_unchecked)]
/// let ptr: *mut u8 = &mut 10u8 as *mut u8;
///
/// unsafe {
/// println!("We got back the value: {}!", ptr.as_ref_unchecked());
/// }
/// ```
// FIXME: mention it in the docs for `as_ref` and `as_uninit_ref` once stabilized.
Copy link
Member

Choose a reason for hiding this comment

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

My comments from before apply down the line here.

/// ```
// FIXME: mention it in the docs for `as_ref` and `as_uninit_ref` once stabilized.
#[unstable(feature = "ptr_as_ref_unchecked", issue = "122034")]
#[rustc_const_unstable(feature = "const_ptr_as_ref", issue = "91822")]
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we should be associating this with const-stability of this set of fn instead of just landing these as const stable when we land them? Can you make sure the two issues are crosslinked and mention that as an option?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed we shouldn't associate this with const_ptr_as_ref, so I'm adjusting that in #130164.

When a const fn is still #[unstable], it should generally use the same feature to track its regular stability and const-stability. Then when that feature moves towards stabilization we can decide whether the const-ness can be stabilized as well, or whether it should be moved into a new feature.

Comment on lines +399 to +401
/// unsafe {
/// println!("We got back the value: {}!", ptr.as_ref_unchecked());
/// }
Copy link
Member

Choose a reason for hiding this comment

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

A lot of doctests don't really test anything because they're no_run or they only have prints like this one. Perhaps this could test something simple like

    assert!(10.eq(ptr.as_ref_unchecked()));

/// let ptr: *mut u32 = s.as_mut_ptr();
/// let first_value = unsafe { ptr.as_mut_unchecked() };
/// *first_value = 4;
/// # assert_eq!(s, [4, 2, 3]);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we hiding the assert_eq! here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ah, delicious italian food

Comment on lines +747 to +776
/// Returns a unique reference to the value behind the pointer.
/// If the pointer may be null or the value may be uninitialized, [`as_uninit_mut`] must be used instead.
/// If the pointer may be null, but the value is known to have been initialized, [`as_mut`] must be used instead.
///
/// For the shared counterpart see [`as_ref_unchecked`].
///
/// [`as_mut`]: #method.as_mut
/// [`as_uninit_mut`]: #method.as_uninit_mut
/// [`as_ref_unchecked`]: #method.as_mut_unchecked
///
/// # Safety
///
/// When calling this method, you have to ensure that all of the following is true:
///
/// * The pointer must be properly aligned.
///
/// * It must be "dereferenceable" in the sense defined in [the module documentation].
///
/// * The pointer must point to an initialized instance of `T`.
///
/// * 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.
/// In particular, while this reference exists, the memory the pointer points to must
/// not get mutated (except inside `UnsafeCell`).
///
/// This applies even if the result of this method is unused!
/// (The part about being initialized is not yet fully decided, but until
/// it is, the only safe approach is to ensure that they are indeed initialized.)
///
/// [the module documentation]: crate::ptr#safety
Copy link
Member

Choose a reason for hiding this comment

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

My comments from before etc.

@workingjubilee
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024
@Dylan-DPC
Copy link
Member

@GrigorenkoPV any updates on this? thanks

@workingjubilee
Copy link
Member

I hope my feedback wasn't overwhelming. ^^; I would be okay with deferring many of these questions to later if issues were opened for each.

I also reviewed the generated HTML and dismissed one of my own questions.

@GrigorenkoPV
Copy link
Contributor Author

Oh wow, it has been 1½ months already. Sorry, got caught up with IRL shenanigans.

Anyways

Documentation

Indeed, an overhaul is needed.

What needs to be done

I would argue that the documentation pieces for

  1. <*const T>::as_ref
  2. <*const T>::as_ref_unchecked
  3. <*mut T>::as_ref
  4. <*mut T>::as_ref_unchecked
  5. <*mut T>::as_mut
  6. <*mut T>::as_mut_unchecked

should stay more or less in sync, so it doesn't really make sense to modify half of those without modifying the others. On top of that, it's probably not going to be a trivial fix, because there's quite a few problems to address:

This PR?

With that said, I feel like non-trivial changes to existing documentation is out of scope for a PR simply implementing some counterparts for existing APIs. Counterargument: this PR doubles the amount of flawed documentation, reaching the critical mass, so it should resolve this issue or at least be blocked on it. In any case, I don't feel like I would be able to tackle the documentation issues, so I am asking anyone else to approach this if they are willing to do so.

So, should I open a separate issue? Is this PR blocked until the issue is resolved, or can we move on in the meantime?

Const-stability

Are we sure we should be associating this with const-stability of this set of fn instead of just landing these as const stable when we land them?

Yes, I would say so. All of the as_ref/as_uninit_ref/as_uninit_slice_mut and friends are const-gated behind this feature, so I see no reason, to treat this one differently. Especially considering how similar it is to as_ref. (If I understand correctly, a method that is const-stable but not generally stable still cannot be used, even in const contexts, without a feature gate? If that is so, then I see no problem with reusing that const_ptr_as_ref). Any counterarguments?

Can you make sure the two issues are crosslinked and mention that as an option?

Well, #91822 already got notified because I mentioned it in code, but I will mention it once more right now. I am also going to ask to update the tracking issue OP when and if this PR gets merged using this feature gate.

Beep-boop

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2024
@workingjubilee
Copy link
Member

I am also going to ask to update the tracking issue OP when and if this PR gets merged using this feature gate.

Ah okay, cool!

So, should I open a separate issue? Is this PR blocked until the issue is resolved, or can we move on in the meantime?

Okay, if we're tabling any (heh) issues for later, then yes I would like them on the issue tracker. Either as separate issues or a single issue with todo bullets or whatever other division you think most suits.

Regarding one thing, the doctests: those will be using this API specifically, and I would say there is an argument that, as the return type is literally not the same, there is every reason to make the doctests fairly distinct. At the very least, there should be tests that assert on certain uses for as_ref but make no sense when applied to as_ref_unchecked, whether those exist or not. Also, it's far less ambiguous as to what the improvement for those would look like:

  • Does it assert?
  • Does that assert depend on the API?
  • Does the assert pass?

Would you be willing to make that happen? I don't even care about the randomly hidden array, tbh.

@GrigorenkoPV
Copy link
Contributor Author

Okay, if we're tabling any (heh) issues for later, then yes I would like them on the issue tracker. Either as separate issues or a single issue with todo bullets or whatever other division you think most suits.

Done: #124669

@workingjubilee
Copy link
Member

Okay
@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 3, 2024

📌 Commit d6a1b36 has been approved by workingjubilee

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 May 3, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request May 3, 2024
… r=workingjubilee

Implement ptr_as_ref_unchecked

Implementation of rust-lang#122034.

Prefixed the feature name with `ptr_` for clarity.

Linked const-unstability to rust-lang#91822, so the post there should probably be updated to mentions the 3 new methods when/if this PR is merged.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 3, 2024
… r=workingjubilee

Implement ptr_as_ref_unchecked

Implementation of rust-lang#122034.

Prefixed the feature name with `ptr_` for clarity.

Linked const-unstability to rust-lang#91822, so the post there should probably be updated to mentions the 3 new methods when/if this PR is merged.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#122492 (Implement ptr_as_ref_unchecked)
 - rust-lang#123815 (Fix cannot usage in time.rs)
 - rust-lang#124059 (default_alloc_error_hook: explain difference to default __rdl_oom in alloc)
 - rust-lang#124510 (Add raw identifier in a typo suggestion)
 - rust-lang#124555 (coverage: Clean up creation of MC/DC condition bitmaps)
 - rust-lang#124593 (Describe and use CStr literals in CStr and CString docs)
 - rust-lang#124630 (CI: remove `env-x86_64-apple-tests` YAML anchor)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a296693 into rust-lang:master May 3, 2024
11 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
Rollup merge of rust-lang#122492 - GrigorenkoPV:ptr_as_ref_unchecked, r=workingjubilee

Implement ptr_as_ref_unchecked

Implementation of rust-lang#122034.

Prefixed the feature name with `ptr_` for clarity.

Linked const-unstability to rust-lang#91822, so the post there should probably be updated to mentions the 3 new methods when/if this PR is merged.
@GrigorenkoPV GrigorenkoPV deleted the ptr_as_ref_unchecked branch May 3, 2024 22:50
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.

6 participants