Clojure

case expression fails to match a BigDecimal

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Completed
  • Affects Version/s: Release 1.5, Release 1.6
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Incomplete

Description

In 1.5.1 (anywhere before the CLJ-1118 patch), this is the behavior on BigDecimal case matching:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "BigDecimal" "none")

In 1.6 the behavior (post CLJ-1118 patch) has changed:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "none" "none")

In 1.6 after CLJ-1118, I expect to see: ("Long" "BigDecimal" "BigDecimal") as they now have the same hash and hasheq.

Cause: The case constants are hashed in the clojure.core/case macro using clojure.core/hash which calls clojure.lang.util/hasheq(). In Compiler.emitExprForHashes(), a call to clojure.lang.Util/hash(). In Clojure 1.5 these hash values are the same (hash of 1.0M == hasheq of 1.0M == 311). In Clojure 1.6, they are different (hash of 1.0M = 311, hasheq of 1.0M = 31).

In any cases where Java's hashCode and Clojure's hasheq return different values, the case statement can fail to do the correct thing.

Approach: Change Compiler.java to use clojure.lang.Util hasheq() to match the case macro use of clojure.core/hash (which calls clojure.lang.Util.hasheq()).

Patch: clj-1301-1.diff

Screened by:

  1. case-alt.patch
    24/Jan/14 9:46 AM
    3 kB
    Alex Miller
  2. clj-1301-1.diff
    23/Nov/13 5:00 PM
    2 kB
    Andy Fingerhut

Activity

Hide
Alex Miller added a comment -

Alternative patch in the direction of using hashcode/equals instead of hasheq/equiv. Note that this test causes some test failures. This is not yet a candidate patch - further work needs to be done in evaluating this path.

Show
Alex Miller added a comment - Alternative patch in the direction of using hashcode/equals instead of hasheq/equiv. Note that this test causes some test failures. This is not yet a candidate patch - further work needs to be done in evaluating this path.
Hide
Andy Fingerhut added a comment -

One could consider having the default behavior of case to use hasheq and clojure.core/= everywhere, but add a 'fast' option to use hashCode and Java equals.

Show
Andy Fingerhut added a comment - One could consider having the default behavior of case to use hasheq and clojure.core/= everywhere, but add a 'fast' option to use hashCode and Java equals.
Hide
Alex Miller added a comment -

Re Andy's comments above, I walked down that path a bit and built such a patch, however we currently have tests in clojure.test-clojure.control:

(testing "test number equivalence"
    (is (= :1 (case 1N 1 :1 :else))))

which clearly seems to expect Clojure equiv() behavior over Java equals() behavior in case constant matching. So either that is a bad test or this is not a viable approach (it also suggests we could break existing code with this change).

Show
Alex Miller added a comment - Re Andy's comments above, I walked down that path a bit and built such a patch, however we currently have tests in clojure.test-clojure.control:
(testing "test number equivalence"
    (is (= :1 (case 1N 1 :1 :else))))
which clearly seems to expect Clojure equiv() behavior over Java equals() behavior in case constant matching. So either that is a bad test or this is not a viable approach (it also suggests we could break existing code with this change).
Hide
Alex Miller added a comment -

Simplified examples.

Show
Alex Miller added a comment - Simplified examples.
Hide
Alex Miller added a comment -

Added better example demonstrating the problem (the specific problem exposed by CLJ-1118).

Show
Alex Miller added a comment - Added better example demonstrating the problem (the specific problem exposed by CLJ-1118).
Hide
Alex Miller added a comment -

Based on discussion comments, move back to Incomplete until we resolve.

Show
Alex Miller added a comment - Based on discussion comments, move back to Incomplete until we resolve.
Hide
Andy Fingerhut added a comment -

Alex, unless I am missing something, changing case to use Java's hashCode() would also require changing its current equality comparison from Clojure = (aka equiv()) to something consistent with hashCode(), which I think must be Java's equals().

Such a change would mean that all of the things that are = but not equals() will not match each other in a case statement, e.g. a case value of (Integer. 5) will not match a (Long. 5) value to compare against in a case branch.

Is that really what is desired here? I almost hesitate to create such a patch, for fear it might be committed

Show
Andy Fingerhut added a comment - Alex, unless I am missing something, changing case to use Java's hashCode() would also require changing its current equality comparison from Clojure = (aka equiv()) to something consistent with hashCode(), which I think must be Java's equals(). Such a change would mean that all of the things that are = but not equals() will not match each other in a case statement, e.g. a case value of (Integer. 5) will not match a (Long. 5) value to compare against in a case branch. Is that really what is desired here? I almost hesitate to create such a patch, for fear it might be committed
Hide
Alex Miller added a comment -

And in case you were wondering, the reason is that the Java hashcode is generally faster (case is all about speed) and there are easy opportunities for you to properly cast your expression and/or case constants (where as the situations with collections where boxing is difficult to fix generically, that is not true).

Show
Alex Miller added a comment - And in case you were wondering, the reason is that the Java hashcode is generally faster (case is all about speed) and there are easy opportunities for you to properly cast your expression and/or case constants (where as the situations with collections where boxing is difficult to fix generically, that is not true).
Hide
Alex Miller added a comment -

Andy, I talked to Rich and the conclusion was that we should make the opposite change here such that the case macro should route to the Java hashcode version clojure.lang.util.hash() and the Compiler should be left as is. Can you update the patch?

Show
Alex Miller added a comment - Andy, I talked to Rich and the conclusion was that we should make the opposite change here such that the case macro should route to the Java hashcode version clojure.lang.util.hash() and the Compiler should be left as is. Can you update the patch?
Hide
Alex Miller added a comment -

Putting in 1.6 release per Rich.

Show
Alex Miller added a comment - Putting in 1.6 release per Rich.
Hide
Andy Fingerhut added a comment -

This bug is also the root cause for the recent failures of tests for the test.generative library.

Show
Andy Fingerhut added a comment - This bug is also the root cause for the recent failures of tests for the test.generative library.
Hide
Andy Fingerhut added a comment -

Patch clj-1301-1.diff modifies Compiler.java so that case* statements use hasheq on the test expression value, rather than Java's hashCode. It also adds a test case that currently fails with latest Clojure master, but passes with the patch.

Show
Andy Fingerhut added a comment - Patch clj-1301-1.diff modifies Compiler.java so that case* statements use hasheq on the test expression value, rather than Java's hashCode. It also adds a test case that currently fails with latest Clojure master, but passes with the patch.

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: