Clojure

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

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6
  • Fix Version/s: None
  • Component/s: None
  • Patch:
    Code

Description

The docstrings of both `assoc!` and `conj!` say "Returns coll.", suggesting the transient edit happens always in-place, `coll` being the first argument.

However, the fact that the following example omits the key `8` in its result proves that in-place edits aren't always the case:

(let [a (transient {})]
      (dotimes [x 9]
        (assoc! a x :ok))
      (persistent! a))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok}

Instead, programmers should be guided towards using constructs like `reduce` with transients:

(persistent! (reduce #(assoc! %1 %2 :ok)
                 (transient {})
                 (range 9)))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok, 8 :ok}

The easiest way to achieve this is by changing the docstrings of (at least) `conj!` and `assoc!` to not read "Returns coll." but instead tell that the change is destructive.

Activity

Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Gary Fredericks added a comment -

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.

Show
Gary Fredericks added a comment - 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.
Hide
Alex Miller added a comment -

@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.

Show
Alex Miller added a comment - @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.
Hide
Pyry Jahkola added a comment -

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

Show
Pyry Jahkola added a comment - @Alex, that update makes it sound right to me, FWIW.
Hide
Gary Fredericks added a comment -

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?

Show
Gary Fredericks added a comment - 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?
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Alex Miller added a comment -

Yup, patch desired.

Show
Alex Miller added a comment - Yup, patch desired.
Hide
Gary Fredericks added a comment -

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).

Show
Gary Fredericks added a comment - 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).
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Gary Fredericks added a comment -

Attached a new patch.

Show
Gary Fredericks added a comment - Attached a new patch.

People

Vote (2)
Watch (2)

Dates

  • Created:
    Updated: