Clojure

java.lang.VerifyError when compiling deftype or defrecord with argument name starting with double underscore characters.

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.3
  • Fix Version/s: Release 1.4
  • Component/s: None
  • Labels:
    None
  • Environment:
    clojure-1.3.0-beta3 and git master, JDK 1.6.0_26, OS X 1.7.1
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

The minimal test to trigger the verification error is :

(deftype t [__])
or
(defrecord r [__])

It works fine in clojure 1.2, 1.2.1

Works fine with one underscore.

Output from REPL:

user=> (defrecord r [__])
CompilerException java.lang.VerifyError: (class: user/r, method: create signature: (Lclojure/lang/IPersistentMap;)Luser/r Expecting to find object/array on stack, compiling:(NO_SOURCE_PATH:2)

Stack trace from slime session:

(class: clojure/data/priority_map/r, method: create signature: (Lclojure/lang/IPersistentMap;)Lclojure/data/priority_map/r Expecting to find object/array on stack
[Thrown class java.lang.VerifyError]

Restarts:
0: [QUIT] Quit to the SLIME top level

Backtrace:
0: java.lang.Class.forName0(Native Method)
1: java.lang.Class.forName(Class.java:247)
2: clojure.lang.RT.classForName(RT.java:2013)
3: clojure.lang.Compiler$HostExpr.maybeClass(Compiler.java:929)
4: clojure.lang.Compiler$HostExpr.access$400(Compiler.java:710)
5: clojure.lang.Compiler$NewExpr$Parser.parse(Compiler.java:2391)
6: clojure.lang.Compiler.analyzeSeq(Compiler.java:6368)
7: clojure.lang.Compiler.analyze(Compiler.java:6175)
8: clojure.lang.Compiler.analyze(Compiler.java:6136)
9: clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5531)
10: clojure.lang.Compiler$FnMethod.parse(Compiler.java:4967)
[No Locals]
11: clojure.lang.Compiler$FnExpr.parse(Compiler.java:3588)
12: clojure.lang.Compiler.analyzeSeq(Compiler.java:6366)
13: clojure.lang.Compiler.analyze(Compiler.java:6175)
14: clojure.lang.Compiler.analyzeSeq(Compiler.java:6356)
15: clojure.lang.Compiler.analyze(Compiler.java:6175)
16: clojure.lang.Compiler.access$100(Compiler.java:37)
17: clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:492)
18: clojure.lang.Compiler.analyzeSeq(Compiler.java:6368)
19: clojure.lang.Compiler.analyze(Compiler.java:6175)
20: clojure.lang.Compiler.analyzeSeq(Compiler.java:6356)
21: clojure.lang.Compiler.analyze(Compiler.java:6175)
22: clojure.lang.Compiler.analyze(Compiler.java:6136)
23: clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5531)
24: clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5832)
25: clojure.lang.Compiler.analyzeSeq(Compiler.java:6368)
26: clojure.lang.Compiler.analyze(Compiler.java:6175)
27: clojure.lang.Compiler.analyze(Compiler.java:6136)
28: clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5531)
29: clojure.lang.Compiler$FnMethod.parse(Compiler.java:4967)
30: clojure.lang.Compiler$FnExpr.parse(Compiler.java:3588)
31: clojure.lang.Compiler.analyzeSeq(Compiler.java:6366)
32: clojure.lang.Compiler.analyze(Compiler.java:6175)
33: clojure.lang.Compiler.eval(Compiler.java:6421)
34: clojure.lang.Compiler.eval(Compiler.java:6390)
35: clojure.core$eval.invoke(core.clj:2795)
36: swank.commands.basic$eval_region.invoke(basic.clj:47)
37: swank.commands.basic$eval_region.invoke(basic.clj:37)
38: swank.commands.basic$eval764$listener_eval__765.invoke(basic.clj:71)
39: clojure.lang.Var.invoke(Var.java:401)
40: clojure.data.priority_map$eval2759.invoke(NO_SOURCE_FILE)
41: clojure.lang.Compiler.eval(Compiler.java:6424)
42: clojure.lang.Compiler.eval(Compiler.java:6390)
43: clojure.core$eval.invoke(core.clj:2795)
44: swank.core$eval_in_emacs_package.invoke(core.clj:92)
45: swank.core$eval_for_emacs.invoke(core.clj:239)
46: clojure.lang.Var.invoke(Var.java:409)
47: clojure.lang.AFn.applyToHelper(AFn.java:167)
48: clojure.lang.Var.applyTo(Var.java:518)
49: clojure.core$apply.invoke(core.clj:600)
50: swank.core$eval_from_control.invoke(core.clj:99)
[No Locals]
51: swank.core$eval_loop.invoke(core.clj:104)
52: swank.core$spawn_repl_thread$fn_529$fn_530.invoke(core.clj:309)
53: clojure.lang.AFn.applyToHelper(AFn.java:159)
54: clojure.lang.AFn.applyTo(AFn.java:151)
55: clojure.core$apply.invoke(core.clj:600)
56: swank.core$spawn_repl_thread$fn__529.doInvoke(core.clj:306)
57: clojure.lang.RestFn.invoke(RestFn.java:397)
58: clojure.lang.AFn.run(AFn.java:24)
59: java.lang.Thread.run(Thread.java:680)

Activity

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

The privileged names __meta and __extmap are implementation details of types and records and should probably not be viewed as real. In other words, since they are not published as part of the type/record docs, then they may not exist in the future. Having said that, the current Clojure implementation treats any name starting with __ as a special field and doesn't count them toward the field count. As a result, when emitting the MyType.create method (another implementation detail), the fields marked as __ throw off the field count and as a result the bytecode is not emitted properly. I know how to change the code to allow names prefixed with __ and fixing the validation error (not called by those two special names listed above), but I hesitate to say that is the solution
without further reflection.

Show
Fogus added a comment - The privileged names __meta and __extmap are implementation details of types and records and should probably not be viewed as real. In other words, since they are not published as part of the type/record docs, then they may not exist in the future. Having said that, the current Clojure implementation treats any name starting with __ as a special field and doesn't count them toward the field count. As a result, when emitting the MyType.create method (another implementation detail), the fields marked as __ throw off the field count and as a result the bytecode is not emitted properly. I know how to change the code to allow names prefixed with __ and fixing the validation error (not called by those two special names listed above), but I hesitate to say that is the solution without further reflection.
Hide
Permalink
Fogus added a comment - - edited

I think the prudent fix is as follows:

  • Allow any legal name except those in the set #{__meta __extmap}
  • Fix the #create emission
  • Documentation should be changed to reflect that the aforementioned set is disallowed

Thankfully this is an easy fix. However, the "magic" of __ prefixed names will go away. That is, the following behavior will differ from v1.2:

(defrecord R [__a ___b])
(:__a (R. 42 9))
;=> 42

(deftype T [__a])
(.__a (T. 36))
;=> 36

(deftype T2 [a b __meta])
;; AssertionError

Any thoughts?

Show
Fogus added a comment - - edited I think the prudent fix is as follows:
  • Allow any legal name except those in the set #{__meta __extmap}
  • Fix the #create emission
  • Documentation should be changed to reflect that the aforementioned set is disallowed
Thankfully this is an easy fix. However, the "magic" of __ prefixed names will go away. That is, the following behavior will differ from v1.2: 
(defrecord R [__a ___b])
(:__a (R. 42 9))
;=> 42

(deftype T [__a])
(.__a (T. 36))
;=> 36

(deftype T2 [a b __meta])
;; AssertionError
Any thoughts?
Hide
Permalink
Fogus added a comment -

If anyone is interested, the changes reflected in the previous comment can be found on my own branch: https://github.com/fogus/clojure/tree/CLJ-837 Code-review would be nice.

Show
Fogus added a comment - If anyone is interested, the changes reflected in the previous comment can be found on my own branch: https://github.com/fogus/clojure/tree/CLJ-837 Code-review would be nice.
Hide
Permalink
Rich Hickey added a comment -

differing behavior of undocumented features is a non-problem

Show
Rich Hickey added a comment - differing behavior of undocumented features is a non-problem
Hide
Permalink
Fogus added a comment -

Got it. Thanks Rich.

Show
Fogus added a comment - Got it. Thanks Rich.
Hide
Permalink
Fogus added a comment -

Added patch file.

Show
Fogus added a comment - Added patch file.
Hide
Permalink
Steve Miner added a comment -

I suggest that the documentation should simply say that all fields beginning with double-underscore (__) are reserved. You don't have to enforce that (as the patch allows any field name except __meta and __extmap), but it's better to be conservative with the documentation so that you have some room to innovate.

Show
Steve Miner added a comment - I suggest that the documentation should simply say that all fields beginning with double-underscore (__) are reserved. You don't have to enforce that (as the patch allows any field name except __meta and __extmap), but it's better to be conservative with the documentation so that you have some room to innovate.
Hide
Permalink
Steve Miner added a comment -

Just by inspection, it looks like patch has an extra space before the 'b' in the line near the end:

+ 2 (:___ b r)

Show
Steve Miner added a comment - Just by inspection, it looks like patch has an extra space before the 'b' in the line near the end: + 2 (:___ b r)
Hide
Permalink
Stuart Halloway added a comment -

0837-fixed is same as original patch, but with fixed unit test.

Show
Stuart Halloway added a comment - 0837-fixed is same as original patch, but with fixed unit test.

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
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.