Merge map-of into map

[jeans11]

#43

(m/validate [:map keyword? int?] {:x 1 :y 2})
;; true

(m/validate
  [:map
    [:x string?]
    [[:opt :y] int?]
    [keyword? [:vector int?]]]
  {:x "x"
   :z [1 2 3]})
;; true

(m/validate [:map [:x int?] [keyword? string?]] {:x 1 :z 2})
;; false

[ikitommi]

Thanks for the PR! Looking good, few comments to make it even better :)

[ikitommi]

I think less is more: map already has special syntax (in form of map-entries are represented as a vector), I think we could force all entries to be represented this way, so special syntax for only map-of. The code would be much simpler too, I believe.

;; illegal
[:map int? int?]

;; ok
[:map [int? int?]]

what do you think?

[jeans11]

Good idea!

[ikitommi]

let's not push any code from compile time into validation time if possible. Hre, validators vector could contain first the normal key validators and the the extra validators. Same goes for -explainer and -transformer methods, all on the hot perf path.

There is a small perf regression test suite if you want to peek how this would effect perf. My guess it's 2-5x times slower than before, see
https://github.com/metosin/malli/blob/master/perf/malli/perf_test.cljc#L244

[ikitommi]

there should be a test that for multiple predicate keys too. Schema explicitly only allows one extra key per map, I think we might not need that restriction.

[jeans11]

What do you mean?

[ikitommi]

My bad, was thinking Plumatic schema, which has the limitation - just one predicate key per map

[jeans11]

Allow something like that?

(m/schema
  [:map
    [keyword? int?]
    [keyword? string?]])

[ikitommi]

Let's not. That can already be defined as:

[:map
  [keyword? [:or int? string?]]

... and this would cause trouble when later doing tools to merge maps and removing keyword? key might or might not remove all keys. Same actually applies to normal keys, this should be illegal:

[:map [:x int?] [:x string?]]

[ikitommi]

Thanks! I'll merge this in and will check the perf & think about corner cases.

[ikitommi]

perf regressed for validation of [:map [:x int?] [:y boolean?]] from 17ns to 110ns (6x). I need this for the generator branch, so will do some perf tuning on top of this.

[ikitommi]

There is actually ambiguity rule here:

Given a schema:

[:map
  [:a int?]
  [:b string?]
  [::id string?]

One could have binded the ::id into a defined value via a registry, so it's a valid schema. The code in this PR doesn't resolve ::id as a schema as the code doesn't pass options into the -valid-schema? function. That's a bug. If that is fixed, the ::id would be resolved into a schema and it's value is used instead ::id as a key => there is no way to represent key ::id in the map, which is not acceptable. Also, few commits ago, I made the IntoSchema work with malli.core/schema too, so:

(m/schema :tuple)
; :tuple

=> :tuple would be illegal key in any map.

Will remove the changes and put the commits aside until resolved. Need to think this over.

[ikitommi]

Master is reset back before this, and these changed are stored in branch https://github.com/metosin/malli/tree/map-of-with-map