core.match

Matches against maps are treated as wildcards, even though they are not

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

The following tests fail:

(deftest map-pattern-match-bind-2
  (is (= (let [xqq {:cz 1 :dz 2}]
           (match [xqq]
             [{:z a :zz b}] [:a0 a b]
             [{:cz a :dz b}] [:a2 a b]
             :else []))
        [:a2 1 2])))

(deftest map-pattern-match-bind-3
  (is (= (let [xmm {:bz 2}]
           (match [xmm]
             [{:az a}] [:a0 a]
             [{:bz b}] [:a1 b]
             :else []))
        [:a1 2])))

Activity

Hide
David Pollak added a comment -

Awesome. Thanks! Please let me know when there are new JAR files in clojars.

Show
David Pollak added a comment - Awesome. Thanks! Please let me know when there are new JAR files in clojars.
Hide
David Nolen added a comment -

This should be fixed in master as of this commit http://github.com/clojure/core.match/commit/a07c2e9620df5b9d331bd6c380d47c15bf7cd60d

Let me know if it works for you.

Show
David Nolen added a comment - This should be fixed in master as of this commit http://github.com/clojure/core.match/commit/a07c2e9620df5b9d331bd6c380d47c15bf7cd60d Let me know if it works for you.
Hide
David Nolen added a comment - - edited

The issue is a bit trickier than it seems. core.match is written in a way such that we always try to share tests across patterns as much as possible. For example consider what happens with map patterns:

(match [x]
  [{:a _ :b 1}] ...
  [{:b _ :c 2}] ...)

In order to do optimal test sharing we expand the above into the following more or less (which is wrong):

:a :b :c
[_  1  _]
[_  _  2]

The problem here is that only the last element in the first row and the first element of the second row are true wildcards. Currently as you discovered the analysis doesn't consider the fact that the other wildcards really must be tested. So really we want something like the following:

:a    :b    :c
[V(_)  1     _]
[_     V(_)  2]

Here you can see we wrap the wildcard in a new pattern type - MapValuePattern. This will prevent the bad analysis.

I hope this makes sense - I've started implementing this and I don't think it will take me too long.

Just to make it a bit more clear imagine the following:

(match [x]
  [{:a _ :b _}] ...
  [{:b _ :c _}] ...)
:a    :b    :c
[V(_)  V(_)  _   ]
[_     V(_)  V(_)]

The wrapping now prevents the first row from being considered a row of wildcards.

Show
David Nolen added a comment - - edited The issue is a bit trickier than it seems. core.match is written in a way such that we always try to share tests across patterns as much as possible. For example consider what happens with map patterns:
(match [x]
  [{:a _ :b 1}] ...
  [{:b _ :c 2}] ...)
In order to do optimal test sharing we expand the above into the following more or less (which is wrong):
:a :b :c
[_  1  _]
[_  _  2]
The problem here is that only the last element in the first row and the first element of the second row are true wildcards. Currently as you discovered the analysis doesn't consider the fact that the other wildcards really must be tested. So really we want something like the following:
:a    :b    :c
[V(_)  1     _]
[_     V(_)  2]
Here you can see we wrap the wildcard in a new pattern type - MapValuePattern. This will prevent the bad analysis. I hope this makes sense - I've started implementing this and I don't think it will take me too long. Just to make it a bit more clear imagine the following:
(match [x]
  [{:a _ :b _}] ...
  [{:b _ :c _}] ...)
:a    :b    :c
[V(_)  V(_)  _   ]
[_     V(_)  V(_)]
The wrapping now prevents the first row from being considered a row of wildcards.
Hide
David Pollak added a comment -

Thanks David. I was thinking about putting a "is this really a wildcard" flag on Wildcard and set it to false for Wildcards in MapPattern.

Anyway... if you want me to keep working on the issue, I'll be glad to. If my clumsy code and clumsy approach doesn't help... I'm down for sitting on the sidelines and watching you do the work.

Show
David Pollak added a comment - Thanks David. I was thinking about putting a "is this really a wildcard" flag on Wildcard and set it to false for Wildcards in MapPattern. Anyway... if you want me to keep working on the issue, I'll be glad to. If my clumsy code and clumsy approach doesn't help... I'm down for sitting on the sidelines and watching you do the work.
Hide
David Nolen added a comment -

Thanks for the patch but this won't work - this defeats sharing tests. I'm looking into it.

Show
David Nolen added a comment - Thanks for the patch but this won't work - this defeats sharing tests. I'm looking into it.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: