Clojure

check that argument to keys/vals is a Map

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Backlog
  • Fix Version/s: Backlog
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Current behavior:

  • If you call keys or vals on something that is not a Map, you do not get a ClassCastException until the KeySeq or ValSeq is consumed
  • Calling keys or vals on an empty collection of any type, even non-Map types, returns nil

The attached patch:

  • checks the type of the argument to keys and vals and throws IllegalArgumentException if it is neither java.util.Map nor null
  • changes tests for keys and vals to check that those functions throw IllegalArgumentException on empty collections that are not maps

Activity

Hide
Rich Hickey added a comment -

Please don't test for specific exception types - thanks

Show
Rich Hickey added a comment - Please don't test for specific exception types - thanks
Hide
Stuart Sierra added a comment -

keys-vals-type-2.patch replaces previous. Tests check for Exception instead of IllegalArgumentException

Show
Stuart Sierra added a comment - keys-vals-type-2.patch replaces previous. Tests check for Exception instead of IllegalArgumentException
Hide
OHTA Shogo added a comment -

Is there any reason why this patch hasn't been applied yet?
Recently I suffered from the ClassCastException raised by something like (keys [[:a 0]]), which caused my CIDER to miss stack traces somehow and made debug harder.

Show
OHTA Shogo added a comment - Is there any reason why this patch hasn't been applied yet? Recently I suffered from the ClassCastException raised by something like (keys [[:a 0]]), which caused my CIDER to miss stack traces somehow and made debug harder.
Hide
Andy Fingerhut added a comment -

It was committed here: https://github.com/clojure/clojure/commit/13d9404b5227f3b9e8f86371d211be890e5302a9

later it was reverted with this commit: https://github.com/clojure/clojure/commit/d93ef6293bbcc6059e6da77dbf6ed26167f36cdb
The comment on that commit was 'breaks subset'. I don't know the details of how it breaks subseq.

Show
Andy Fingerhut added a comment - It was committed here: https://github.com/clojure/clojure/commit/13d9404b5227f3b9e8f86371d211be890e5302a9 later it was reverted with this commit: https://github.com/clojure/clojure/commit/d93ef6293bbcc6059e6da77dbf6ed26167f36cdb The comment on that commit was 'breaks subset'. I don't know the details of how it breaks subseq.
Hide
Andy Fingerhut added a comment -

'breaks subset' in my previous comment should have been 'breaks subseq'. Perhaps JIRA only lets one edit their comments for tickets that are still open.

Show
Andy Fingerhut added a comment - 'breaks subset' in my previous comment should have been 'breaks subseq'. Perhaps JIRA only lets one edit their comments for tickets that are still open.
Hide
Andy Fingerhut added a comment -

I don't know if dynalint [1] includes a check for this, but you might consider suggesting the addition of such a run-time check for that project.

There is also Dire [2] which may help add dynamic run-time checks to functions in clojure.core (I haven't used it for that before).

[1] https://github.com/frenchy64/dynalint
[2] https://github.com/MichaelDrogalis/dire

Show
Andy Fingerhut added a comment - I don't know if dynalint [1] includes a check for this, but you might consider suggesting the addition of such a run-time check for that project. There is also Dire [2] which may help add dynamic run-time checks to functions in clojure.core (I haven't used it for that before). [1] https://github.com/frenchy64/dynalint [2] https://github.com/MichaelDrogalis/dire
Hide
Alex Miller added a comment -

Subseq and rsubseq are kind of special cases where iirc they work with seqs of mapentries instead of a map. I ran into this recently with CLJ-1602 and added some tests there for subseq.

Show
Alex Miller added a comment - Subseq and rsubseq are kind of special cases where iirc they work with seqs of mapentries instead of a map. I ran into this recently with CLJ-1602 and added some tests there for subseq.
Hide
Andy Fingerhut added a comment -

Temporarily reopening ticket to add a 'checkargs' label to it. Will return to original state (except for the new label) soon.

Show
Andy Fingerhut added a comment - Temporarily reopening ticket to add a 'checkargs' label to it. Will return to original state (except for the new label) soon.
Hide
OHTA Shogo added a comment -

Thank you for comments.
I didn't know keys/vals should work with seqs of mapentries (it doesn't seem their docstrings are saying so either).

Show
OHTA Shogo added a comment - Thank you for comments. I didn't know keys/vals should work with seqs of mapentries (it doesn't seem their docstrings are saying so either).

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: