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

Type warning for contains? on seq #1021

Closed
borkdude opened this issue Sep 28, 2020 · 3 comments
Closed

Type warning for contains? on seq #1021

borkdude opened this issue Sep 28, 2020 · 3 comments
Labels
enhancement New feature or request type warnings
Projects

Comments

@borkdude
Copy link
Member

(contains? (map :name tags) "foo")

should give a type warning.

@borkdude borkdude added enhancement New feature or request type warnings labels Sep 28, 2020
@borkdude borkdude added this to Needs triage in clj-kondo via automation Sep 28, 2020
@borkdude borkdude moved this from Needs triage to High priority (next release) in clj-kondo Sep 28, 2020
@borkdude borkdude moved this from High priority (next release) to In progress in clj-kondo Oct 4, 2020
borkdude added a commit that referenced this issue Oct 4, 2020
@borkdude borkdude moved this from In progress to Next release in clj-kondo Oct 4, 2020
clj-kondo automation moved this from Next release to Done Oct 10, 2020
@yuhan0
Copy link
Contributor

yuhan0 commented Dec 1, 2020

The commit which closed this does not seem right (d3b71fc)

While using contains? on a vector (associative) is valid, it's quite likely to be an error:

(contains? [:a :b] :a)
;; => false
(contains? [:a :b] 0)
;; => true

contains? also does not work on strings:

(contains? "abc" \a)
;; => Execution error (IllegalArgumentException) at repro/eval135854 (REPL:83).
;;    contains? not supported on type: java.lang.String

I believe a better type signature for it would be

{:arities {2 {:args [#{:map :set} :any] 
              :ret boolean}}}

If that sounds alright I can open a PR.

@borkdude
Copy link
Member Author

borkdude commented Dec 1, 2020

Contains is supported on vectors and strings, but the key should be numeric in that case. Remember that contains checks for key values, and strings are considered arrays keyed by their index. As such, I don't see a reason to change the existing type.

https://github.com/clojure/clojure/blob/f47c139e20970ee0852166f48ee2a4626632b86e/src/jvm/clojure/lang/RT.java#L830-L853

@yuhan0
Copy link
Contributor

yuhan0 commented Dec 1, 2020

Thanks for the clarification, I didn't realise it worked on strings depending on the 2nd arg, and I suppose the current type system can't express something like [#{:string :vector} :int] OR [#{:map :set} :any].

Using contains? on a string or array still seems to me like a likely bug or at the least an unidiomatic way of checking the array bounds, but that's a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request type warnings
Projects
clj-kondo
  
Done
Development

No branches or pull requests

2 participants