<< Back to previous view

[CLJ-373] update-in with empty key paths Created: 04/Jun/10  Updated: 04/Aug/14  Resolved: 10/Jun/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Declined Votes: 7
Labels: None

Attachments: Text File 0001-CLJ-373-Alter-behaviour-of-update-in-with-empty-key-.patch     Text File 0001-Support-empty-path-in-update-in.-CLJ-373.patch     Text File clj-373-alter-behavior-of-update-in-with-empty-key-patch2.txt     Text File CLJ-373-nested-ops.patch    
Patch: Code and Test
Approval: Triaged
Waiting On: Chouser

 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



 Comments   
Comment by Assembla Importer [ 08/Oct/10 10:33 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/373

Comment by Chouser [ 13/Nov/10 6:37 PM ]

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

Comment by Stuart Halloway [ 29/Nov/10 8:20 AM ]

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

Comment by Scott Lowe [ 08/May/12 12:20 PM ]

I will write some tests and correct this.

Comment by Scott Lowe [ 09/May/12 8:39 PM ]

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.

Comment by Andy Fingerhut [ 24/May/12 10:35 PM ]

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.

Comment by Fogus [ 15/Aug/12 11:21 AM ]

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.

Comment by Gunnar Völkel [ 08/Sep/12 6:11 AM ]

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.

Comment by Tim McCormack [ 25/Nov/13 11:45 AM ]

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

Comment by Ambrose Bonnaire-Sergeant [ 05/Jan/14 9:13 AM ]

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.

Comment by Andy Fingerhut [ 14/Feb/14 11:42 AM ]

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.

Comment by Ambrose Bonnaire-Sergeant [ 24/Mar/14 12:39 PM ]

Updated with clean patch.

Comment by Timothy Baldridge [ 30/Jun/14 10:33 AM ]

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.

Comment by Andy Fingerhut [ 01/Jul/14 6:24 PM ]

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.

Comment by Anders Hovmöller [ 20/Jul/14 2:09 PM ]

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!

Comment by Rich Hickey [ 04/Aug/14 10:29 AM ]

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

Comment by Timothy Baldridge [ 04/Aug/14 11:10 AM ]

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.

Generated at Sun Nov 23 01:31:25 CST 2014 using JIRA 4.4#649-r158309.