Allow the use of an extra trailing element in map restructure binding contexts

Description

Clojure programmers sometimes avoid using functions with keyword arguments because it's less performant than function calls using strictly positional arguments.

Additionally, Clojure programmers currently must choose between alternating key/value pairs or a single map in map binding contexts.

For example, programmers can define a function that takes keyword args or a map but not both:

If a programmer wishes to write a function that takes key/value pairs, or a map, or key/value pairs followed by a map then they must write code to perform checks in the function on its varargs and perhaps wrap that into a custom macro.

Cause

Clojure supports map destructure binding such that programmers must choose between APIs that support people and APIs that support programs. Ideally, any change should preserve partial application of keyword arguments. Further, map and Seq destructuring is absorbed by Clojure programmers and any such behavior added by this change should be available to developers wishing to emulate the same in their own applications and libraries. Additionally, the path from keyword args to a map detructure binding passes through ArraySeq which is handed off to PersistentHashMap/create(ISeq). This path could be optimized.

Approach

Map binding contexts like function parameters and let blocks allow destructuring against a Seq of key/value pairs OR a single map. Currently, destructuring a Seq of key/values requires precisely an even number of elements, each being a matched pair. We can leverage the fact that having an odd element at the end is an error mode by modifying Clojure to instead treat it as a priviledged element. This trailing element could be used to augment the map made with the preceeding key/value pairs. Handling of this trailing element should be an additive measure and not break older code.

Speed gains can be found by using PersistentArrayMap/createAsIfByAssoc(Object[]) to build map destructure RHS maps and tightening up the conversion of an ArraySeq to an array. This method could then handle the trailing map case to augment the resulting RHS map.

Environment

None

Activity

Show:
Alex Miller
February 1, 2021, 8:36 PM

Comments on -4 patch:

  • Re the docstrings - does is seem a little weird to "as if by assoc" but for the trailing arg, "as if by conj"?

  • PAM: for the "normal" even-count path, create(ISeq) -> createAsIfByAssoc(init) will check in both methods for even/oddness. I don't really care about the direct call to createAsIfByAssoc as presumably no one is calling that anymore but it's a shame to incur that check twice (actually 3 times as cAIBA is called twice) in the array-map construction path which is expected to be fast. Can we pull the guts of createAsIfByAssoc out sans the check into a new private method and call that from both instead?

  • PAM.growSeedArray() - seems like you could construct the result array, copy seed to results, and then write trailing directly into result rather than writing to newKvs and copying, so newKvs would go away

  • PAM.growSeedArray() - in the two latter places where you use newKvs.length, I think it would be faster to use sz instead (don't need to ask the array, you already have the number sitting there in a local). This is moot if prior suggestion is taken though.

  • Still looks like there are some tests that rely on keys/vals order from unordered maps, like `(keys (array-map :a 1 {:b 2})) '[:a :b]`

  • Maybe we should add a generative test to gen kvs, construct with hash-map and array-map (and sorted-map?) and assert they are all =, have same count, etc

Michael Fogus
February 2, 2021, 1:40 PM

Thank you for the feedback. In the case of the key/val ordering in at array-map case I kept the ordered keys/val check because that is part of the guarantee inherent to array-map. I decided to skip the generative tests for now as they seem outside of the scope of this change but I’d love to discuss a ticket that addresses missing generative tests beyond just these two functions.

Assignee

Michael Fogus

Reporter

Michael Fogus

Approval

Vetted

Patch

Code and Test

Fix versions

Priority

Major