spectrum icon indicating copy to clipboard operation
spectrum copied to clipboard

Add special case (ann) for clojure.core/keyword

Open setzer22 opened this issue 8 years ago • 3 comments

clojure.core/keyword takes a string and returns a keyword. However, in order to support certain behaviours the function returns nil whenever it gets something that is not a string, symbol or keyword, instead of crashing.

This is unfortunate for type checking, since you cannot assume keyword will return not-nil even when the type checker can prove that it's being called with a string, forcing you to pollute the codebase with nilable specs.

This can be fixed with the following ann:

(spectrum.ann/ann #'keyword
                  (fn [fnspec argspec]
                    (if (#{#'clojure.core/symbol?
                           #'clojure.core/keyword?
                           #'clojure.core/string?}
                         (-> argspec :ps first :pred))
                      (-> fnspec
                          (update-in [:ret :ps] pop)
                          (update-in [:ret :ks] pop))
                      fnspec)))

Would this be a good addition to ann.clj?

setzer22 avatar Jul 09 '17 13:07 setzer22

Yes, keyword needs an ann, and would make a good addition. If you're interested in making a PR, a few notes:

  • prefer c/first*, c/rest* over :ps. :ps is an implementation detail of concrete spects, while first*, rest* use protocols.
  • in the cases where keyword returns nil, update the fnspec :ret to (c/value nil). c/value is a spect that says, 'I know exactly which value this fn returns'.
  • Make sure you handle both arities of keyword. Look at the spec for keyword in core_specs.clj, in particular, the 1-arity case accepts any? in the first arg, while the 2 arity requires string or nil in the first arg, and string in the second.
  • Add tests in ann_test, the best way is to probably use check/type-of, ie

(= (c/pred-spec #'keyword?) (check/type-of '(keyword x) {:x (c/pred-spec #'string?)}))

Thanks!

arohner avatar Jul 09 '17 16:07 arohner

I'd be glad to contribute! I'll be looking at it this weekend. Thanks for the comments!

setzer22 avatar Jul 11 '17 09:07 setzer22

is this ticket still valid?

spectrum.repl> (f/infer-var #'keyword)
[fn
 {[cat ?t±897] [or [#'clojure.core/keyword? [value nil]]],
  [cat
   [or [#'clojure.core/string? [value nil]]]
   [or [#'clojure.core/string? [value nil]]]]
  [or [#'clojure.core/keyword? [value nil]]]}]

divs1210 avatar Jun 03 '19 18:06 divs1210