Clojure

keyword function returns nil on bad input

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test

Description

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:

Activity

Hide
Eric Normand added a comment -

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

Show
Eric Normand added a comment - 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 []) 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.
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Eric Normand added a comment -

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

Show
Eric Normand added a comment - Understood. I'll remove the two-argument case.
Hide
Eric Normand added a comment -

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

Show
Eric Normand added a comment - 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 []) 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.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated: