0837-fixed.patch
CLJ-837-double-underscore-type-records.diff
[CLJ-837] java.lang.VerifyError when compiling deftype or defrecord with argument name starting with double underscore characters. Created: 11/Sep/11 Updated: 09/Dec/11 Resolved: 09/Dec/11 |
|
| Status: | Closed |
| Project: | Clojure |
| Component/s: | None |
| Affects Version/s: | Release 1.3 |
| Fix Version/s: | Release 1.4 |
| Type: | Defect | Priority: | Minor |
| Reporter: | Tchavdar Roussanov | Assignee: | Fogus |
| Resolution: | Completed | Votes: | 1 |
| Labels: | None | ||
| Environment: |
clojure-1.3.0-beta3 and git master, JDK 1.6.0_26, OS X 1.7.1 |
||
| Attachments: |
0837-fixed.patch
CLJ-837-double-underscore-type-records.diff
|
| Patch: | Code and Test |
| Approval: | Ok |
| Waiting On: | Rich Hickey |
| Description |
|
The minimal test to trigger the verification error is : (deftype t [__]) It works fine in clojure 1.2, 1.2.1 Works fine with one underscore. Output from REPL: user=> (defrecord r [__]) Stack trace from slime session: (class: clojure/data/priority_map/r, method: create signature: (Lclojure/lang/IPersistentMap;)Lclojure/data/priority_map/r Restarts: Backtrace: |
| Comments |
| Comment by Fogus [ 11/Sep/11 9:18 PM ] |
|
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 |
| Comment by Fogus [ 12/Sep/11 8:32 AM ] |
|
I think the prudent fix is as follows:
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? |
| Comment by Fogus [ 12/Sep/11 9:44 AM ] |
|
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. |
| Comment by Rich Hickey [ 12/Sep/11 10:09 AM ] |
|
differing behavior of undocumented features is a non-problem |
| Comment by Fogus [ 12/Sep/11 12:05 PM ] |
|
Got it. Thanks Rich. |
| Comment by Fogus [ 12/Sep/11 12:25 PM ] |
|
Added patch file. |
| Comment by Steve Miner [ 12/Sep/11 2:11 PM ] |
|
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. |
| Comment by Steve Miner [ 12/Sep/11 2:13 PM ] |
|
Just by inspection, it looks like patch has an extra space before the 'b' in the line near the end: + 2 (:___ b r) |
| Comment by Stuart Halloway [ 02/Dec/11 1:53 PM ] |
|
0837-fixed is same as original patch, but with fixed unit test. |
Expecting to find object/array on stack, compiling:(NO_SOURCE_PATH:2)