Clojure

[spec] Nesting cat inside ? causes unform to return nested result

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Critical Critical
  • Resolution: Unresolved
  • Affects Version/s: Release 1.9
  • Fix Version/s: Release 1.11
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

Calling conform and then unform with a spec that consists of some cat nested inside of some ? creates an extra level of nesting in the result:

(require '[clojure.spec :as s])

(let [spec (s/? (s/cat :foo #{:foo}))
      initial [:foo]
      conformed (s/conform spec initial)
      unformed (s/unform spec conformed)]
  [initial conformed unformed])
;;=> [[:foo] {:foo :foo} [(:foo)]]

This behavior does not occur with just ? or cat alone:

(let [spec (s/? #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> [:foo]

(let [spec (s/cat :foo #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> (:foo)

Patch: CLJ-2003-corrected.patch

  1. CLJ-2003.patch
    09/Aug/17 9:41 PM
    1 kB
    Tyler Tallman
  2. CLJ-2003-corrected.patch
    31/Jul/18 1:12 PM
    3 kB
    Francis Avila

Activity

Hide
Phil Brown added a comment - - edited

I came across another case of extra nesting, when repeating one or more sequences with an optional element at the beginning or end, where that element's predicate also matches the element at the other end:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} [{:k :b, :v 2}]]

where I expected

[{:k :a, :v 1} {:k :b, :v 2}]

The following give expected results:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b])
[{:k :a, :v 1} {:k :b}]
user=> (s/conform (s/+ (s/cat :k keyword? :v (s/? int?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
user=> (s/conform (s/* (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
Show
Phil Brown added a comment - - edited I came across another case of extra nesting, when repeating one or more sequences with an optional element at the beginning or end, where that element's predicate also matches the element at the other end:
user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} [{:k :b, :v 2}]]
where I expected
[{:k :a, :v 1} {:k :b, :v 2}]
The following give expected results:
user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b])
[{:k :a, :v 1} {:k :b}]
user=> (s/conform (s/+ (s/cat :k keyword? :v (s/? int?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
user=> (s/conform (s/* (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
Hide
Alex Miller added a comment -

Phil, I think your example is a different issue and you should file a new jira for that.

Show
Alex Miller added a comment - Phil, I think your example is a different issue and you should file a new jira for that.
Hide
Alex Miller added a comment -

Well, maybe I take that back, they may be related.

Show
Alex Miller added a comment - Well, maybe I take that back, they may be related.
Hide
Brandon Bloom added a comment -

I just ran in to this trying to make sense of some defn forms. Here's an example:

user=> (s/unform :clojure.core.specs/defn-args (s/conform :clojure.core.specs/defn-args '(f [& xs])))
(f ((& xs)))

Show
Brandon Bloom added a comment - I just ran in to this trying to make sense of some defn forms. Here's an example: user=> (s/unform :clojure.core.specs/defn-args (s/conform :clojure.core.specs/defn-args '(f [& xs]))) (f ((& xs)))
Hide
Tyler Tallman added a comment -

This seems to be all that is needed.

Show
Tyler Tallman added a comment - This seems to be all that is needed.
Hide
Francis Avila added a comment -

The problem is actually more universal than (? (cat ...)). s/unform of a s/? with any regex child op will introduce an extra level of nesting. When the child is a regex, we are consuming the same "level" of sequence so unform should not introduce an extra level. However in other cases (non-regex ops), we should still possibly produce a nested collection.

The previous patch was too aggressive: it unwrapped all sub-unforms of s/?. This patch CLJ-2003-corrected.patch only unwraps when the sub-op is a regex.

Unfortunately it is impossible to distinguish between a desired-but-optional nil and a non-match from s/?. Specifically, the following tests now hold:

(testing "s/? matching nil"
  (is (nil? (s/conform (s/? nil?) [nil])))
  (is (nil? (s/conform (s/? nil?) [])))
  (is (nil? (s/conform (s/? nil?) nil)))
  (is (= (s/unform (s/? nil?) nil) [])))

(I did not add these tests to the patch because I was unsure if they should be part of the contract of unform. However, they are pretty big gotchas.)

I also added tests for every possible subop of s/?, except ::s/accept, which I could not think of a test case for. (I'm not sure ::s/accept is actually reachable inside s/op-unform?)

Show
Francis Avila added a comment - The problem is actually more universal than (? (cat ...)). s/unform of a s/? with any regex child op will introduce an extra level of nesting. When the child is a regex, we are consuming the same "level" of sequence so unform should not introduce an extra level. However in other cases (non-regex ops), we should still possibly produce a nested collection. The previous patch was too aggressive: it unwrapped all sub-unforms of s/?. This patch CLJ-2003-corrected.patch only unwraps when the sub-op is a regex. Unfortunately it is impossible to distinguish between a desired-but-optional nil and a non-match from s/?. Specifically, the following tests now hold:
(testing "s/? matching nil"
  (is (nil? (s/conform (s/? nil?) [nil])))
  (is (nil? (s/conform (s/? nil?) [])))
  (is (nil? (s/conform (s/? nil?) nil)))
  (is (= (s/unform (s/? nil?) nil) [])))
(I did not add these tests to the patch because I was unsure if they should be part of the contract of unform. However, they are pretty big gotchas.) I also added tests for every possible subop of s/?, except ::s/accept, which I could not think of a test case for. (I'm not sure ::s/accept is actually reachable inside s/op-unform?)
Hide
Alex Miller added a comment -

Thanks for working on this - I will take a look when I get a chance.

Show
Alex Miller added a comment - Thanks for working on this - I will take a look when I get a chance.
Hide
Francis Avila added a comment -

I had to amend my patch slightly (same name): one of the test cases wasn't testing the correct thing.

Show
Francis Avila added a comment - I had to amend my patch slightly (same name): one of the test cases wasn't testing the correct thing.
Hide
Francis Avila added a comment -

Patch was no longer applying cleanly to master because of tests added by other commits. Patch rebased to master

Show
Francis Avila added a comment - Patch was no longer applying cleanly to master because of tests added by other commits. Patch rebased to master

People

Vote (7)
Watch (8)

Dates

  • Created:
    Updated: