Explain fully qualified syntax for `Rc` and `Arc` #76138

camelid · 2020-08-31T01:47:11Z

Also cleaned up some other small things.

@rustbot modify labels: T-doc

rust-highfive · 2020-08-31T01:47:14Z

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

matklad · 2020-09-01T10:47:30Z

library/alloc/src/rc.rs

@@ -35,15 +35,26 @@
 //! `Rc<T>` automatically dereferences to `T` (via the [`Deref`] trait),
 //! so you can call `T`'s methods on a value of type [`Rc<T>`][`Rc`]. To avoid name
 //! clashes with `T`'s methods, the methods of [`Rc<T>`][`Rc`] itself are associated
-//! functions, called using function-like syntax:
+//! functions, called using [fully qualified syntax]:


Do we actually call this "fully qualified syntax"? I thought that would be reserved for <RC>::downgrade form.

Naively, "function like syntax" or "associated function syntax" seem like better options to me here.

Yes, "fully-qualified sytax" is used for the form with the <>s. I would drop the extra clause, and just end after "functions."

Hmm, wouldn’t it be better to give the readers somewhere to read more about fully qualified syntax? Side note, shouldn’t it be “fully-qualified syntax” with a hyphen?

wouldn’t it be better to give the readers somewhere to read more about fully qualified syntax?

Maybe. I am purely thinking about the wording itself.

Side note, shouldn’t it be “fully-qualified syntax” with a hyphen?

I don't think so; the hyphen would be used if it is confusing which part "qualified" is attached to, that is, to disambiguate between

fully-qualified syntax fully qualified-syntax

but I don't think that the latter would be a common interpretation

matklad · 2020-09-01T10:54:47Z

library/alloc/src/rc.rs

+//! `Rc<T>`'s implementations of traits like `Clone` should also be called using
+//! fully qualified syntax to avoid confusion as to whether the *reference* is being
+//! cloned or the *backing data* (`T`) is being cloned:
+//!
+//! ```
+//! use std::rc::Rc;
+//!
+//! let my_rc = Rc::new(());
+//! let your_rc = Rc::clone(&my_rc);
+//! ```


Note that we explicitly decided against this in #63252.

My personal opinion is that people do use Arc::clone syntax in the wild, so this should be in the official docs one way or another, but probably without "should" or "idiomatic" langauge.

Urgh, but #63252 didn't update

https://github.com/rust-lang/rust/blame/6f1bbf5ee014cdad5d95f13266b177d89cc40d89/library/alloc/src/rc.rs#L64-L66

I guess, the status quo is confused here, and we should probably try to be consistent at least withing stdlib itself :)

We teach the ::clone version in the book too; I wasn't notified of that thread either :)

I learned the ::clone version (I guess from the book?) and I think it has clearer intent. Maybe for now we’ll go with it here and then we can change it later if we end up updating places like in the book?

+1 for not using ::clone as per #63252
It's no longer the official guidance, and I don't think the existence of its use in other resources is good justification for continuing to use it in example code.

still waiting for this change, @camelid . (I don't like it but it appears to be the consensus.)

bors · 2020-09-03T08:34:43Z

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

camelid · 2020-09-03T19:54:01Z

Rebased.

bors · 2020-09-04T01:57:23Z

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

camelid · 2020-09-04T02:36:04Z

Not again!

camelid · 2020-09-04T02:43:43Z

Rebased.

cramertj · 2020-09-15T23:57:46Z

compiler/rustc_typeck/src/astconv/mod.rs

@@ -1266,7 +1266,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
        } else {
            err.span_suggestion(
                span,
-                "use fully-qualified syntax",
+                "use fully qualified syntax",


Why was the hyphen here removed? It seems correct to me.

I agree; I think it's correct with the hyphen. The issue is that it's referred to without the hyphen everywhere else in the compiler and in the book. I figure that unless and until we change it elsewhere, it should be consistent.

See @steveklabnik's comment: #76138 (comment).

crlf0710 · 2020-10-08T10:45:42Z

r? @steveklabnik

steveklabnik · 2020-10-08T14:58:11Z

library/alloc/src/rc.rs

+//! `Rc<T>`'s implementations of traits like `Clone` should also be called using
+//! fully qualified syntax to avoid confusion as to whether the *reference* is being
+//! cloned or the *backing data* (`T`) is being cloned:
+//!
+//! ```
+//! use std::rc::Rc;
+//!
+//! let my_rc = Rc::new(());
+//! let your_rc = Rc::clone(&my_rc);
+//! ```


still waiting for this change, @camelid . (I don't like it but it appears to be the consensus.)

Dylan-DPC-zz · 2020-10-28T00:37:12Z

@camelid any updates on this? thanks

camelid · 2020-10-28T00:42:23Z

Yeah, I've been meaning to get to this.

camelid · 2020-10-28T23:32:14Z

Starting with a rebase.

camelid · 2020-10-28T23:33:47Z

I'm dropping the commit changing fully-qualified syntax to fully qualified syntax in the compiler since it should be a separate change.

That recommendation was removed last year; there isn't a particular style that is officially recommended anymore.

camelid · 2020-10-28T23:50:31Z

@steveklabnik This is ready for a re-review :)

steveklabnik · 2020-10-29T15:18:29Z

Thank you!

@bors: r+ rollup

bors · 2020-10-29T15:18:31Z

📌 Commit 4e30e10 has been approved by steveklabnik

…as-schievink Rollup of 11 pull requests Successful merges: - rust-lang#75078 (Improve documentation for slice strip_* functions) - rust-lang#76138 (Explain fully qualified syntax for `Rc` and `Arc`) - rust-lang#78244 (Dogfood {exclusive,half-open} ranges in compiler (nfc)) - rust-lang#78422 (Do not ICE on invalid input) - rust-lang#78423 (rustc_span: improve bounds checks in byte_pos_to_line_and_col) - rust-lang#78431 (Prefer new associated numeric consts in float error messages) - rust-lang#78462 (Use unwrapDIPtr because the Scope may be null.) - rust-lang#78493 (Update cargo) - rust-lang#78499 (Prevent String::retain from creating non-utf8 strings when abusing panic) - rust-lang#78505 (Update Clippy - temporary_cstring_as_ptr deprecation) - rust-lang#78527 (Fix some more typos) Failed merges: r? `@ghost`

Fix missing link for "fully qualified syntax" This issue can currently be seen at https://doc.rust-lang.org/stable/std/rc/index.html#toggle-all-docs:~:text=%5B-,fully%20qualified%20syntax It originates from rust-lang#76138, where the link was added to `library/alloc/src/sync.rs`, but not `library/alloc/src/rc.rs`.

**This Commit** Denies the use of `.clone()` on reference counted pointers as advised by [this `clippy` lint][0] and [The Rust Book][1]. It also addresses instances where the lint was violated. **Why?** It's best practice to make clear the clone is cheap. There is [some debate][2] on the issue and the [standard library no longer explicitly recommends it][3] but it is still noted as more idiomatic. In any case, for us there are no issues making the change and if there is a possibility for being more expressive, we should take it. See #6 (comment) for more details. [0]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr [1]: https://doc.rust-lang.org/book/ch15-04-rc.html#using-rct-to-share-data [2]: rust-lang/rust-clippy#2048 [3]: rust-lang/rust#76138

**This Commit** Adds history to the functional BST and no longer requires ownership to `insert`/`delete`. **Why?** Mainly to make benchmarking easier but this has the benefit of more closely matching a functional data structure in that, when new trees are created from "mutating" operations, the history of trees/operations is still accessible. Use struct update syntax for conciseness **Why?** I didn't think I could do this but the issue was that I had `derive`d `Clone` on `Node` when I should've implemented it directly because it's generic and leverages reference counting. See #6 (comment) Explicitly clone needed fields of nodes **This Commit** Removes instances of instantiating new `Node`s using the functional update syntax of a clone of an existing `Node`. That is it replaces: ```rust Node { some_field: some_value, ..existing_node.clone() } ``` with ```rust Node { some_field: some_value, other_field: existing_node.other_field.clone(), // etc. } ``` **Why?** For clarity - when using `..node.clone()` what's happening is we are first cloning the node in its entirety (which results in bumping the reference count for all its fields), moving the fields we care about to the new `Node`, and then dropping the remaining fields (and decrementing their reference count). This is a surprise to the reader and is needless and not what we want to express. It also may have a performance impact but that isn't measured. For more details see [this comment thread][0]. [0]: #6 (comment) Deny `clippy::clone_on_ref_ptr` **This Commit** Denies the use of `.clone()` on reference counted pointers as advised by [this `clippy` lint][0] and [The Rust Book][1]. It also addresses instances where the lint was violated. **Why?** It's best practice to make clear the clone is cheap. There is [some debate][2] on the issue and the [standard library no longer explicitly recommends it][3] but it is still noted as more idiomatic. In any case, for us there are no issues making the change and if there is a possibility for being more expressive, we should take it. See #6 (comment) for more details. [0]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr [1]: https://doc.rust-lang.org/book/ch15-04-rc.html#using-rct-to-share-data [2]: rust-lang/rust-clippy#2048 [3]: rust-lang/rust#76138

rustbot added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Aug 31, 2020

rust-highfive assigned cramertj Aug 31, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2020

matklad reviewed Sep 1, 2020

View reviewed changes

camelid force-pushed the rc-fully-qualified-syntax branch from 66444e3 to 9f59bc6 Compare September 3, 2020 19:53

camelid force-pushed the rc-fully-qualified-syntax branch from 9f59bc6 to 3cdd7cb Compare September 4, 2020 02:43

cramertj reviewed Sep 15, 2020

View reviewed changes

rust-highfive assigned steveklabnik and unassigned cramertj Oct 8, 2020

steveklabnik suggested changes Oct 8, 2020

View reviewed changes

camelid 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 Oct 11, 2020

camelid added 2 commits October 28, 2020 16:31

Explain fully qualified syntax for Rc and Arc

bd7cbae

Fix broken intra-doc link

e0eed3c

camelid force-pushed the rc-fully-qualified-syntax branch from 3cdd7cb to b95e0c1 Compare October 28, 2020 23:32

camelid force-pushed the rc-fully-qualified-syntax branch from b95e0c1 to e0eed3c Compare October 28, 2020 23:34

Don't say you "should" use fully qualified syntax

4e30e10

That recommendation was removed last year; there isn't a particular style that is officially recommended anymore.

camelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 28, 2020

camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 28, 2020

steveklabnik approved these changes Oct 29, 2020

View reviewed changes

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 Oct 29, 2020

jonas-schievink mentioned this pull request Oct 29, 2020

Rollup of 11 pull requests #78528

Merged

bors merged commit a384a58 into rust-lang:master Oct 30, 2020

rustbot added this to the 1.49.0 milestone Oct 30, 2020

camelid deleted the rc-fully-qualified-syntax branch October 30, 2020 00:33

jjlin mentioned this pull request Jan 3, 2021

Fix missing link for "fully qualified syntax" #80666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explain fully qualified syntax for `Rc` and `Arc` #76138

Explain fully qualified syntax for `Rc` and `Arc` #76138

camelid commented Aug 31, 2020

rust-highfive commented Aug 31, 2020

matklad Sep 1, 2020

steveklabnik Sep 2, 2020

camelid Sep 2, 2020

steveklabnik Sep 9, 2020

matklad Sep 1, 2020

matklad Sep 1, 2020 •

edited

Loading

steveklabnik Sep 2, 2020

camelid Sep 2, 2020

cramertj Sep 15, 2020

steveklabnik Oct 8, 2020

bors commented Sep 3, 2020

camelid commented Sep 3, 2020

bors commented Sep 4, 2020

camelid commented Sep 4, 2020

camelid commented Sep 4, 2020

cramertj Sep 15, 2020

camelid Sep 16, 2020

camelid Sep 16, 2020

crlf0710 commented Oct 8, 2020

steveklabnik Oct 8, 2020

Dylan-DPC-zz commented Oct 28, 2020

camelid commented Oct 28, 2020

camelid commented Oct 28, 2020

camelid commented Oct 28, 2020

camelid commented Oct 28, 2020

steveklabnik commented Oct 29, 2020

bors commented Oct 29, 2020

Explain fully qualified syntax for Rc and Arc #76138

Explain fully qualified syntax for Rc and Arc #76138

Conversation

camelid commented Aug 31, 2020

rust-highfive commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 3, 2020

camelid commented Sep 3, 2020

bors commented Sep 4, 2020

camelid commented Sep 4, 2020

camelid commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crlf0710 commented Oct 8, 2020

Choose a reason for hiding this comment

Dylan-DPC-zz commented Oct 28, 2020

camelid commented Oct 28, 2020

camelid commented Oct 28, 2020

camelid commented Oct 28, 2020

camelid commented Oct 28, 2020

steveklabnik commented Oct 29, 2020

bors commented Oct 29, 2020

Explain fully qualified syntax for `Rc` and `Arc` #76138

Explain fully qualified syntax for `Rc` and `Arc` #76138

matklad Sep 1, 2020 •

edited

Loading