<< Back to previous view

[CLJS-639] Produce errors when records are initialized incorrectly Created: 27/Oct/13  Updated: 19/Nov/13  Resolved: 19/Nov/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Scott Feeney Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord, errormsgs


 Description   

There are a couple of wrong ways you can use a record initializer that don't produce errors in ClojureScript, just nils:

ClojureScript:cljs.user> (defrecord Thing [foo bar])
cljs.user/Thing
ClojureScript:cljs.user> (Thing. 1 2)  ; correct usage
#cljs.user.Thing{:foo 1, :bar 2}
ClojureScript:cljs.user> (Thing. 1)    ; wrong number of fields
#cljs.user.Thing{:foo 1, :bar nil}
ClojureScript:cljs.user> (Thing 1 2)   ; forgetting the dot
nil

Compare Clojure:

user=> (defrecord Thing [foo bar])
user.Thing
user=> (Thing. 1 2)
#user.Thing{:foo 1, :bar 2}
user=> (Thing. 1)

CompilerException java.lang.IllegalArgumentException: No matching ctor found for class user.Thing, compiling:(/tmp/form-init7089728177345913731.clj:1:1) 
user=> (Thing 1 2)

RuntimeException Expecting var, but Thing is mapped to class user.Thing  clojure.lang.Util.runtimeException (Util.java:219)

It would make debugging easier if ClojureScript followed Clojure's example here and gave a useful error immediately in case of the last 2 examples.



 Comments   
Comment by David Nolen [ 29/Oct/13 6:40 PM ]

Thanks for the report, these warnings would indeed be nice.

Comment by David Nolen [ 19/Nov/13 4:41 PM ]

fixed, http://github.com/clojure/clojurescript/commit/8c6dd2468a3913e316d021fc0b09745bd3ac7dcd

Comment by David Nolen [ 19/Nov/13 4:43 PM ]

Wrong arity constructor issue fixed. Separate ticket need for invoking ClojureScript ctors as functions.





[CLJ-1556] Add instance check functions to defrecord/deftype Created: 09/Oct/14  Updated: 09/Oct/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord, deftype

Attachments: Text File 0001-CLJ-1556-Generate-type-functions-with-instance-check.patch    
Patch: Code

 Description   

It is often necessarty to test for instance? on deftypes/defrecords, this patch makes the two macros automatically generate a type? function implemented as (fn [x] (instance? type x)), to complement ->type and map->type
Example:

user=>(deftype x [])
user.x
user=>(x? (x.))
true


 Comments   
Comment by Jozef Wagner [ 09/Oct/14 9:11 AM ]

What about camel cased types? predicate SomeType? does not look like an idiomatic type predicate. I suggest to have this type predicate function and its name optional, through e.g. :predicate metadata on a type name. Moreover, it is far more useful to have such predicate on protocols, rather than types.

Comment by Nicola Mometto [ 09/Oct/14 9:17 AM ]

I don't think camel cased types should pose any issue. we use ->SomeType just as fine, I don't see why SomeType? should be problematic.

I disagree that it's more useful to have a predicate for protocols since protocols are already regular Vars and it's just a matter of (satisfies? theprotocol x), the value of the predicate on types/record is to minimize the necessity of having to import the actual class





[CLJ-1495] Defining a record with defrecord twice breaks record equality Created: 07/Aug/14  Updated: 07/Aug/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6, Release 1.7
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Matt Halverson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

If I spin up a fresh repl and type the following four lines, I consistently get this unexpected behavior. I discovered it because it was breaking a unit test.

user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true
true
user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true also -- but it doesn't!
false
user>

This may be related to http://dev.clojure.org/jira/browse/CLJ-1457.

You may also find the following interesting (posted by a fellow irc chatter, reproducible on my machine):

user=> (defrecord Foo [a])
user.Foo
user=> #user.Foo[1]
#user.Foo{:a 1}
user=> (defrecord Foo [b])
user.Foo
user=> (Foo. 1)
#user.Foo{:a 1}





[CLJ-1459] records should support transient Created: 05/Jul/14  Updated: 06/Jul/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

user=> (defrecord R [a])
user.R
user=> (transient (->R nil))
ClassCastException user.R cannot be cast to clojure.lang.IEditableCollection clojure.core/transient (core.clj:3060)






[CLJ-1455] Postcondition in defrecord: Compiler unable to resolve symbol % Created: 28/Jun/14  Updated: 29/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord


 Description   

Clojure's postconditions[1] are a splendiferous, notationally
idiot-proof way to scrutinize a function's return value without
inadvertently causing it to return something else.

Functions (implementing protocols) for a record type may be defined in
its defrecord or with extend-type. In functions defined in
extend-type, postconditions work as expected. Therefore, it is a
surprise that functions defined in defrecord cannot use
postconditions.

Actually it appears defrecord sees a pre/postcondition map as ordinary
code, so the postcondition runs at the beginning of the function (not
the end) and the symbol % (for return value) is not bound.

The code below shows a protocol and two record types that implement
it. Type "One" has an in-the-defrecord function definition where the
postcondition does not compile. Type "Two" uses extend-type and the
postcondition works as expected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(defprotocol ITimesThree
  (x3 [a]))

;; defrecord with functions inside cannot use postconditions.
(defrecord One
    []
  ITimesThree
  (x3 [a]
    {:pre [(do (println "One x3 pre") 1)] ;; (works fine)
     :post [(do (println "One x3 post, %=" %) 1)]
     ;; Unable to resolve symbol: % in this context.
     ;; With % removed, it compiles but runs at start, not end.
     }
    (* 1 3)))

;; extend-type can add functions with postconditions to a record.
(defrecord Two
    [])
(extend-type Two
  ITimesThree
  (x3 [a]
    {:pre [(do (println "Two x3 pre") 1)] ;; (works fine)
     :post [(do (println "Two x3 post, %=" %) 1)] ;; (works fine)
     }
    (* 2 3)))

(defn -main
  "Main"
  []
  (println (x3 (->One)))
  (println (x3 (->Two))))

[1] http://clojure.org/special_forms, in the fn section.






[CLJ-1413] A problem involving user.clj and defrecord Created: 29/Apr/14  Updated: 28/Oct/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Lars Bohl Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: defrecord
Environment:

Java 1.7.0_51 OpenJDK 64-Bit Server VM


Attachments: File CLJ-1413.tar.bz2     File new-src-folder.tar.bz2    

 Description   

On-the-fly generation of defrecord classes fails under certain circumstances.

This github documents the issue: https://github.com/methylene/class-not-found

This was created after a leiningen ticket was closed: https://github.com/technomancy/leiningen/issues/1517



 Comments   
Comment by Alex Miller [ 29/Apr/14 11:33 PM ]

This ticket needs some work to have a better more concise description of the problem and how to reproduce on the ticket.

Comment by Lars Bohl [ 30/Apr/14 1:49 PM ]

Prerequisites:

  • a leiningen installation for the first step (install.sh, creates a library jar).
  • The scripts compile.sh and run.sh expect clojure-1.6.0.jar to be in $HOME/Downloads

Reproduce:

by running ./install.sh && ./compile.sh && ./run.sh

This should print something like "#record_holder.def.ParallelAggregator{}". See the main method in src/class_not_found/core.clj.

Instead it fails with a stacktrace, in the last step (./run.sh)

Notice how ./run.sh doesn't fail, after enabling aot in lib/record-holder/project.clj, and then running ./install.sh again.

Also, notice how ./run.sh doesn't fail, after commenting out this line from test/user.clj: (require '[record-holder.def]), and then running ./compile.sh again.

Comment by Lars Bohl [ 11/May/14 4:48 PM ]

Update:

Attaching new src folder..
The shell scripts mentioned above are not needed.
Also, leiningen is not needed.
Reproduce the error by running error.sh, with these contents:

[CLJ-1413(master)]$ cat error.sh
#!/bin/sh
CLOJURE_JAR=$HOME/Downloads/clojure-1.6.0.jar
rm -rf target && mkdir target
echo -e "(set! *compile-path* \"target\")\n(compile 'class-not-found.core)\n" | java -cp src:$CLOJURE_JAR clojure.main -
java -cp target:$CLOJURE_JAR class_not_found.core

There must be a src folder containing user.clj, core.clj and def.clj as follows:

[CLJ-1413]$ find src/ -type f
src/user.clj
src/class_not_found/core.clj
src/record_holder/def.clj

[CLJ-1413(master)]$ find src/ -type f -exec cat "{}" ";"
;; user.clj
(ns user)
(require '[record-holder.def]) ;; remove this line to get rid of error
;; core.clj
(ns class-not-found.core
  (:gen-class)
  (:require record-holder.def)
  (:import record_holder.def.ParallelAggregator))
(defn -main [& _] (println (ParallelAggregator.)))
;; def.clj
(ns record-holder.def)
(defrecord ParallelAggregator [])
Comment by Nicola Mometto [ 28/Oct/14 6:14 PM ]

This ticket and http://dev.clojure.org/jira/browse/CLJ-1457 might be related





[CLJ-1388] equality bug on records created with nested calls to map->record Created: 18/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

Attachments: Text File 0001-FIX-CLJ-1388.patch     Text File CLJ-1388.patch     Text File CLJ-1388-record-equality-and-map-record-factory.patch     Text File CLJ-1388v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

user> (defrecord a []) 
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2)  ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)  ;; expected => {:a 1}
#user.a{:a 1}

Cause: The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

Approach: Clean the extmap before putting it into the record constructor.

Patch: CLJ-1388-record-equality-and-map-record-factory.patch

Screened by: Alex Miller



 Comments   
Comment by Steve Miner [ 28/Apr/14 3:06 PM ]

The proposed patch 0001-FIX-CLJ-1388.patch makes every map->Record method pay to copy the argument map every time. However, according to my tests, the problem only occurs with records without any fields. So it should be sufficient to generate the (into {} m#) case only when `fields` is empty. [Update: this is wrong, explained below.]

Comment by Steve Miner [ 28/Apr/14 3:10 PM ]

It would be better to fix the problem in the Java Record/create method, but I couldn't figure out how that worked. On the other hand, this bug seems like a fairly rare edge case so I think my patch is acceptable.

Comment by Alex Miller [ 28/Apr/14 3:23 PM ]

Moving out of Screened due to new patch

Comment by Nicola Mometto [ 28/Apr/14 3:35 PM ]

Steve, the problem doesn't occur with records without any fields, your testing was reporting that only because you are only using one record type.

Here's an example that returns true with my patch, but still returns false with yours.

user=> (defrecord a [a])
user.a
user=> (defrecord b [b])
user.b
user=> (def x1 (map->a {:a 1 :b 2}))
#'user/x1
user=> (def x2 (map->a (map->b {:a 1 :b 2})))
#'user/x2
user=> x1
#user.a{:a 1, :b 2}
user=> x2
#user.a{:a 1, :b 2}
user=> (= x1 x2)
false
user=> (.__extmap x2)
#user.b{:b 2}
Comment by Nicola Mometto [ 28/Apr/14 3:37 PM ]

It should also be noted that the overhead of copying the record map is probably insignificant.

Comment by Nicola Mometto [ 28/Apr/14 3:42 PM ]

I also thought at first to fix the problem either on the /create method or on the 3-arity ctor but given that:

  • a fix there would involve messing with the bytecode emitted and thus would be harder to implement than this simple 1-line patch
  • neither the /create method nor the 3-arity ctor is documented and thus should be considered implementation details

I think patching the map->record function is the best way to go.

Comment by Steve Miner [ 28/Apr/14 3:56 PM ]

Nicola, thanks for the correction. I missed the case with multiple records. I withdrew my patch. I'd still like to find a more finely tuned patch, but first I'll have to improve my tests as you demonstrated.

Comment by Steve Miner [ 29/Apr/14 10:17 AM ]

Attached CLJ-1388-record-equality-and-map-record-factory.patch that checks arg to map->Record for MapEquivalence, uses (into {} m#) when necessary. This makes equiv test work correctly with records as the argument (and other map-like values). Added tests with variety of args to map->Record.

Comment by Steve Miner [ 29/Apr/14 10:46 AM ]

A few comments about the new patch... I think the basic issue is a bad interaction between = for records and the generated Record/create method. Everything works when the interal __extmap is a regular map (MapEquivalence), but it fails if __extmap is another record. I think that's because of equiv calling = on the __extmap's.

The user expects to create a new record using the value of another record because it's just like a map. However, = on records respects the record type so it's not = to a map.

The general work-around is to use (into {} x) on the argument to the map->Record. To meet the user's expectation, that `into` call can be incorporated into the map->Record. But I didn't like the defensive copy as most of the time it's unnecessary – the argument is typically a regular map. The `into` work-around is only necessary if the arg is not a MapEquivalence.

There might be a better way to fix the Record/create method but I couldn't figure it out.

Comment by Nicola Mometto [ 29/Apr/14 1:52 PM ]

Steve's last comment made me realize that the root of the problem is on the record .equiv method, where the extmaps are compared via `=`

This new patch (CLJ-1388.patch) addresses this issue by comparing the extmaps with Utils/equiv rather than `=`, which compares maps in a type-indipendent way.

There's still a case where we need recreate the given map, that is when the given map is not an IPersistentMap but simply a java.util.Map.

Steve, my new patch incorporates my fix and your tests, I modified your patch to include only the tests (that were really comprehensive) since I figured it's fair to keep your authorship on those, let me know if that's a problem with you.

Comment by Steve Miner [ 29/Apr/14 2:10 PM ]

Whatever works for you regarding the tests is fine by me.

Comment by Alex Miller [ 30/Apr/14 12:07 AM ]

It seems weird to me that a record should ever contain another record as its extmap. We should be considering the performance aspect but I'm concerned that not locking down extmap more just invites other weirder problems later.

In CLJ-1388.patch, you mention Utils/equiv in your comment but the patch calls Utils/equals - which did you mean?

Also, that patch currently checks if m# is an IPersistentMap - I can't imagine what case we would want to allow where a valid m# is NOT an IPersistentMap?

Comment by Nicola Mometto [ 30/Apr/14 4:15 AM ]

Alex, the Utils/equiv in my comment is wrong (it's easy to confuse between equiv/equals, sorry), Utils/equals in the patch is the right method to use since it compares in a type agnostic way.

Since __extmap is an implementation detail and is only used internally by defrecord for its methods, I don't think it's going to be a problem whether it's a record or a regular clojure map. (Clojure only requires it to be an IPersistentMap)

Regarding the check for m# being an IPersistentMap, Steve in his tests had a case where the map->record ctor was invoked with a java.util.Map, I went to look into the docs for defrecord and it only mentions that the argument to map->record has to be a "map", it doesn't specify that it has to be a clojure map/IPersistentMap, so it seemed right to allow for java maps too and wrap them in IPersistentMaps internally.

Comment by Steve Miner [ 30/Apr/14 8:27 AM ]

My test with java.util.Map was an extension of the idea that anything map-like could be used to initialize a record. That might be a bridge too far, but my patch was testing for MapEquivalence to handle records so it made sense to allow j.u.Map, etc. With Nicola's latest patch, it's probably unnecessary to support non-IPersistentMaps so map->Record doesn't actually need to change.

Comment by Nicola Mometto [ 30/Apr/14 3:57 PM ]

CLJ-1388v2.patch is like CLJ-1388.patch except it doesn't copy non IPersistentMaps in a clojure map.

To summarize, here's the status of the different patches for this ticket:

  • 0001-FIX-CLJ-1388.patch copies the argument of map->record in a clojure map via `(into {} m#)`, be it already a clojure map, a record, or a java.util.Map
  • CLJ-1388-record-equality-and-map-record-factory.patch adopts the same approach except it only copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.MapEquivalence
  • CLJ-1388.patch fixes the issue by changing the function that compares __extmaps from `=` (type aware) to `clojure.lang.Utils/equals` (type agnostic), this patch also copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersistentMap
  • CLJ-1388v2.patch is the same as CLJ-1388.patch except it doesn't copy the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersitentMap, thus map->record will not work with bare java.util.Maps (which is the behaviour it has already)
Comment by Alex Miller [ 05/May/14 1:59 PM ]

Are these patches all still in play? Having 4 active patches does not help move a ticket forward.

Can someone re-summarize at this point what questions exist?

Comment by Nicola Mometto [ 06/May/14 5:26 AM ]

0001-FIX-CLJ-1388.patch should be superseded by the other 3 patches since they solve the same problem in a more performant way.

To pick between the other patches, we need to chose which approach to go with.
Patches CLJ-1388.patch and CLJ-1388v2.patch fix the issue in the equiv method of the defrecord, patch CLJ-1388-record-equality-and-map-record-factory.patch fixes the issue in the map->record ctor by converting maps that don't implement MapEquivalence to a clojure map.

I'd go with either CLJ-1388.patch or CLJ-1388v2.patch since they both avoid copying alltoghether in the cases where map->record currently works, while CLJ-1388-record-equality-and-map-record-factory.patch needs to copy the arg into a map if the arg is a custom IPersistentMap or a record.

To pick between CLJ-1388.patch or CLJ-1388v2.patch we need to decide whether or not the current behaviour of map->record to require strictly an IPersistentMap is the way to go: if we decide that it's ok to pass non IPersitentMap maps like java.util.Map to map->record then pick CLJ-1388.patch otherwise CLJ-1388v2.patch

Comment by Alex Miller [ 06/May/14 10:22 AM ]

From brief conversation with Rich, we should not allow arbitrary map types in __extmap so would prefer to force a clean map and rely on standard equality checking. I think CLJ-1388-record-equality-and-map-record-factory.patch is the preferred path based on that, which still seems like it should avoid copying in nearly all common cases.

Comment by Alex Miller [ 23/May/14 11:19 AM ]

Screened specifically CLJ-1388-record-equality-and-map-record-factory.patch - use map as is if it supports MapEquivalence (and can thus be compared under a map) and otherwise dump into a clojure map.





[CLJ-1344] defrecord still uses old hashing algorithm Created: 07/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

Attachments: Text File clj-1344-1.patch    
Patch: Code
Approval: Ok

 Description   

defrecord implements hasheq by calling clojure.lang.APersistentMap/mapHasheq. mapHasheq uses the old map hash calculation instead of the new one. At least one external collection (data.avl) also calls this function. It should be updated to match the new hasheq calculations.

I considered changing defrecord to call Murmur3 directly, but this would create a case where the generated class does not work with older Clojure runtimes so I left it at calling mapHasheq instead.

Patch: clj-1344-1.patch



 Comments   
Comment by Alex Miller [ 07/Feb/14 1:33 PM ]

Attached patch to make mapHasheq use new hash map calculation.

Comment by Alex Miller [ 10/Feb/14 4:01 PM ]

oops





[CLJ-1261] Invalid defrecord results in exception attributed to namespace that imports namespace with defrecord Created: 12/Sep/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.7

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord, errormsgs

Attachments: File clj-1261-2.diff     File clj-1261-3.diff     File clj-1261-4.diff     File clj-1261-5.diff    
Patch: Code and Test
Approval: Ok

 Description   

I was introducing a namespace that included a defrecord.

My defrecord was wrong; it used a keyword to define a field, not a symbol. Minimal test case:

% cat src/useclj16/init.clj
(ns useclj16.init)

(defrecord Application [:shutdown-fn])
% cat src/useclj16/app.clj 
(ns useclj16.app
  (:require [useclj16.init :as init]))

However, the exception was perplexing:

% java -cp clojure-1.6.0-master-SNAPSHOT.jar:src clojure.main
user=> (require 'useclj16.app)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)

user=> (pst *e 100)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj
        clojure.core/with-meta (core.clj:214)
        clojure.core/defrecord/fn--147 (core_deftype.clj:362)
        clojure.core/map/fn--4210 (core.clj:2494)
        clojure.lang.LazySeq.sval (LazySeq.java:42)
        clojure.lang.LazySeq.seq (LazySeq.java:60)
        clojure.lang.RT.seq (RT.java:484)
        clojure.lang.LazilyPersistentVector.create (LazilyPersistentVector.java:31)
        clojure.core/vec (core.clj:354)
        clojure.core/defrecord (core_deftype.clj:362)
        clojure.lang.Var.invoke (Var.java:427)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.lang.Compiler.macroexpand1 (Compiler.java:6483)
        clojure.lang.Compiler.macroexpand (Compiler.java:6544)
        clojure.lang.Compiler.eval (Compiler.java:6618)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        useclj16.app/eval322/loading--4916--auto----323 (app.clj:1)
        useclj16.app/eval322 (app.clj:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6623)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        user/eval318 (NO_SOURCE_FILE:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6597)
        clojure.core/eval (core.clj:2864)
        clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
        clojure.main/repl/read-eval-print--6594 (main.clj:260)
        clojure.main/repl/fn--6603 (main.clj:278)
        clojure.main/repl (main.clj:278)
        clojure.main/repl-opt (main.clj:344)
        clojure.main/main (main.clj:442)
        clojure.lang.Var.invoke (Var.java:411)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.main.main (main.java:37)
nil

The error was attributed to app.clj (useclj16.app), a namespace which requires useclj16.init, the namespace containing the defrecord.

No indication that this concerned a defrecord, or even what namespace contained the error, was present in the exception.

Patch: clj-1261-5.diff

Approach: Check explicitly that the fields are all symbols, for both defrecord and deftype, and throw a CompilerException with file, line, and column number if not. Example of exception after patch is applied, in the case give above:

user=> (require 'useclj16.app)
CompilerException java.lang.AssertionError: defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, compiling:(useclj16/init.clj:3:1)

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 12/Sep/13 8:58 PM ]

Can you include an example of the defrecord definition just so we're clear what it looks like?

Comment by Alex Miller [ 12/Sep/13 8:59 PM ]

Also, "feedback" is not a useful label. Please use "errormsgs" for stuff like this. See the list of many commonly used labels here: http://dev.clojure.org/display/community/Creating+Tickets

Comment by Howard Lewis Ship [ 13/Sep/13 10:42 AM ]

"Feedback" is my own personal crusade http://tapestryjava.blogspot.com/2013/05/once-more-feedback-please.html

In my case, my invalid code was:

(defrecord Application [:shutdown-fn])

And the mistake was that :shutdown-fn should be a symbol, not a keyword.

Here it is, more completely:

(ns novate.services.initialization
  "Infrastructure for system-as-transient state.")

(defrecord Application [:shutdown-fn])

and

(ns novate.services.activator
  "Responsible for bootstrapping the application by loading certain namespaces and invoking certain functions, guided by data in JAR manifests."
  (:gen-class)
  (:require [clojure.edn :as edn]
            [clojure.java.io :as io]
            [novate.util.logging :as l]
            [novate.services
             [initialization :as init]
             [ordering :as o]])
  (:import [java.io PushbackReader]))

The error was attributed to this file.

Comment by Andy Fingerhut [ 13/Sep/13 11:48 AM ]

Patch clj-1261-v1.txt throws an exception if any fields given to defrecord or deftype are not symbols. They are CompilerExceptions, so include an accurate file, line, and column number.

Comment by Andy Fingerhut [ 13/Sep/13 11:57 AM ]

Updated description to give minimal test case.

Comment by Andy Fingerhut [ 23/Nov/13 1:06 AM ]

Patch clj-1261-2.diff is identical to clj-1261-v1.txt except that it applies cleanly to latest master. The only change was to some lines of context due to recent commits to Clojure.

Comment by Alex Miller [ 18/Apr/14 1:16 PM ]

I think the patch is ok but I have two suggestions in the error message - first, include the record/type ns+name (I think the classname in the patched fn is what you want). Second, I think the wording could be adjusted a bit and the parens should go away - those look like but don't actually have meaning in the original context (since you are filtering out the symbols). Maybe something like:

"defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, :foo-bar"

Comment by Andy Fingerhut [ 30/Apr/14 9:32 AM ]

Patch clj-1261-3.diff attempts to incorporate Alex's suggested error message changes.

There are other errors caught by function validate-fields that could have more details like the namespace and record/type name added to them, but I don't want to go out of scope for the ticket. I can create another patch that does that if there is interest.

Comment by Alex Miller [ 30/Apr/14 2:56 PM ]

Can you update the "after" example in the Approach section of the description to match new?

Comment by Andy Fingerhut [ 01/May/14 4:18 PM ]

Updated example output at end of description to be what is seen after patch clj-1261-3.diff is applied.

Comment by Alex Miller [ 05/May/14 1:56 PM ]

Description looks good, patch looks good. Test?

Comment by Andy Fingerhut [ 05/May/14 2:03 PM ]

I'd be happy to write one, if I had a "similar" one to pattern them on.

By similar, I mean: are there any existing tests that require a namespace that isn't already loaded & compiled when the tests begin running, and catch exceptions thrown during the require?

Comment by Alex Miller [ 05/May/14 3:56 PM ]

I think you should be able to test the right error message here by just invoking the defrecord form.

Otherwise, maybe https://github.com/clojure/clojure/blob/master/test/clojure/test_clojure/ns_libs.clj#L87 ?

Comment by Andy Fingerhut [ 10/May/14 6:43 PM ]

Patch clj-1261-4.diff is identical to clj-1261-3.diff except that it adds a couple of unit tests verifying that an exception of the desired type and with an appropriate message is thrown when keywords are used as defrecord or deftype fields.

Comment by Alex Miller [ 13/May/14 10:41 PM ]

same as -4 but changed final defrecord to deftype in test (seemed like a typo)

Comment by Andy Fingerhut [ 13/May/14 11:40 PM ]

Thanks for the catch on that typo in the tests. You changed it to what I had intended.

Comment by Alex Miller [ 13/May/14 11:54 PM ]

seemed pretty clear





[CLJ-1224] Records do not cache hash like normal maps Created: 24/Jun/13  Updated: 06/Oct/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.8

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: defrecord, performance

Attachments: Text File 0001-cache-hasheq-and-hashCode-for-records.patch    
Patch: Code
Approval: Vetted

 Description   

Records do not cache their hash codes like normal Clojure maps, which affects their performance. This problem has been fixed in CLJS, but still affects JVM CLJ.

Approach: Cache hash values in record definitions, similar to maps.

Patch: 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch

Also see: http://dev.clojure.org/jira/browse/CLJS-281



 Comments   
Comment by Nicola Mometto [ 14/Feb/14 5:46 PM ]

I want to point out that my patch breaks ABI compatibility.
A possible approach to avoid this would be to have 3 constructors instead of 2, I can write the patch to support this if desired.

Comment by Stuart Halloway [ 27/Jun/14 11:09 AM ]

The patch 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch is broken in at least two ways:

  • The fields __hash and __hasheq are adopted by new records created by .assoc and .without, which will cause those records to have incorrect (and likely colliding) hash values
  • The addition of the new fields breaks the promise of defrecord, which includes an N+2 constructor taking meta and extmap. With the patch, defrecords get an N+4 constructor letting callers pick hash codes.

I found these problems via the following reasoning:

  • Code has been touched near __extmap
  • Grep for all uses of __extmap and see what breaks
Comment by Nicola Mometto [ 27/Jun/14 2:53 PM ]

Patch 0001-cache-hasheq-and-hashCode-for-records.patch fixes both those issues, reintroducing the N+2 arity constructor

Comment by Alex Miller [ 27/Jun/14 4:08 PM ]

Questions addressed, back to Vetted.

Comment by Andy Fingerhut [ 29/Aug/14 4:32 PM ]

All patches dated Jun 7 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch.

Comment by Alex Miller [ 29/Aug/14 4:40 PM ]

Would be great to get this one updated as it's otherwise ready to screen.

Comment by Nicola Mometto [ 29/Aug/14 4:58 PM ]

Updated patch to apply to lastest master





[CLJ-1208] Namespace is not loaded on defrecord class init Created: 03/May/13  Updated: 06/Oct/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.8

Type: Enhancement Priority: Major
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: defrecord

Approval: Vetted

 Description   

As a user of Clojure interop from Java, I want defrecords (and deftypes?) to load their namespaces upon class initialization so that I can simply construct and use AOT'd record classes without manually requiring their namespaces first.

Calling the defrecord's constructor may or may not result in "Attempting to call unbound fn" exceptions, depending on what code has already been run.

This issue has been raised several times over the years, but I could not find an existing ticket for it:






[CLJ-1132] For record type Rec, (instance? Rec (map->Rec {...})) need not return true, though (instance? Rec (Rec. ...)) does. Created: 18/Dec/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Christopher Genovese Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord
Environment:

Apache Tomcat/6.0.24 JVM/1.6.0_26-b03 Linux 2.6.32-279.el6.x86_64

Clojure 1.4.0, Ring 1.1.6, Compojure 1.1.3, Lein-Ring Plugin 0.7.5 (for lein ring uberwar)


Attachments: File recordbug.tgz    

 Description   

(defrecord Rec ...)

(instance? Rec (Rec. ...)) ;=> true
(instance? Rec (map->Rec {...})) ;=> can be false

This occurs when the code is wrapped in a tomcat servlet by `lein ring
uberwar`, but not when run at the REPL or under Jetty, say.

Although produced under ring, this seems to me to be a general problem
with the map-> constructor, as (true? (instance? Rec (map->Rec {...})))
should be an invariant, regardless of the environment or context.
The problem seems to arise in some aspect of the class loading process:

(.getClassLoader Rec) ;=> WebappClassLoader (delegate: false, repositories: /WEB-INF/classes/, parent: org.apache.catalina.loader.StandardClassLoader@790bc49d)
(.getClassLoader (class (Rec. ...))) ;=> WebappClassLoader (same as the previous line)
(.getClassLoader (class (map->Rec ...))) ;=> clojure.lang.DynamicClassLoader@2e7227a8

The map->Rec delegates to the create method, which seems to be where the problem lies.

The record namespace is AOT compiled, properly I think/hope, and the requisite classes
imported as they should be.

I have attached a minimal web app that reproduces the problem and shows
the configuration. As a sanity check, I have also created a trivial
workaround defrecord* that creates a clojure-native map constructor,
for which the problem does not occur. See the README.md in the tar
file for usage details, and the project.clj for my configuration.

Again, I've only been able to reproduce the problem under Tomcat,
not under Jetty or at the REPL.






[CLJ-929] Accessing Java property starting with _ has issues in 1.4 Created: 07/Feb/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

When attempting to use interop syntax with symbols which aren't legal Java names (such as deleted?), the names are mangled a bit. That's necessary, of course, and the method of munging can be internal to the compiler. However, the behavior when munging changed a little between 1.3 and 1.4 beta1. Obviously the specifics of munging are something I should avoid relying on, but the way it changed looks like an accident or a bug even so.

The use-case I ran into is that defrecords contain a field named __meta for tracking their metadata. In both 1.3 and 1.4 you can get at that field with (. record __meta), which avoids munging. But on 1.3 (. record --meta) also accesses it (translating each - to a _), while on 1.4 (. record -meta) works and (. record --meta) doesn't.

Actually, looking at line 883 of Compiler.java, it looks like this may be related to the (. foo -property) syntax ported from CLJS, and indeed (. record ---meta) works, I guess by reducing to an "old style" (. record --meta). So that clears up why --meta fails: it's looking for __meta. I'm still not clear on why (. record -meta) works, though.

So it looks like the - prefix for properties is not 100% backwards-compatible like it seemed to be. Is this an issue we need to fix, or is the recommendation simply to never have fields that start with - or _?



 Comments   
Comment by Fogus [ 09/Feb/12 2:33 PM ]

Is this a general problem with fields starting with _ or just fields named __meta as in (defrecord [__meta] ...)

Comment by Alan Malloy [ 09/Feb/12 3:01 PM ]

It's a general issue. (defrecord [__meta]) actually breaks immediately, because the record mechanism itself generates a field named __meta, but any field named with a - or _ prefix has this issue.

user=> (defrecord Foo [-blah])
user.Foo
user=> (.-blah (Foo. 1))
IllegalArgumentException No matching field found: blah for class user.Foo clojure.lang.Reflector.getInstanceField (Reflector.java:289)





[CLJ-864] defrecord positional arity factory fn should have an inline version that calls the record constructor Created: 26/Oct/11  Updated: 04/Dec/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

defrecord positional arity factory fn should have an inline version that calls the record constructor



 Comments   
Comment by Gary Fredericks [ 26/May/13 3:39 PM ]

I had the idea recently that the factory fn was useful partly for being a redefinable var, e.g. that you could wrap with contracts or anything else. This idea would preclude that.

It makes sense though if the only purpose of ->Foo is to avoid having to :import something.

Comment by Kevin Downey [ 13/Aug/13 4:04 PM ]

interesting, that is a good point

Comment by Gary Fredericks [ 04/Dec/13 7:21 AM ]

Another thought – using the factory fns rather than constructors directly gives you a little bit of protection against code-reloading issues, does it not? I don't think I understand the code reloading issues in great detail, so I'm not confident about this. My assumption is that the compiled code refers to vars rather than classes.





[CLJ-405] better error messages for bad defrecord calls Created: 20/Jul/10  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord, errormsgs


 Description   

defrecord could tell you if, e.g., you didn't specify an interface before leaping into method bodies. See http://groups.google.com/group/clojure/browse_thread/thread/f52f90954edd8b09



 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:28 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/405

Comment by Assembla Importer [ 24/Aug/10 12:28 AM ]

stu said: This could be fixed with an assert-valid-defrecord call in core_deftype, similar to assert-valid-fdecl in core.clj. Such a function would also be a place to hang other defrecord error messages.





[CLJ-394] Add record? predicate Created: 05/Jul/10  Updated: 11/Jan/14  Resolved: 11/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

Attachments: File clj-394-add-predicate-for-record-3.diff     File clj-394-add-predicates-for-type-and-record-2.diff     File clj-394-add-predicates-for-type-and-record.diff    
Patch: Code and Test
Approval: Ok

 Description   

Would like a predicate that can be used to check whether an object is an instance of a defrecord.

Patch: clj-394-add-predicate-for-record-3.diff

Approach: Use existing marker interface IRecord to add new record? predicate.

user=> (defrecord Foo [x])
user.Foo
user=> (def f (->Foo 1))
#'user/f
user=> (record? f)
true

Screened by: Alex Miller



 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:04 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/394

Comment by Steve Miner [ 20/Apr/12 1:55 PM ]

As of Clojure 1.3, there are marker interfaces named clojure.lang.IType and clojure.lang.IRecord. You can use instance? with those interfaces. I'm not sure if they're actually documented for public use, but they seem to work as expected in 1.3 and 1.4. If you want record?, you can try this:

(defn record? [rec] (instance? clojure.lang.IRecord rec))

Comment by Devin Walters [ 20/Oct/12 6:38 PM ]

See attached code and test. I'm unsure as to whether or not the location of the tests and predicates make sense. Please let me know if I should move them elsewhere.

Comment by Alex Miller [ 16/Oct/13 9:29 AM ]

Tweaked patch to say added in 1.6, not 1.5.

Comment by Rich Hickey [ 22/Nov/13 11:41 AM ]

I think this should do record? only. type? conveys no semantics, and the things are not types themselves.

Comment by Alex Miller [ 22/Nov/13 12:39 PM ]

Altering title and description per Rich's comment.

Comment by Alex Miller [ 22/Nov/13 12:49 PM ]

Added updated patch that just adds record?

Comment by Alex Miller [ 22/Nov/13 12:53 PM ]

Added new patch that contains only the record? fn and omits the type stuff. Marking Screened again.





Generated at Fri Oct 31 23:52:09 CDT 2014 using JIRA 4.4#649-r158309.