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

Deref docs: expand and remove "smart pointer" qualifier #110340

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

jmaargh
Copy link
Contributor

@jmaargh jmaargh commented Apr 14, 2023

Ready for review

This is an unpolished draft to be sanity-checked

Fixes #91004

Comments on substance and content of this are welcome. This is deliberately unpolished until ready to review so please try to stay focused on the big-picture.

Once this has been sanity checked, I will similarly update DerefMut and polish for review.

@rustbot
Copy link
Collaborator

rustbot commented Apr 14, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@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 Apr 14, 2023
@stepancheg
Copy link
Contributor

PathBuf derefs to Path, but it does not fit well into that definition: it has quite rich set of methods. And it does not seem to be a smart pointer.

Similarly, consider scenario: AbsPath wrapper for Path which only type-asserts that path is absolute. It is unclear from this description whether it is OK for AbsPath to deref to Path.

Style suggestion: make a subsection in the rustdoc something like "When to implement Deref and when not" to make clearer distinction between precise behavior or Deref from language point of view, and a guide when to implement.

@jmaargh
Copy link
Contributor Author

jmaargh commented Apr 15, 2023

Good suggestion on the sectioning. I think it's still a good idea to keep a "you really should think carefully about this"-style warning near the top, but the details can certainly be secontioned off.

Thinking again, I guess there are two broad "typical" cases. I covered the "generic" case where you're dereffing to some unknown type. For String and PathBuf style cases, you're dereffing to a known type, so they point about a rich API isn't so relevant. I'll have a crack at incorporating this other type tomorrow.

@Noratrieb
Copy link
Member

Maybe the method restriction is better expressed as "doesn't have methods that could potentially conflict with the target type". Box is generic over any type, so any method could be conflicting. Vec and slice are carefully set up in a way to make sure that nothing could be conflicting (they have some overlap, but the overlapping methods do the same thing).

@JanBeh
Copy link
Contributor

JanBeh commented Apr 15, 2023

Regarding "Deref should not fail", I would like to refer to this post/thread on URLO where I stated the hypothesis that it's more about stability rather than failure (and bringing up the case of once_cell::sync::Lazy or the unstable std::sync::LazyLock, see also #74465).

@Mark-Simulacrum Mark-Simulacrum 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 Apr 19, 2023
@jmaargh
Copy link
Contributor Author

jmaargh commented Apr 19, 2023

I'm happy I left this alone for a bit to gather comments. I'll synthesise these over the next day or two and produce a revised draft which should be ready for non-draft-PR status.

In particular, I'll try and find a good balance that is comprehensive enough for official docs, but does not stray into "this should be a book chapter" territory. I think this will necessarily mean concentrating on good, simple advice that works the large majority of the time while sidelining the many many exceptions where it can be sensible to break the letter of that advice.

@Dylan-DPC
Copy link
Member

@jmaargh any updates on this? thanks

@Noratrieb
Copy link
Member

note that this repo does not allow these merge commits, you need to rebase: https://rustc-dev-guide.rust-lang.org/git.html#i-made-a-merge-commit-by-accident

@jmaargh jmaargh marked this pull request as ready for review June 16, 2023 13:28
@jmaargh
Copy link
Contributor Author

jmaargh commented Jun 16, 2023

Apologies for the delay.

I've redrafted with the comments in mind. The idea is to clearly state the potential problems with implementing these traits, and then give general advice which should apply for most users. I've tried to strike the right balance between giving clear advice and accuracy. However, I've purposefully avoided trying to be encyclopaedic since that could easily balloon into a short book chapter.

Particular changes:

  • followed the long-stated "definition" of "smart pointers" from the book (I'm not seeking to adjudicate on whether this is the "right" definition, but it's a term in use so mentioning it here seems appropriate)
  • broke out "fallibility" sections, since that advice stood out to me as perhaps the most pertinent
  • kept the longer advice section to Deref and linked to it from DerefMut (easily duplicated if this isn't desirable style)

As always, happy for comments.

@rust-log-analyzer

This comment has been minimized.

/// type, causing confusion for users. `Box<T>` has few methods (though
/// several associated functions), partly for this reason.
///
/// Specific implementations, such as for [`String`][string] (which only
Copy link
Contributor

@kpreid kpreid Jun 16, 2023

Choose a reason for hiding this comment

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

I think that as currently written, this pair of paragraphs can be interpreted as too strong a claim. Note that Vec<T> is a generic type, but its Deref implementation is, in the terms of this section, a “specific” one because it dereferences to [T] rather than T. The thing to watch out for is not impl<T> Deref for Foo<T>, but type Target = T.

Attempted expansion to clarify this:

Generic implementations, such as for [`Box<T>`][box] (which is generic over
every type and dereferences to `T`) should be careful to provide few or no
methods, since the target type is unknown and therefore every method could
collide with one on the target type, causing confusion for users.
`impl<T> Box<T>` has no methods (though several associated functions),
partly for this reason.

Specific implementations, such as for [`String`][string] (whose `Deref`
implementation has `Target = str`) can have many methods, since avoiding
collision is much easier. `String` and `str` both have many methods, and
`String` additionally behaves as if it has every method of `str` because of
deref coercion. The implementing type may also be generic while the
implementation is still specific in this sense; for example, [`Vec<T>`][vec]
dereferences to `[T]`, so methods of `T` are not applicable.

I also revised the statement that Box<T> has few methods; in fact it has no methods. The only methods on the Box page are for e.g. Box<MaybeUninit<T>>, not Box<T>.

@bors
Copy link
Contributor

bors commented Sep 16, 2023

☔ The latest upstream changes (presumably #115889) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@jmaargh any updates on this?

@jmaargh
Copy link
Contributor Author

jmaargh commented Oct 21, 2023

@Dylan-DPC I guess I was waiting on reviews. I'll incorporate @kpreid 's suggestion verbatim now, because I think it works and is not too verbose. Otherwise, IMHO this is ready to merge with enough reviews

Comment on lines +92 to +94
/// However, infallibility is not enforced and therefore not guaranteed.
/// As such, `unsafe` code should not rely on infallibility in general for
/// soundness.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a merge conflict from the change introduced in PR #115607, which added the following (after the previous comment about how this "...must not fail..."):

Violating these requirements is a logic error. The behavior resulting from a logic error is not
specified, but users of the trait must ensure that such logic errors do not result in
undefined behavior. This means that unsafe code must not rely on the correctness of this
method.

Frankly, this took me a while to understand at all (what are "these requirements", exactly? by "users" do we mean implementors of this trait, or some other users? this trait is safe so we can't cause undefined behaviour without unsafe code so... what are we talking about?) I guess this isn't helped by the fact that paragraph was pasted near-verbatim across several structs, rather than written for each context.

Reviewing the original issue that PR was solving ( #73682 ), I decided to replace it with the above, which I think covers the required ground while being more understandable.

@rust-log-analyzer

This comment has been minimized.

@jmaargh
Copy link
Contributor Author

jmaargh commented Oct 21, 2023

@rustbot review

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 21, 2023
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2023
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Nov 4, 2023

This looks good to me. I think we should squash commits, can you do that? Then we can merge.

Re-draft Deref docs

Make general advice more explicit and note the difference between
generic and specific implementations.

Re-draft DerefMut docs in-line with Deref

Fix Deref docs typos

Fix broken links

Clarify advice for specific-over-generic impls

Add comment addressing Issue rust-lang#73682

x fmt

Copy faillibility warning to DerefMut
@jmaargh
Copy link
Contributor Author

jmaargh commented Nov 4, 2023

@Mark-Simulacrum Squashed, should be good to merge after CI

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 4, 2023

📌 Commit 7a2f83f has been approved by Mark-Simulacrum

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 Nov 4, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#110340 (Deref docs: expand and remove "smart pointer" qualifier)
 - rust-lang#116894 (Guarantee that `char` has the same size and alignment as `u32`)
 - rust-lang#117534 (clarify that the str invariant is a safety, not validity, invariant)
 - rust-lang#117562 (triagebot no-merges: exclude different case)
 - rust-lang#117570 (fallback for `construct_generic_bound_failure`)
 - rust-lang#117583 (Remove `'tcx` lifetime on `PlaceholderConst`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 58645e0 into rust-lang:master Nov 4, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 4, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2023
Rollup merge of rust-lang#110340 - jmaargh:jmaargh/deref-docs, r=Mark-Simulacrum

Deref docs: expand and remove "smart pointer" qualifier

**Ready for review**

~~This is an unpolished draft to be sanity-checked~~

Fixes rust-lang#91004

~~Comments on substance and content of this are welcome. This is deliberately unpolished until ready to review so please try to stay focused on the big-picture.~~

~~Once this has been sanity checked, I will similarly update `DerefMut` and polish for review.~~
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 6, 2023
84: Automated pull from upstream `master` r=Dajamante a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117585
* rust-lang/rust#117576
* rust-lang/rust#96979
* rust-lang/rust#117191
* rust-lang/rust#117179
* rust-lang/rust#117574
* rust-lang/rust#117537
* rust-lang/rust#117608
  * rust-lang/rust#117596
  * rust-lang/rust#117588
  * rust-lang/rust#117524
  * rust-lang/rust#116017
* rust-lang/rust#117504
* rust-lang/rust#117469
* rust-lang/rust#116218
* rust-lang/rust#117589
* rust-lang/rust#117581
* rust-lang/rust#117503
* rust-lang/rust#117590
  * rust-lang/rust#117583
  * rust-lang/rust#117570
  * rust-lang/rust#117562
  * rust-lang/rust#117534
  * rust-lang/rust#116894
  * rust-lang/rust#110340
* rust-lang/rust#113343
* rust-lang/rust#117579
* rust-lang/rust#117094
* rust-lang/rust#117566
* rust-lang/rust#117564
  * rust-lang/rust#117554
  * rust-lang/rust#117550
  * rust-lang/rust#117343
* rust-lang/rust#115274
* rust-lang/rust#117540
* rust-lang/rust#116412
* rust-lang/rust#115333
* rust-lang/rust#117507
* rust-lang/rust#117538
  * rust-lang/rust#117533
  * rust-lang/rust#117523
  * rust-lang/rust#117520
  * rust-lang/rust#117505
  * rust-lang/rust#117434
* rust-lang/rust#117535
* rust-lang/rust#117510
* rust-lang/rust#116439
* rust-lang/rust#117508



Co-authored-by: Ben Wiederhake <BenWiederhake.GitHub@gmx.de>
Co-authored-by: SabrinaJewson <sejewson@gmail.com>
Co-authored-by: J-ZhengLi <lizheng135@huawei.com>
Co-authored-by: koka <koka.code@gmail.com>
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com>
Co-authored-by: lengyijun <sjtu5140809011@gmail.com>
Co-authored-by: Zalathar <Zalathar@users.noreply.github.com>
Co-authored-by: Oli Scherer <git-spam-no-reply9815368754983@oli-obk.de>
Co-authored-by: Philipp Krones <hello@philkrones.com>
Co-authored-by: y21 <30553356+y21@users.noreply.github.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: bohan <bohan-zhang@foxmail.com>
@jmaargh jmaargh deleted the jmaargh/deref-docs branch November 16, 2023 21:34
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.

Deref documentation: Remove references to "smart pointers"