keyword function returns nil on bad input

Question

The keyword function should throw an exception on bad input rather than return nil.

user=> (keyword 5) nil user=> (keyword []) nil

Cause: The keyword function is defined as a cond with cases for keywords, symbols, and strings. There is no :else so all other cases return nil.

Proposal: Add an :else branch and throw an exception in keyword.

Patch:

Answer 1

Comment made by: ericnormand

The keyword function should throw an IllegalArgumentException on wrong argument type rather than return nil. For consistency, the two-argument case should throw an IllegalArgumentException if not both arguments are strings.

The find-keyword function should behave similarly to maintain the same signature.

Current behavior:

user=> (keyword 5)
nil
user=> (keyword (link: ))
nil
user=> (keyword 1 1)
java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.String

Cause: The keyword function is defined as a cond with cases for keywords, symbols, and strings. There is no :else so all other cases return nil. The same goes for find-keyword, which should behave in the same way. The two-argument case does throw an exception, but the message is not very helpful.

Proposal: I have added an :else branch to the cond that throws an IllegalArgumentException with a message that indicates the acceptable types and prints the actual argument. I made the same change to find-keyword. There are also simple tests.

Patch: keyword-1341-2014-02-12.patch

Note: This change does not check for all bad input, just the type. For instance, it is still possible to pass in a string with "illegal" keyword characters.

Answer 2

Comment made by: alexmiller

Hey Eric, thanks for the patch! The 1 arg change looks good.

On the 2 arg change I have a concern - I'm worried that we are adding new checks into a pretty hot code path (keyword creation). The 2 arg path is not a silent failure as you'll get a ClassCastException so I do not think adding these checks here is worth it. In the 1-arg case you've already fallen through the else, so there's no additional cost.

Answer 3

Comment made by: ericnormand

Understood. I'll remove the two-argument case.

Answer 4

Comment made by: ericnormand

The keyword function should throw an IllegalArgumentException on wrong argument type rather than return nil. The two-argument case already throws an exception.

The find-keyword function should behave similarly to maintain the same signature.

Current behavior:

user=> (keyword 5)
nil
user=> (keyword (link: ))
nil
user=> (keyword 1 1)
java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.String

Cause: The keyword function is defined as a cond with cases for keywords, symbols, and strings. There is no :else so all other cases return nil. The same goes for find-keyword, which should behave in the same way.

Proposal: I have added an :else branch to the cond that throws an IllegalArgumentException with a message that indicates the acceptable types and prints the actual argument. I made the same change to find-keyword. There are also simple tests.

Alternatives: Adding checks for the two-argument case was considered but it was feared that adding the extra overhead was not worth it since it already threw an exception. No significant overhead is added in the single-argument case since it will only affect erroneous input.

Patch: keyword-1341-2014-02-12.2.patch

Note: This change does not check for all bad input, just the type. For instance, it is still possible to pass in a string with "illegal" keyword characters.

Answer 5

Reference: https://clojure.atlassian.net/browse/CLJ-1341 (reported by alexmiller)