Clojure

compiler makes different concrete maps then the reader

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.3, Release 1.4, Release 1.5
  • Fix Version/s: Release 1.5, Release 1.6
  • Component/s: None
  • Labels:
    None
  • Environment:
    Clojure 1.3 om Mac OS 10.7, Clojure 1.5.0 alpha1 on Linux x86_64 (OpenJDK 1.7.0 b147)
  • Patch:
    Code and Test
  • Approval:
    Screened

Description

I (Stu) agree with Nicola's assessment in the comments: the single real problem here is that the compiler's MapExpr parser emits maps differently from other map-makers, e.g. RT's factory functions.

Patch clj944-plus-tests does three things:

  • has the compiler emit map types consistent with the reader (like the previous "0002" patch)
  • adds tests
  • removes broken tests

Original report follows:

Hi guys, I am getting the following exception:

(.containsKey {:one 1} :one)
;=> ClassCastException clojure.lang.PersistentArrayMap cannot be cast to clojure.lang.PersistentHashMap

The map is a clojure.lang.PersistentArrayMap, which obviously has a containsKey method (https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/PersistentArrayMap.java#L95).

Casting it works fine though:

(.containsKey ^clojure.lang.PersistentArrayMap {:one 1} :one)
;=> true

The mailing list suggest that the compiler injects an incorrect cast to clojure.lang.PersistentHashMap. In this case it should probably be cast to a clojure.lang.Associative, the highest common interface having the .containsKey method.

The problem is not present in Clojure 1.2.1.

Attachments

  1. Text File
    0001-Fix-for-CLJ-944.patch
    30/Oct/12 5:02 PM
    0.9 kB
    Nicola Mometto
  2. Text File
    0002-Fix-for-CLJ-944.patch
    01/Nov/12 3:48 PM
    1 kB
    Nicola Mometto
  3. Text File
    clj944-plus-tests.patch
    12/Apr/13 3:57 PM
    5 kB
    Stuart Halloway

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Nicola Mometto added a comment -

The attached patch fixes the issue, by emitting IPersistentMap instead of Persistent{Hash|Array}Map as class type for maps literals

Show
Nicola Mometto added a comment - The attached patch fixes the issue, by emitting IPersistentMap instead of Persistent{Hash|Array}Map as class type for maps literals
Hide
Permalink
Nicola Mometto added a comment - - edited

I uploaded another patch fixing the same problem in a different way.
While 0001-Fix-for-CLJ-944.patch makes clojure.lang.Complier.ConstantExpr#getJavaClass return clojure.lang.IPersistentMap for both clojure.lang.PersistentHashMap and clojure.lang.PersistentArrayMap, 0002-Fix-for-CLJ-944.patch makes clojure.lang.Compiler.MapExpr#parse return a PersistentArrayMap if the length is <= HASHTABLE_THRESHOLD, instead of always returning a PersistentHashMap.

This approach is more consistent, making the type of the compiler's internal representation of a map literal equal to the one of the reader.

Note that this second approach while being more consistent, breaks some tests that assume some operations on maps (specifically `seq` and `print`) to be order dependent, and written with the hash-map return order implementation in mind.

That should not be the case and if the second patch is preferred over the first one, I'll gladly fix those tests.

Show
Nicola Mometto added a comment - - edited I uploaded another patch fixing the same problem in a different way. While 0001-Fix-for-CLJ-944.patch makes clojure.lang.Complier.ConstantExpr#getJavaClass return clojure.lang.IPersistentMap for both clojure.lang.PersistentHashMap and clojure.lang.PersistentArrayMap, 0002-Fix-for-CLJ-944.patch makes clojure.lang.Compiler.MapExpr#parse return a PersistentArrayMap if the length is <= HASHTABLE_THRESHOLD, instead of always returning a PersistentHashMap. This approach is more consistent, making the type of the compiler's internal representation of a map literal equal to the one of the reader. Note that this second approach while being more consistent, breaks some tests that assume some operations on maps (specifically `seq` and `print`) to be order dependent, and written with the hash-map return order implementation in mind. That should not be the case and if the second patch is preferred over the first one, I'll gladly fix those tests.
Hide
Permalink
Stuart Halloway added a comment -

Approach #2, relying on consistent choice of concrete map class by size throughout, feels quite fragile.

Approach #1 seems to abuse the method name getJavaClass(), now having it return "get the base type I would need for cast".

Maybe there needs to be a different thing entirely?

Show
Stuart Halloway added a comment - Approach #2, relying on consistent choice of concrete map class by size throughout, feels quite fragile. Approach #1 seems to abuse the method name getJavaClass(), now having it return "get the base type I would need for cast". Maybe there needs to be a different thing entirely?
Hide
Permalink
Nicola Mometto added a comment - - edited

Patch #2 should get merged (IMHO) regardless of the fragility of its approach to fixing this ticket's bug, since it fixes another bug:

prior to the patch:

user=> (class {:a 1})
clojure.lang.PersistentArrayMap
user=> (def a {:a 1})
#'user/a
user=> (class a)
clojure.lang.PersistentHashMap

after the patch:

user=> (class {:a 1})
clojure.lang.PersistentArrayMap
user=> (def a {:a 1})
#'user/a
user=> (class a)
clojure.lang.PersistentArrayMap

This should also lead to some minor performance enhancement since prior to this moment, every map def'ed would be a HashMap instead of an ArrayMap

So, I think patch #2 should be applied if not for this ticket's bug, at least for the reason stated above.
If somebody has any proposal for making this patch more solid regarding this ticket's bug, any help is welcome

Show
Nicola Mometto added a comment - - edited Patch #2 should get merged (IMHO) regardless of the fragility of its approach to fixing this ticket's bug, since it fixes another bug: prior to the patch: user=> (class {:a 1}) clojure.lang.PersistentArrayMap user=> (def a {:a 1}) #'user/a user=> (class a) clojure.lang.PersistentHashMap after the patch: user=> (class {:a 1}) clojure.lang.PersistentArrayMap user=> (def a {:a 1}) #'user/a user=> (class a) clojure.lang.PersistentArrayMap This should also lead to some minor performance enhancement since prior to this moment, every map def'ed would be a HashMap instead of an ArrayMap So, I think patch #2 should be applied if not for this ticket's bug, at least for the reason stated above. If somebody has any proposal for making this patch more solid regarding this ticket's bug, any help is welcome
Hide
Permalink
Rich Hickey added a comment -

This should not have passed screening. There are two issues, should be separate. I have no idea what has been screened nor what will be applied should it be approved. There's contention in the discussion but no resolution.

Show
Rich Hickey added a comment - This should not have passed screening. There are two issues, should be separate. I have no idea what has been screened nor what will be applied should it be approved. There's contention in the discussion but no resolution.
Hide
Permalink
Nicola Mometto added a comment -

I don't think that there are two issues here.
The issue is only one: the compiler doesn't emit maps in a way consistent with what the reader returns and with how the compiler itself uses maps.
The symptoms are two: some interop calls fail, and def'ed vars with a literal map as value never use a PersistentArrayMap.

The underlying cause of those two symtoms is fixed by the patch 002 that i submitted (incorporated in Stu's clj944-plus-tests patch.

Stuart said that this approach feels fragile but the bug is caused by the fact that everywhere else clojure returns a PersistentArrayMap when the element count is <= than the PersistentHashMap threshold, and when emitting maps, it doesn't.

Making clojure emit maps consistently with how clojure does internally everywhere else looks to me like the only solution, and I don't really see how making clojure consistent is a fragile approach.

But again, if somebody can suggest a better solution to this problem, I'll gladly submit another patch.

Show
Nicola Mometto added a comment - I don't think that there are two issues here. The issue is only one: the compiler doesn't emit maps in a way consistent with what the reader returns and with how the compiler itself uses maps. The symptoms are two: some interop calls fail, and def'ed vars with a literal map as value never use a PersistentArrayMap. The underlying cause of those two symtoms is fixed by the patch 002 that i submitted (incorporated in Stu's clj944-plus-tests patch. Stuart said that this approach feels fragile but the bug is caused by the fact that everywhere else clojure returns a PersistentArrayMap when the element count is <= than the PersistentHashMap threshold, and when emitting maps, it doesn't. Making clojure emit maps consistently with how clojure does internally everywhere else looks to me like the only solution, and I don't really see how making clojure consistent is a fragile approach. But again, if somebody can suggest a better solution to this problem, I'll gladly submit another patch.

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.