<< Back to previous view

[CLJ-720] check that argument to keys/vals is a Map Created: 18/Jan/11  Updated: 24/Feb/15  Resolved: 23/Feb/15

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Backlog
Fix Version/s: Backlog

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: checkargs

Attachments: Text File keys-vals-type-1.patch     Text File keys-vals-type-2.patch    
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


 Comments   
Comment by Rich Hickey [ 20/Jan/11 7:50 AM ]

Please don't test for specific exception types - thanks

Comment by Stuart Sierra [ 20/Jan/11 7:59 AM ]

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

Comment by OHTA Shogo [ 20/Feb/15 7:37 PM ]

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.

Comment by Andy Fingerhut [ 20/Feb/15 9:48 PM ]

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.

Comment by Andy Fingerhut [ 20/Feb/15 9:50 PM ]

'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.

Comment by Andy Fingerhut [ 20/Feb/15 9:52 PM ]

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

Comment by Alex Miller [ 20/Feb/15 9:54 PM ]

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.

Comment by Andy Fingerhut [ 23/Feb/15 5:53 PM ]

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

Comment by OHTA Shogo [ 24/Feb/15 7:32 PM ]

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).





[CLJ-17] GC Issue 13: validate in (keyword s) and (symbol s) Created: 17/Jun/09  Updated: 24/Feb/15  Resolved: 24/Feb/15

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Backlog

Type: Enhancement Priority: Blocker
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: checkargs


 Description   
Reported by richhickey, Dec 17, 2008
Make sure they create readable keywords/symbols

Comment 1 by p...@hagelb.org, Apr 27, 2009
Could this be done with a regex, or should it try to confirm the name using the reader?
Comment 2 by p...@hagelb.org, Apr 27, 2009
I've implemented this in the attached patch. One thing that could be improved is that
invalid names simply raise an Exception, though it seems LispReader's ReaderException
would be more appropriate. I wasn't sure how to raise that though since it needs a
line number; it's not clear how to get the current line number.

The patch is still an improvement on the current state of things, though I'd
appreciate a tip as to how to raise the right exception.

I've also attached a patch to the test suite that ensures (symbol s) and (keyword s)
work properly in the context of invalid names. I can re-submit this to the contrib
project if that's desired if the core patch is accepted.
 0001-Test-invalid-symbol-keyword-names-raise-exceptions.patch
1.6 KB Download
Comment 3 by p...@hagelb.org, Apr 27, 2009
Last patch had a problem; used things like defn- etc. before they were defined in
core.clj. This attachment fixes that.
 validate-symbol-keyword-names.patch
2.1 KB Download
Comment 4 by p...@hagelb.org, Jun 13 (3 days ago)
This exists as a git branch too now: 
http://github.com/technomancy/clojure/tree/validate-symbols-issue-13


 Comments   
Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/17
Attachments:
0001-Test-invalid-symbol-keyword-names-raise-exceptions.patch - https://www.assembla.com/spaces/clojure/documents/cpC-Qow3ar3P8LeJe5afGb/download/cpC-Qow3ar3P8LeJe5afGb
validate-symbol-keyword-names.patch - https://www.assembla.com/spaces/clojure/documents/cpDbtWw3ar3P8LeJe5afGb/download/cpDbtWw3ar3P8LeJe5afGb
17-validate-keywords-symbols.diff - https://www.assembla.com/spaces/clojure/documents/d61sHuVFer3OGKeJe5afGb/download/d61sHuVFer3OGKeJe5afGb

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

cemerick said: [file:cpC-Qow3ar3P8LeJe5afGb]

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

cemerick said: [file:cpDbtWw3ar3P8LeJe5afGb]

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

technomancy said: These patches no longer cleanly apply. Working on an updated version.

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

technomancy said: [file:d61sHuVFer3OGKeJe5afGb]: test and implementation

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

richhickey said: I just wonder if we can afford the overhead of this validation all the time

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

richhickey said: Anyone have any bright ideas on how to avoid the overhead?

Comment by Assembla Importer [ 28/Sep/10 6:50 AM ]

cemerick said: I'm sure this was brought up a long time ago in prior discussions, but: should validity of symbols and keywords even be defined? Insofar as libraries use them for naming things, especially as read from external representations (e.g. XML, RDF, SGML, etc), it seems like any validation would be an entirely artificial limitation.

For my money, having keywords and symbols print in a failsafe-readable way, e.g. #|symbol with whitespace| (maybe checking at print-time to see if the more common printing is suitable, as would be most common) would:

  • guarantee that symbols and keywords are readably printable
  • not limit libraries from using keywords and such in very convenient ways
  • provide a pleasant shortcut for using symbols and keywords that might not be "valid", but still somehow necessary
Comment by Rich Hickey [ 07/Oct/11 8:03 AM ]

Runtime validation off the table for perf reasons. cemerick's suggestion that arbitrary symbol support will render them valid is sound, but arbitrary symbol support is a different ticket/idea.

Comment by Andy Fingerhut [ 24/Feb/15 12:03 PM ]

Temporarily reopening to add a label. Will return to original state shortly.





Generated at Sun Mar 01 19:10:21 CST 2015 using JIRA 4.4#649-r158309.