Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place

Question

The docstrings of both assoc! and conj! say "Returns coll.", possibly suggesting the transient edit happens (always) in-place, coll being the first argument. However, this is not the case and the returned collection should always be the one that's used.

Approach: Replace "Returns coll." with "Returns an updated collection." in conj(image: , assoc), pop! docstrings.

Patch: CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch

Screened by: Alex Miller

Answer 1

Comment made by: alexmiller

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! -- transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Answer 2

Comment made by: gfredericks

Alex I think you must have misread the ticket -- the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and {{assoc}} do not return {{coll}} at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

Answer 3

Comment made by: alexmiller

@Gary - you're right, I did misread that.

{{assoc}} and {{conj}} both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Answer 4

Comment made by: pyrtsa

@Alex, that update makes it sound right to me, FWIW.

Answer 5

Comment made by: gfredericks

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Answer 6

Comment made by: jafingerhut

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Answer 7

Comment made by: alexmiller

Yup, patch desired.

Answer 8

Comment made by: gfredericks

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).

Answer 9

Comment made by: jafingerhut

Patch CLJ-1385-reword-docstrings-on-transient-update-funct.patch dated Apr 6 2014 no longer applies to latest Clojure master cleanly, due to some changes committed earlier today. I suspect it should be straightforward to update the patch to apply cleanly, given that they are doc string changes, but there may have been doc string changes committed to master, too.

Answer 10

Comment made by: gfredericks

Attached a new patch.

Answer 11

Comment made by: richhickey

I think it could be clearer still, since we want people to know the original coll might have been affected and returned, and the return must be used for subsequent calls. I think some of the language from the transients page should make it into these docstrings.

Answer 12

Comment made by: jafingerhut

Would it be correct to say that the collection passed into pop! conj(image: assoc) etc. has undefined contents after the operation completes, and only the return value has defined contents?

That kind of strong wording may get people's attention.

Answer 13

Comment made by: alexmiller

I'm working on this.

Answer 14

Comment made by: alexmiller

incomplete and I (still) own this one

Answer 15

Reference: https://clojure.atlassian.net/browse/CLJ-1385 (reported by alex+import)