Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require data to match coll type #237

Closed
brjann opened this issue Jun 23, 2020 · 10 comments
Closed

Require data to match coll type #237

brjann opened this issue Jun 23, 2020 · 10 comments
Labels

Comments

@brjann
Copy link

brjann commented Jun 23, 2020

It does not seem to be possible to require data to match the type of collection in a data spec.

For example both (s/valid? (ds/spec ::x #{int?}) [1 2 3]) and (s/valid? (ds/spec ::x [int?]) #{1 2 3}) return true. I have been bitten by this where I accidentally passed a list instead of a set to a function, which passed the spec check and since contains? accepts list, no error occured.

Is it possible to require an exact match on collection type?

@ikitommi
Copy link
Member

sounds like a bug. PR welcome.

@ikitommi ikitommi added the bug label Jun 26, 2020
@brjann
Copy link
Author

brjann commented Jun 28, 2020

I wish I had enough Clojure / spec skills to submit a PR on this :-)

@brjann
Copy link
Author

brjann commented Jun 28, 2020

While
(s/valid? (ds/spec ::x {int? int?}) #{1 2}) returns false,
(s/valid? (ds/spec ::x {int? int?}) #{[1 2]}) returns true

So it seems that one must be able to convert the value to its correct form for the spec check to succeed

A call to s/conform seems to coerce the value to it's correct form
(s/conform (ds/spec ::x {int? #{int}}) [[1 [1 2 3]]])
=> {1 #{1 3 2}}

@wandersoncferreira
Copy link
Contributor

@brjann there is some explanation in the mentioned PR above to fix the issue you reported.

@brjann
Copy link
Author

brjann commented Jul 1, 2020

@wandersoncferreira wow, thanks! Does this also make (s/valid? (ds/spec ::x {int? int?}) #{[1 2]}) return false?

@wandersoncferreira
Copy link
Contributor

Unfortunately not @brjann , it seems related but it is not the same error. From some investigation, seems like the code performing the destructuring of #{[1 2]} is treating it the same way as #{1 2}. I am trying to look where this is done.

@wandersoncferreira
Copy link
Contributor

Which is interesting is that (s/valid? (ds/spec ::x {int? int?}) #{[1 2 3]}) returns false as expected. This error is only happening with exact 2 elements inside the array.

@brjann
Copy link
Author

brjann commented Jul 1, 2020

I would guess that ´[k v]´ is identified as a key value pair. (s/valid? (ds/spec ::x {int? int?}) [[1 2] [2 1]]) is also valid, while (s/valid? (ds/spec ::x {int? int?}) [[1 2] [2 1 3]]) is not.
Also, (s/conform (ds/spec ::x {int? int?}) [[1 2] [2 1]]) => {1 2 2 1}

@wandersoncferreira
Copy link
Contributor

wandersoncferreira commented Jul 1, 2020

Yes, I got the whole picture now.

When the map-of-spec is called to create our data spec, we rely on clojure.spec.alpha/every-impl to create it. However, this function has the following logic to perform the dispatch for each value when you ask to conform-all values in the map: https://github.com/clojure/spec.alpha/blob/master/src/main/clojure/clojure/spec/alpha.clj#L1298

And the problem lies in the (seq x) call. x is our data in this case #{[1 2 3 4]} . However, take a look at the destructuring below (simplified version of what the every-impl does):

;; the value of v
(let [x #{[1 2 3]}
      [v & vs :as vseq] (seq x)]
  v) ;; => [1 2 3]

;;; where v is used
(let [x #{[1 2 3]}
      [v & vs :as vseq] (seq x)]
  (conform* spec v))

And what is spec? Looking at impl/map-of-spec, spec is checking for a key-value tuple. And seq just got rid off our #{} literal. Then when the number of itens inside the vector is even, (conform* spec v) returns true and everything is fine.

Now, I'm not sure how can we approach to fix the issue. I looked under the hood of s/map-of which seems to do this just fine, however the implementation to some of the parts, e.g. cpred parameter, is a lot more complex than the current coll? call spec-tools is using right now.

EDIT:

Seems like changing cpred from coll? to map? fix this issue and does not produce any regression problem with the test suite. I will add this new commit to the open PR. (Y)

@brjann
Copy link
Author

brjann commented Jul 7, 2020

Thank you @wandersoncferreira !

@brjann brjann closed this as completed Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants