Clojure

update-in with empty key paths

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Declined
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Triaged

Description

To the topic of get-in and update-in. While I realize this is not a bug it is odd and in my eyes unexpected and unwanted behavior.

get-in called with an empty path returns the hashmap it was given to walk through - this is as I expect and makes a lot of sense when dynamically (with generated pathes ) walking a hash map.

update-in behaves differently and while from the implementation side, it's behavior too makes sense, it does not work as expected (at least not for me) update-in with an empty map creates a new key 'nil' so:

(update-in {...} [] f) ist he same as (update-in {...} [nil] f) while (get-in {...} []) is not the same as (get-in {...} [nil]) and of cause differs from what update-in does.

For automatically walking trees the behavior of get-in makes a lot more sense since the current behavior of update-in forces you to check for empty paths and if they are empty fall back to plain assoc and get (or get-in since this works):

(if-let [r (butlast @path)]
(do
  (alter m update-in r dissoc (last @path))
  (alter m update-in r assoc {:name @sr} c))
(do
  (alter m dissoc (last @path))
  (alter m assoc {:name @sr} c)))

Next argument is that update-in with an empty map working on nil isn't easy to gasp, one needs to know the implementation details to realize that it works, I think 90% of the people reading update-in with [] will not instinctively know that it works on the key nil, so changing this would most likely not break any current code, and if it would the code would be bad anyway .

Chouser has, a very nice solution on the mailing list that would fix the problem I'm not sure if I'm entitled to post it here since I did not wrote it but it can be found in this thread: http://groups.google.com/group/clojure/browse_thread/thread/de5b20b8c3fe498b?hl=en

Regards,
Heinz

Activity

Hide
Assembla Importer added a comment -
Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/373
Rich Hickey made changes -
Field Original Value New Value
Issue Type Defect [ 1 ] Enhancement [ 4 ]
Reporter Assembla Importer [ importer ]
Priority Blocker [ 1 ]
Hide
Chouser added a comment -

I've attached my code from the g.group thread in the form of a patch, in case it's sufficient. (Thanks to abedra for the gentle kick in the pants.)

Show
Chouser added a comment - I've attached my code from the g.group thread in the form of a patch, in case it's sufficient. (Thanks to abedra for the gentle kick in the pants.)
Chouser made changes -
Attachment 0001-Support-empty-path-in-update-in.-CLJ-373.patch [ 10019 ]
Approval Vetted Test
Hide
Stuart Halloway added a comment -

Looks to me like the patch is misusing if-let, e.g.

(when-let [[k & mk] []] "Doh!") 
=> Doh!

Please correct, and add tests for nil and [] keys (at least).

Show
Stuart Halloway added a comment - Looks to me like the patch is misusing if-let, e.g.
(when-let [[k & mk] []] "Doh!") 
=> Doh!
Please correct, and add tests for nil and [] keys (at least).
Stuart Halloway made changes -
Priority Blocker [ 1 ] Minor [ 4 ]
Approval Test Incomplete
Waiting On chouser@n01se.net
Alexander Redington made changes -
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Release.Next [ 10038 ]
Hide
Scott Lowe added a comment -

I will write some tests and correct this.

Show
Scott Lowe added a comment - I will write some tests and correct this.
Hide
Scott Lowe added a comment - - edited

I'm sorry to report that my good intentions of wanting to help clear some of the project backlog has created more work by way of further questions. I'd also like to clarify the desired new behaviours for the test cases.

Heinz proposed that an empty key sequence will not create a new nil key in the returned map.
He also suggested that the following behaviour changes be made (compare old and new* behaviours):

 
(update-in {1 2} [] (constantly {2 3}))
{nil {2 3}, 1 2}

(update-in* {1 2} [] (constantly {2 3}))
{2 3}

(update-in {1 2} [] assoc  1 3)
{nil {1 3}, 1 2}

(update-in* {1 2} [] assoc  1 3)
{1 3}

Chouser also added that nil keys should be honoured, as before:

 
(update-in* {nil 2} [nil] (constantly 3))
{nil 3}

I've added a variety of tests to cover the existing behaviour and would like to confirm that the above is all that's required for new behaviour.

The patch from November 2010 didn't work, but I tweaked it with a when-let as Stuart suggested and placed a check for an empty sequence of keys before the when-let block; because essentially, the primary behaviour change boils down to simply handling an empty sequence of keys, in addition to the existing behaviours.

I'm not entirely convinced that these changes are a good thing, but at least there's now something concrete for discussion. Please have a look at what is there. The good news is that at least there are some tests covering update-in now.

Show
Scott Lowe added a comment - - edited I'm sorry to report that my good intentions of wanting to help clear some of the project backlog has created more work by way of further questions. I'd also like to clarify the desired new behaviours for the test cases. Heinz proposed that an empty key sequence will not create a new nil key in the returned map. He also suggested that the following behaviour changes be made (compare old and new* behaviours):
 
(update-in {1 2} [] (constantly {2 3}))
{nil {2 3}, 1 2}

(update-in* {1 2} [] (constantly {2 3}))
{2 3}

(update-in {1 2} [] assoc  1 3)
{nil {1 3}, 1 2}

(update-in* {1 2} [] assoc  1 3)
{1 3}
Chouser also added that nil keys should be honoured, as before:
 
(update-in* {nil 2} [nil] (constantly 3))
{nil 3}
I've added a variety of tests to cover the existing behaviour and would like to confirm that the above is all that's required for new behaviour. The patch from November 2010 didn't work, but I tweaked it with a when-let as Stuart suggested and placed a check for an empty sequence of keys before the when-let block; because essentially, the primary behaviour change boils down to simply handling an empty sequence of keys, in addition to the existing behaviours. I'm not entirely convinced that these changes are a good thing, but at least there's now something concrete for discussion. Please have a look at what is there. The good news is that at least there are some tests covering update-in now.
Scott Lowe made changes -
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Hide
Andy Fingerhut added a comment -

clj-373-alter-behavior-of-update-in-with-empty-key-patch2.txt dated May 24, 2012 supersedes patch 0001-CLJ-373Alter-behaviour-of-update-in-with-empty-key.patch dated May 9, 2012. It makes no changes to the contents of the patch except in the lines of context that have changed since the earlier patch was created. It applies cleanly to the latest master whereas the earlier patch no longer does. Builds and passes tests with Oracle/Apple JDK 1.6 on Mac OS X 10.6.8.

Show
Andy Fingerhut added a comment - clj-373-alter-behavior-of-update-in-with-empty-key-patch2.txt dated May 24, 2012 supersedes patch 0001-CLJ-373Alter-behaviour-of-update-in-with-empty-key.patch dated May 9, 2012. It makes no changes to the contents of the patch except in the lines of context that have changed since the earlier patch was created. It applies cleanly to the latest master whereas the earlier patch no longer does. Builds and passes tests with Oracle/Apple JDK 1.6 on Mac OS X 10.6.8.
Andy Fingerhut made changes -
Rich Hickey made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Release 1.5 [ 10150 ]
Fogus made changes -
Assignee Fogus [ fogus ]
Hide
Fogus added a comment -

This patch creates a new error mode highlighted by the following:

(update-in {:a 1} [] inc)

; ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.lang.Number

The reason is that the code that executes in the event that the keys are empty (or nil) blindly assumes that the function given can be applied to the map itself. This is no problem in the case of assoc and the like, but clearly not for inc and many other useful functions.

The old behavior was to throw an NPE and if any code relies on that fact is now broken. Maybe this is not a problem, but I'm kicking it back to get a little more discussion and to request that whatever the ultimate fix happens to be, its behavioral changes and error modes are noted in the docstring for update-in.

Show
Fogus added a comment - This patch creates a new error mode highlighted by the following:
(update-in {:a 1} [] inc)

; ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.lang.Number
The reason is that the code that executes in the event that the keys are empty (or nil) blindly assumes that the function given can be applied to the map itself. This is no problem in the case of assoc and the like, but clearly not for inc and many other useful functions. The old behavior was to throw an NPE and if any code relies on that fact is now broken. Maybe this is not a problem, but I'm kicking it back to get a little more discussion and to request that whatever the ultimate fix happens to be, its behavioral changes and error modes are noted in the docstring for update-in.
Fogus made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Rich Hickey made changes -
Fix Version/s Release 1.5 [ 10150 ]
Fogus made changes -
Assignee Fogus [ fogus ]
Hide
Gunnar Völkel added a comment - - edited

I vote for changing `update-in` to support empty key vectors.

Because I think "(apply f m args)" in case of an empty vector is the natural continuation of the usual behavior of update-in:

  • update-in with 2 keys the second level map is updated
  • update-in with 1 key the first level map (child of given map) is updated
  • update-in with no key the given map (zero level) is updated.

Otherwise, you will always have to wrap update-in in something like the following when the keys vector is computed:

(if (seq keys) (apply update-in m keys f args) (apply f m args))

To Fogus' last post: (update-in {:a {:b 1}} [:a] inc) fails similar and is not handled with a special exception.

Show
Gunnar Völkel added a comment - - edited I vote for changing `update-in` to support empty key vectors. Because I think "(apply f m args)" in case of an empty vector is the natural continuation of the usual behavior of update-in:
  • update-in with 2 keys the second level map is updated
  • update-in with 1 key the first level map (child of given map) is updated
  • update-in with no key the given map (zero level) is updated.
Otherwise, you will always have to wrap update-in in something like the following when the keys vector is computed:
(if (seq keys) (apply update-in m keys f args) (apply f m args))
To Fogus' last post: (update-in {:a {:b 1}} [:a] inc) fails similar and is not handled with a special exception.
Alex Miller made changes -
Approval Incomplete [ 10006 ]
Hide
Tim McCormack added a comment -

Fogus, if there was code relying on that brokenness, I'd say it was only working by accident.

Show
Tim McCormack added a comment - Fogus, if there was code relying on that brokenness, I'd say it was only working by accident.
Hide
Ambrose Bonnaire-Sergeant added a comment -

Attached new patch CLJ-373-nested-ops.patch.

It implements update-in with an empty path equivalent to (apply f m args).

Since they are clearly related, I changed assoc-in to throw an error on an empty path, and updated {update,get,assoc}-in's docstring to reflect these changes.

I changed the terminology of a "sequence of keys" to a "path of keys (a sequence)", and henceforth referred to the sequence as a key path, or the name of the related arg.

Show
Ambrose Bonnaire-Sergeant added a comment - Attached new patch CLJ-373-nested-ops.patch. It implements update-in with an empty path equivalent to (apply f m args). Since they are clearly related, I changed assoc-in to throw an error on an empty path, and updated {update,get,assoc}-in's docstring to reflect these changes. I changed the terminology of a "sequence of keys" to a "path of keys (a sequence)", and henceforth referred to the sequence as a key path, or the name of the related arg.
Ambrose Bonnaire-Sergeant made changes -
Attachment CLJ-373-nested-ops.patch [ 12595 ]
Hide
Andy Fingerhut added a comment -

Patch CLJ-373-nested-ops.patch dated Jan 5 2014 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.

Show
Andy Fingerhut added a comment - Patch CLJ-373-nested-ops.patch dated Jan 5 2014 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.
Alex Miller made changes -
Approval Triaged [ 10120 ]
Ambrose Bonnaire-Sergeant made changes -
Attachment CLJ-373-nested-ops.patch [ 12595 ]
Hide
Ambrose Bonnaire-Sergeant added a comment - - edited

Updated with clean patch.

Show
Ambrose Bonnaire-Sergeant added a comment - - edited Updated with clean patch.
Ambrose Bonnaire-Sergeant made changes -
Attachment CLJ-373-nested-ops.patch [ 12897 ]
Rich Hickey made changes -
Resolution Declined [ 2 ]
Status Open [ 1 ] Closed [ 6 ]
Hide
Timothy Baldridge added a comment -

Can we get some feedback on why this was rejected (with no comments)? Seems like a lot of work/interest went into this. Would be nice to know why all that was thrown out without ceremony.

Show
Timothy Baldridge added a comment - Can we get some feedback on why this was rejected (with no comments)? Seems like a lot of work/interest went into this. Would be nice to know why all that was thrown out without ceremony.
Hide
Andy Fingerhut added a comment -

Standard disclaimer: I'm not in the loop. I have a guess where the loop is, but I'm nowhere near it.

Timothy, I doubt many people look at comments on tickets after they are closed, except perhaps Alex Miller and I. If you have as good a communication channel with Rich as Alex does, asking him may be the only way to find out. Other tickets with new functions proposed for clojure.core have been closed with comments similar to "we are not interested in adding this functionality to Clojure at this time". That might be all there is to know.

Show
Andy Fingerhut added a comment - Standard disclaimer: I'm not in the loop. I have a guess where the loop is, but I'm nowhere near it. Timothy, I doubt many people look at comments on tickets after they are closed, except perhaps Alex Miller and I. If you have as good a communication channel with Rich as Alex does, asking him may be the only way to find out. Other tickets with new functions proposed for clojure.core have been closed with comments similar to "we are not interested in adding this functionality to Clojure at this time". That might be all there is to know.
Hide
Anders Hovmöller added a comment -

I was bitten by this today. I had to insert an extra "(if (= (count path) 0) <do something else>...". Was quite surprised at the extra nil inserted into my map!

Show
Anders Hovmöller added a comment - I was bitten by this today. I had to insert an extra "(if (= (count path) 0) <do something else>...". Was quite surprised at the extra nil inserted into my map!
Hide
Rich Hickey added a comment -

The problem with this ticket was identified in the comment by Fogus 15/Aug/12.

Show
Rich Hickey added a comment - The problem with this ticket was identified in the comment by Fogus 15/Aug/12.
Hide
Timothy Baldridge added a comment -

As mentioned by Fogus, the code he posted already throws an exception today (in 1.6):

In 1.6:

=> (update-in {:a 1} []  inc)
NullPointerException   clojure.lang.Numbers.ops (Numbers.java:961)

According to Fogus, after this patch:

(update-in {:a 1} [] inc)

; ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.lang.Number

So before this patch, nil was passed into the function if the path was nil or []. Now the root is passed in. So yes, any code that expected to get nil will now receive the root, sometimes that will change the error type (as shown above) other times code may break existing code. Since the doc-string for update-in does not specify what will occur if the path is empty, perhaps a case could be made that those that depend on the old behavior are relying on undefined semantics.

I've yet to hear of or see code in production that relies on the current semantics of update-in with an empty path, so I would vote for making a note of how this could affect some people and putting that note in the change log.

Show
Timothy Baldridge added a comment - As mentioned by Fogus, the code he posted already throws an exception today (in 1.6): In 1.6:
=> (update-in {:a 1} []  inc)
NullPointerException   clojure.lang.Numbers.ops (Numbers.java:961)
According to Fogus, after this patch:
(update-in {:a 1} [] inc)

; ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.lang.Number
So before this patch, nil was passed into the function if the path was nil or []. Now the root is passed in. So yes, any code that expected to get nil will now receive the root, sometimes that will change the error type (as shown above) other times code may break existing code. Since the doc-string for update-in does not specify what will occur if the path is empty, perhaps a case could be made that those that depend on the old behavior are relying on undefined semantics. I've yet to hear of or see code in production that relies on the current semantics of update-in with an empty path, so I would vote for making a note of how this could affect some people and putting that note in the change log.

People

Vote (7)
Watch (10)

Dates

  • Created:
    Updated:
    Resolved: