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

Remove recommendation about idiomatic syntax for Arc::clone #63252

Open
wants to merge 1 commit into
base: master
from

Conversation

@nrc
Copy link
Member

commented Aug 4, 2019

I believe we should not make this recommendation. I don't want to argue that Arc::clone is less idiomatic than arc.clone, but that the choice is not clear cut and that we should not be making this kind of call in the docs.

The .clone() form has advantages too: it is more succinct, it is more likely to be understood by beginners, and it is more uniform with other clone calls, indeed with most other method calls.

Whichever approach is better, I think that this discussion belongs in a style guide or textbook, rather than the library docs. We don't talk much about idiomatic code in the docs, this place is pretty exceptional.

The recommendation is also not followed in this repo. It is hard to figure out how many calls there are of the .clone() form, but there are 1550 uses of Arc and only 65 uses of Arc::clone. The recommendation has existed for over two years.

The recommendation was added in #42137, as a result of rust-lang/rfcs#1954. However, note that that RFC was closed because it was not necessary to change the docs (the original RFC proposed a new function instead). So I don't think an RFC is necessary here (and I'm not trying to re-litigate the discussion on that RFC (which favoured Arc::clone as idiomatic) in any case).

cc @nical (who added the docs in the first place; sorry :-) )

r? @alexcrichton (or someone else on @rust-lang/libs )

Remove recommendation about idiomatic syntax for Arc::Clone
Signed-off-by: Nick Cameron <nrc@ncameron.org>
@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

it is more uniform with other clone calls

FWIW you call this an advantage but this same criteria also is subjectively considered a disadvantage as part of the motivation for #42137.

@nrc

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

FWIW you call this an advantage but this same criteria also is subjectively considered a disadvantage as part of the motivation for #42137.

Indeed! I believe reasonable people can differ on this point. But I don't think the docs are the right place to discuss it.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

I have a few thoughts:

  1. I agree that it's odd for the standard library to talk about style recommendations, and as you point rightly out, the recommendation doesn't seem to have been effective. (Also cc @rust-lang/docs -- this seems to me mostly a documentation issue rather than "What is the definition and guarantees of the standard library?") For this reason alone, I am in favor of removing the recommendation from the docs here.

  2. I've long wanted to provide a mid-way approach receiver.Arc::clone() which:

    • is more resistant to inference breakages
    • provides up-front queues about the type the method is being called on
    • is a smaller step from receiver.clone()
    • retains the ergonomics from an IDE and language POV of dot notation.

    ...but this is also a language change and will require more effort to push through, particularly in the "maturity year".

  3. If we truly believe being explicit about shallowness/sharing is important here (which is a different consideration than "expensive!") we should follow through on @scottmcm's suggestion of a new trait CloneRef. This has the main benefit of enabling such a distinction in generic code for which Arc::clone(&recv) is not helpful.

trait CloneRef : Clone {
    fn clone_ref(&self) -> Self { self.clone() }
}

impl<'a, T: ?Sized> CloneRef for &'a T {}
impl<T: ?Sized> CloneRef for std::rc::Rc<T> {}
impl<T: ?Sized> CloneRef for std::rc::Weak<T> {}
impl<T: ?Sized> CloneRef for std::sync::Arc<T> {}
impl<T: ?Sized> CloneRef for std::sync::Weak<T> {}
@nical

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

I can't think of a better place to document the idiomatic use of something than the official documentation of that thing. Now clippy or some official style guide doc would probably do as well.

In my opinion, since this is not about re-litigating the decision from rust-lang/rfcs#1954 about idiomatic use of the Rc::clone(&ref) syntax, it would be fair that some other effective means of communicating it be implemented before this one is removed.
I am not super attached to the documentation, I just want to avoid simply discarding it and regressing the original problem back to square one.

@nrc

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

I think adding some discussion to either the book or API guidelines would be best (although it is not part of the API as such). We could also add something to the style guide - that is mostly concerned with formatting style, but does have a few recommendations which are close to this one.

@brson

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I'm kind of indifferent, though I do think the existing advice is hard to follow (I just forget about it and expect to be able to .clone() everything clonable). The difference between the two also seems like something the type system will almost always sort out for you (like of like how variable shadowing "feels" bad, but because of Rust's strong typing is idiomatic in Rust) — if you clone wrong the types won't fit like you want.

@brson

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

It might be worth just softening the language, to say it may be more readable for smart pointer types to use fully-qualified function names. Or, if clippy/rustfmt doesn't just do this transform for you, maybe it should.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@rfcbot fcp merge

I personally agree with @nrc and think we should merge this, so let's see if others agree!

@rfcbot

This comment has been minimized.

Copy link

commented Aug 6, 2019

Team member @alexcrichton 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.

@nical

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

I would have appreciated if you had moved the recommendation to the API guidelines and/or the style guide as suggested by @nrc before going ahead and removing it from the standard doc, or at least filed issues there.
So I just filed two issues a bit in haste (sorry for the brievety), I'll try to find some time to make proper PRs eventually if nobody does it before then.

@Gankra

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Drive-by alumni team chime in: I am genuinely surprised that we still haven't grown an inherent method with a different name, like inc_strong() or something. I've kinda just been assuming people would eventually get annoyed enough with this situation to do it.

Like yeah it's cute that Clone is the same and that's good to know for composition, but, just let people write what they mean. It's not even that weird, we have plenty of inherent methods which incidentally exactly match the signature of common traits. See: new() and Default::default().

@rfcbot

This comment has been minimized.

Copy link

commented Aug 7, 2019

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

@scottmcm

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I'm 👍 for the idiomatic status to be removed.

But I think there'd be value in still talking about the UFCS-style in docs here, especially since it's still in the doctest example with this PR.

Strawman:

/// The [`Arc::clone(&from)`] syntax can be helpful if you wish to emphasize to the reader
/// that one is sharing the `Arc` rather than doing something potentially more expensive.

(And since I was pinged above, I would like x.Arc::clone() for other things, so would like it here too, though obviously that isn't going to happen soon, if ever.)

@nugend

This comment has been minimized.

Copy link

commented Aug 14, 2019

Is the Rc documentation being left as is?

@rfcbot

This comment has been minimized.

Copy link

commented Aug 17, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.