Clojure

[spec] Clarify s/every docstring for :kind

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.9
  • Fix Version/s: Release 1.10
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Ok

Description

Docstring for `s/every` in the `:kind` option says "a pred/spec that the collection type must satisfy, e.g. vector?" but using a spec for `:kind` will fail:

user=> (s/valid? (s/every number? :kind (s/or :vector vector? :list list?))
          [])
ClassCastException clojure.spec$or_spec_impl$reify__13891 cannot be cast to clojure.lang.IFn  dev/eval44499/fn--44501 (form-init3178965928127409998.clj:22)

user=> (pst *e)
ClassCastException clojure.spec$or_spec_impl$reify__13903 cannot be cast to clojure.lang.IFn
	user/eval20/fn--22 (NO_SOURCE_FILE:13)
	clojure.spec/every-impl/reify--14039 (spec.clj:1225)
	clojure.spec/valid? (spec.clj:744)
	clojure.spec/valid? (spec.clj:740)

Proposed: The intent here was just to support a predicate function. Change docstring to say just "pred" rather than "pred/spec".

Patch: clj-2111.patch

Activity

Hide
Alex Miller added a comment -

Certainly a function like this works (s/every number? :kind #(or (vector? %) (list? %))). The question is whether the s/every doc that states "pred/spec" means only a predicate function or "predicate function OR spec". I'm not sure what the intention was. Certainly the code in every seems to be wrapping the kind into a function and then invoking it in every-impl, so it's not written to accept a spec currently.

Show
Alex Miller added a comment - Certainly a function like this works (s/every number? :kind #(or (vector? %) (list? %))). The question is whether the s/every doc that states "pred/spec" means only a predicate function or "predicate function OR spec". I'm not sure what the intention was. Certainly the code in every seems to be wrapping the kind into a function and then invoking it in every-impl, so it's not written to accept a spec currently.
Hide
Alex Miller added a comment -

Marking vetted to either resolve, update docstring, or decline. Need more info from Rich.

Show
Alex Miller added a comment - Marking vetted to either resolve, update docstring, or decline. Need more info from Rich.
Hide
Francis Avila added a comment - - edited

Note that this issue can cause s/valid? and s/explain to disagree:

(s/def ::vector vector?)
=> :user/vector
(s/valid? (s/coll-of any? :kind ::vector) [])
=> false
(s/explain (s/coll-of any? :kind ::vector) [])
Success!
=> nil

The s/explain* codepath will interpret keyword :kind as a spec, but the s/coll-of and s/every macros (maybe every-kv and map-of too?) unconditionally create a collection predicate function cpred which assumes :kind is a predicate; then in every-impl s/conform* consults cpred but s/explain* does not and always looks at :kind directly.

Show
Francis Avila added a comment - - edited Note that this issue can cause s/valid? and s/explain to disagree:
(s/def ::vector vector?)
=> :user/vector
(s/valid? (s/coll-of any? :kind ::vector) [])
=> false
(s/explain (s/coll-of any? :kind ::vector) [])
Success!
=> nil
The s/explain* codepath will interpret keyword :kind as a spec, but the s/coll-of and s/every macros (maybe every-kv and map-of too?) unconditionally create a collection predicate function cpred which assumes :kind is a predicate; then in every-impl s/conform* consults cpred but s/explain* does not and always looks at :kind directly.
Hide
daniel sutton added a comment - - edited

I would argue that of resolve, update docstring, and decline, resolve should be the correct choice. The `valid?` is the reference implementation of speccing something. It and `explain` must agree on whether something is valid. It would seem that either `valid?` must be corrected to allow specs there or the docstring changed to only allow predicates and `explain` be modified to agree with `valid?`'s notion of whether something is valid (in this case, disallowing specs for `:kind`).

Show
daniel sutton added a comment - - edited I would argue that of resolve, update docstring, and decline, resolve should be the correct choice. The `valid?` is the reference implementation of speccing something. It and `explain` must agree on whether something is valid. It would seem that either `valid?` must be corrected to allow specs there or the docstring changed to only allow predicates and `explain` be modified to agree with `valid?`'s notion of whether something is valid (in this case, disallowing specs for `:kind`).
Hide
Alex Miller added a comment -

Applied

Show
Alex Miller added a comment - Applied

People

Vote (2)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: