<< Back to previous view

[CLJ-1807] Add prefer-proto, like prefer-method but for protocols Created: 30/Aug/15  Updated: 30/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols

Attachments: Text File 0001-CLJ-1807-add-prefer-proto.patch    
Patch: Code and Test

 Description   

Currently it's possible to extend a protocol to multiple interfaces but there's no mechanism like prefer-method for multimethods to prefer one implementation over another, as a result, if multiple interfaces match, a random one is picked.

One particular example where this is a problem, is trying to handle generically records and maps (this come up in tools.analyzer): when extending a protocol to both IRecord and IPersistentMap there's no way to make the IRecord implementation be chosen over the IPersistentMap one and thus protocols can't be used.

The attached patch adds a prefer-proto function that's like prefer-method but for protocols.

No performance penalty is paid if prefer-proto is never used, if it's used there will be a penalty during the first protocol method dispatch to lookup the perference table but the protocol method cache will remove that penalty for further calls.

Example:

user=> (defprotocol p (f [_]))
p
user=> (extend-protocol p clojure.lang.Counted (f [_] 1) clojure.lang.IObj (f [_] 2))
nil
user=> (f [1])
2
user=> (prefer-proto p clojure.lang.Counted clojure.lang.IObj)
nil
user=> (f [1])
1

Patch: 0001-CLJ-1807-add-prefer-proto.patch






[CLJ-1806] group-by as reducer / reduction fn Created: 29/Aug/15  Updated: 29/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Karsten Schmidt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers, transducers

Attachments: Text File clj-1806-1.patch    

 Description   

Whilst working on a query engine heavily based on transducers, I noticed that it'd be great to have group-by able to be used as reduction fn. The attached patch adds a single arity version to group-by which enables this use case and also includes a few tests.



 Comments   
Comment by Karsten Schmidt [ 29/Aug/15 8:08 PM ]

patch w/ described changes





[CLJ-1805] :rettag encourages wrong runtime type hints Created: 28/Aug/15  Updated: 29/Aug/15

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints

Approval: Triaged

 Description   

Currently :rettag works only for expressions like:

(defn ^long x [..] ..)

or

(defn ^double x [..] ..)

But at runtime those type-hints are resolved to

#<clojure.core$long ..>
and
#<clojure.core$double ..>

which can cause the compiler to fail, see CLJ-1674 for an example



 Comments   
Comment by Nicola Mometto [ 29/Aug/15 1:27 PM ]

It also appears that the current impl is completely useless: see David Miller's comment: https://github.com/clojure/clojure/commit/2344de2b2aadd5b0e47f1594a6f9e4eb2fdbdf5c#commitcomment-12962027





[CLJ-701] Primitive return type of loop and try is lost Created: 03/Jan/11  Updated: 28/Aug/15

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

Type: Defect Priority: Major
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT


Attachments: Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch     Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch     File hoistedmethod-pass-1.diff     File hoistedmethod-pass-2.diff     File hoistedmethod-pass-3.diff     File hoistedmethod-pass-4.diff     File hoistedmethod-pass-5.diff     File hoistedmethod-pass-6.diff     File hoistedmethod-pass-7.diff    
Patch: Code and Test
Approval: Vetted

 Description   
(set! *warn-on-reflection* true)
(fn [] (loop [b 0] (recur (loop [a 1] a))))

Generates the following warnings:

recur arg for primitive local: b is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: b

This is interesting for several reasons. For one, if the arg to recur is a let form, there is no warning:

(fn [] (loop [b 0] (recur (let [a 1] a))))

Also, the compiler appears to understand the return type of loop forms just fine:

(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}

The problem can of course be worked around using an explicit cast on the loop form:

(fn [] (loop [b 0] (recur (long (loop [a 1] a)))))

Reported by leafw in IRC: http://clojure-log.n01se.net/date/2011-01-03.html#10:31

See Also: CLJ-1422

Patch: 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch



 Comments   
Comment by a_strange_guy [ 03/Jan/11 4:36 PM ]

The problem is that a 'loop form gets converted into an anonymous fn that gets called immediately, when the loop is in a expression context (eg. its return value is needed, but not as the return value of a method/fn).

so

(fn [] (loop [b 0] (recur (loop [a 1] a))))

gets converted into

(fn [] (loop [b 0] (recur ((fn [] (loop [a 1] a))))))

see the code in the compiler:
http://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L5572

this conversion already bites you if you have mutable fields in a deftype and want to 'set! them in a loop

http://dev.clojure.org/jira/browse/CLJ-274

Comment by Christophe Grand [ 23/Nov/12 2:28 AM ]

loops in expression context are lifted into fns because else Hotspot doesn't optimize them.
This causes several problems:

  • type inference doesn't propagate outside of the loop[1]
  • the return value is never a primitive
  • mutable fields are inaccessible
  • surprise allocation of one closure objects each time the loop is entered.

Adressing all those problems isn't easy.
One can compute the type of the loop and emit a type hint but it works only with reference types. To make it works with primitive, primitie fns aren't enough since they return only long/double: you have to add explicit casts.
So solving the first two points can be done in a rather lccal way.
The two other points require more impacting changes, the goal would be to emit a method rather than a fn. So it means at the very least changing ObjExpr and adding a new subclassof ObjMethod.

[1] beware of CLJ-1111 when testing.

Comment by Alex Miller [ 21/Oct/13 10:28 PM ]

I don't think this is going to make it into 1.6, so removing the 1.6 tag.

Comment by Kevin Downey [ 21/Jul/14 7:14 PM ]

an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn

Comment by Kevin Downey [ 22/Jul/14 8:39 PM ]

I have attached a 3 part patch as hoistedmethod-pass-1.diff

3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class

9c39cac1 uses HoistedMethod to compile loops not in the return context

901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)

Comment by Kevin Downey [ 22/Jul/14 8:54 PM ]

with hoistedmethod-pass-1.diff the example code generates bytecode like this

user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a))))))
// Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit)
public final class user$eval1675$fn__1676 extends clojure.lang.AFunction {
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__0;
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__1;
  
  // Method descriptor #10 ()V
  // Stack: 2, Locals: 0
  public static {};
     0  lconst_0
     1  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
     4  putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18]
     7  lconst_1
     8  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
    11  putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20]
    14  return
      Line numbers:
        [pc: 0, line: 1]

 // Method descriptor #10 ()V
  // Stack: 1, Locals: 1
  public user$eval1675$fn__1676();
    0  aload_0 [this]
    1  invokespecial clojure.lang.AFunction() [23]
    4  return
      Line numbers:
        [pc: 0, line: 1]
  
  // Method descriptor #25 ()Ljava/lang/Object;
  // Stack: 3, Locals: 3
  public java.lang.Object invoke();
     0  lconst_0
     1  lstore_1 [b]
     2  aload_0 [this]
     3  lload_1 [b]
     4  invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29]
     7  lstore_1 [b]
     8  goto 2
    11  areturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 11] local: b index: 1 type: long
        [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object

 // Method descriptor #27 (J)J
  // Stack: 2, Locals: 5
  public long __hoisted1677(long b);
    0  lconst_1
    1  lstore_3 [a]
    2  lload_3
    3  lreturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 3] local: a index: 3 type: long
        [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object
        [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object

}
nil
user=> 
  

the body of the method __hoisted1677 is the inner loop

for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version

Comment by Kevin Downey [ 23/Jul/14 12:43 AM ]

hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr

with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass

Comment by Alex Miller [ 23/Jul/14 12:03 PM ]

Thanks for the work on this!

Comment by Kevin Downey [ 23/Jul/14 2:05 PM ]

I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do

Comment by Kevin Downey [ 25/Jul/14 7:08 PM ]

hoistedmethod-pass-3.diff

49782161 * add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 * hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required

all contribs whose tests pass with master pass with this patch.

the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot

Comment by Nicola Mometto [ 26/Jul/14 1:37 AM ]

Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class

Test case:

user=> (fn [] (try 1 (finally)) 2)
VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack  user/eval1 (NO_SOURCE_FILE:1)
Comment by Kevin Downey [ 26/Jul/14 1:46 AM ]

bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too

Comment by Kevin Downey [ 26/Jul/14 12:52 PM ]

hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code

Comment by Kevin Downey [ 26/Jul/14 12:58 PM ]

hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff

Comment by Nicola Mometto [ 21/Aug/15 1:03 PM ]

Kevin Downey FWIW the patch no longer applies

Comment by Michael Blume [ 21/Aug/15 3:06 PM ]

A naive attempt to bring the patch up to date results in

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7112)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7416)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7859)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351)
     [java] 	... 17 more
Comment by Kevin Downey [ 25/Aug/15 1:00 PM ]

the pass 6 patch builds cleanly on 1d5237f9d7db0bc5f6e929330108d016ac7bf76c(HEAD of master as of this moment) and runs clojure's tests. I have not verified it against other projects as I did with the previous patches (I don't remember how I did that)

Comment by Michael Blume [ 25/Aug/15 1:38 PM ]

I get the same error I shared before applying the new patch to 1d5237f

Comment by Alex Miller [ 25/Aug/15 1:56 PM ]

I get some whitespace warnings with hoistedmethod-pass-6.diff but the patch applies. On a compile I get:

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7343)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:588)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7355)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7114)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7418)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7861)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	... 17 more
Comment by Nicola Mometto [ 25/Aug/15 2:36 PM ]

Looks like direct linking interacts with the diffs in this patch in non trivial ways.

Comment by Kevin Downey [ 25/Aug/15 3:05 PM ]

I must have screwed up running the tests some how, I definitely get the same error now

Comment by Kevin Downey [ 25/Aug/15 4:00 PM ]

after you get past the cast issuing (just adding some conditional logic there)

it looks like HoistedMethodInvocationExpr needs to be made aware of if it is emitting in an instance method or a static method, and do the right thing with regard to this pointers and argument offsets. this will likely require HoistedMethod growing the ability to be a static method (and maybe preferring static methods when possible).

if you cause HoistedMethod to set usesThis to true on methods that use it, then everything appears hunky-dory (if I ran the tests correctly), but this largely negates the new direct linking stuff, which is not good.

Comment by Kevin Downey [ 27/Aug/15 9:30 PM ]

hoistedmethod-pass-7.diff adds a single commit to hoistedmethod-pass-5.diff

the single commit changes hoisted methods to always be static methods, and adjusts the arguments in the invocation of the hoisted method based on if the containing function is a static/direct function or not.

again I haven't done the extended testing with this patch.

here is an example of what the hoisted methods look like

user> (println (disassemble (fn ^long [^long y] (let [x (try (/ 1 0) (catch Throwable t 0))] x))))                                                                       

// Compiled from form-init3851661302895745152.clj (version 1.5 : 49.0, super bit)                                                                                        
public final class user$eval1872$fn__1873 extends clojure.lang.AFunction implements clojure.lang.IFn$LL {                                                                
                                                                                                                                                                         
  // Field descriptor #9 Lclojure/lang/Var;                                                                                                                              
  public static final clojure.lang.Var const__0;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__1;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__2;                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 2, Locals: 0                                                                                                                                                 
  public static {};                                                                                                                                                      
     0  ldc <String "clojure.core"> [16]                                                                                                                                 
     2  ldc <String "/"> [18]                                                                                                                                            
     4  invokestatic clojure.lang.RT.var(java.lang.String, java.lang.String) : clojure.lang.Var [24]                                                                     
     7  checkcast clojure.lang.Var [26]                                                                                                                                  
    10  putstatic user$eval1872$fn__1873.const__0 : clojure.lang.Var [28]                                                                                                
    13  lconst_1    
    14  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    17  putstatic user$eval1872$fn__1873.const__1 : java.lang.Object [36]                                                                                                
    20  lconst_0                                                                                                                                                         
    21  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    24  putstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    27  return                                                                                                                                                           
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 1, Locals: 1                                                                                                                                                 
  public user$eval1872$fn__1873();                                                                                                                                       
    0  aload_0 [this]                                                                                                                                                    
    1  invokespecial clojure.lang.AFunction() [41]                                                                                                                       
    4  return                                                                                                                                                            
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #43 (J)J                                                                                                                                          
  // Stack: 2, Locals: 4  
  public final long invokePrim(long y);                                                                                                                                  
     0  lload_1 [y]                                                                                                                                                      
     1  invokestatic user$eval1872$fn__1873.__hoisted1874(long) : java.lang.Object [47]                                                                                  
     4  astore_3 [x]                                                                                                                                                     
     5  aload_3 [x]                                                                                                                                                      
     6  aconst_null                                                                                                                                                      
     7  astore_3                                                                                                                                                         
     8  checkcast java.lang.Number [50]                                                                                                                                  
    11  invokevirtual java.lang.Number.longValue() : long [54]                                                                                                           
    14  lreturn                                                                                                                                                          
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 5, pc: 8] local: x index: 3 type: java.lang.Object                                                                                                          
        [pc: 0, pc: 14] local: this index: 0 type: java.lang.Object                                                                                                      
        [pc: 0, pc: 14] local: y index: 1 type: long                                                                                                                     
                                                                                                                                                                         
  // Method descriptor #59 (Ljava/lang/Object;)Ljava/lang/Object;                                                                                                        
  // Stack: 5, Locals: 2                                                                                                                                                 
  public java.lang.Object invoke(java.lang.Object arg0);                                                                                                                 
     0  aload_0 [this]                                                                                                                                                   
     1  aload_1 [arg0]                                                                                                                                                   
     2  checkcast java.lang.Number [50]                                                                                                                                  
     5  invokestatic clojure.lang.RT.longCast(java.lang.Object) : long [63]                                                                                              
     8  invokeinterface clojure.lang.IFn$LL.invokePrim(long) : long [65] [nargs: 3]                                                                                      
    13  new java.lang.Long [30]                                                                                                                                          
    16  dup_x2                                                                                                                                                           
    17  dup_x2                                                                                                                                                           
    18  pop                                                                                                                                                              
    19  invokespecial java.lang.Long(long) [68]                                                                                                                          
    22  areturn                                                                                                                                                          
                                                                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #45 (J)Ljava/lang/Object;                                                                                                                         
  // Stack: 4, Locals: 4                                                                                                                                                 
  public static java.lang.Object __hoisted1874(long arg0);                                                                                                               
     0  lconst_1                                                                                                                                                         
     1  lconst_0                                      
                                                                                                                                                        
     2  invokestatic clojure.lang.Numbers.divide(long, long) : java.lang.Number [74]                                                                                     
     5  astore_2                                                                                                                                                         
     6  goto 17                                                                                                                                                          
     9  astore_3 [t]                                                                                                                                                     
    10  getstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    13  astore_2                                                                                                                                                         
    14  goto 17                                                                                                                                                          
    17  aload_2                                                                                                                                                          
    18  areturn                                                                                                                                                          
      Exception Table:                                                                                                                                                   
        [pc: 0, pc: 6] -> 9 when : java.lang.Throwable                                                                                                                   
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
        [pc: 2, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 9, pc: 14] local: t index: 3 type: java.lang.Object                                                                                                         
        [pc: 0, pc: 18] local: y index: 1 type: java.lang.Object                                                                                                         
                                                                                                                                                                         
}                           

Comment by Kevin Downey [ 27/Aug/15 9:43 PM ]

there is still an issue with patch 7, (defn f [^long y] (let [x (try (+ 1 0) (catch Throwable t y))] x)) causes a verifier error

Comment by Nicola Mometto [ 28/Aug/15 11:41 AM ]

Patch 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch is patch hoistedmethod-pass-7 with the following changes:

  • fixes the bug mentioned in the last comment by Kevin Downey
  • removes unnecessary whitespace and indentation changes
  • conforms indentation with the surrounding lines
  • squashes the commits into one as preferred by Rich

Authorship is maintained for Kevin Downey

Comment by Kevin Downey [ 28/Aug/15 5:40 PM ]

maybe we need a "Bronsa's Guide to Submitting Patches" to supplement http://dev.clojure.org/display/community/Developing+Patches, I had no idea a single commit was preferred, but that makes sense given the format, although I just noticed the bit on deleting old patches to avoid confusion. Is there a preferred format for patch names too?
If you have recommendations beyond patch formatting into the code itself I am all ears.

Comment by Nicola Mometto [ 28/Aug/15 6:08 PM ]

Kevin, having a single commit per patch is something that I've seen Rich and Alex ask for in a bunch of tickets, as I guess it makes it easier to evaluate the overall diff (even though it sacrifices granularity of description).
No idea for a preferred patch name, I find it hard to imagine it would matter – I just use whatever git format patch outputs.

One thing I personally prefer is to add the ticket name at the beginning of the commit message, it makes it easier to understand changes when using e.g. git blame

Comment by Andy Fingerhut [ 28/Aug/15 6:33 PM ]

Just now I added a suggestion to http://dev.clojure.org/display/community/Developing+Patches that one read their patches before attaching them, and remove any spurious white space changes. Also to consider submitting patches with a single commit, rather than ones broken up into multiple commits, as reviewers tend to prefer those.

Alex Miller recently edited that page with the note about putting the ticket id first in the commit comment.

Only preference on patch file names is the one on that page – that they end with '.patch' or '.diff', because Rich's preferred editor for reading them recognizes those suffixes and displays the file in a patch-specific mode, I would guess.

Comment by Nicola Mometto [ 28/Aug/15 6:38 PM ]

0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch is the same as 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch but it changes some indentation to avoid mixing tabs and spaces





[CLJ-1119] inconsistent behavior of lazy-seq w/ macro & closure on excptions Created: 03/Dec/12  Updated: 28/Aug/15

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

Type: Defect Priority: Minor
Reporter: Hank Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

lazy-seq seems to evaluate inconsistently when body includes a macro and throws and exception. 1st evalutation throws the exceptions, subsequent ones return empty sequence.

demo code:

(defn gen-lazy []
  (let [coll [1 2 3]]
    (lazy-seq
      (when-let [s (seq coll)]
        (throw (Exception.))))))

(def lazy (gen-lazy))

(try
  (println "lazy:" lazy)
  (catch Exception ex
    (println ex)))

(try
  (println "lazy, again:" lazy)
  (catch Exception ex
    (println ex)))

It should throw an exception both times but only does on 1st. Generally speaking an expression shouldn't evaluate to different things depending on whether it's been evaluated before.

When removing the closure ...

(defn gen-lazy []
  (lazy-seq
    (when-let [s (seq [1 2 3])]
      (throw (Exception.)))))

... or removing the when-let macro ...

(defn gen-lazy []
  (let [coll [1 2 3]]
    (lazy-seq
      (seq coll)
      (throw (Exception.)))))

It works i.e. consistently throws the exception, so seems to be some interaction between the closure and the macro at work here. This particular combination is used in the 'map' function.

See also: https://groups.google.com/forum/?fromgroups=#!topic/clojure/Z3EiBUQ7Inc



 Comments   
Comment by Hank [ 03/Dec/12 4:26 AM ]

N.B. The primary use case I have for this, in case it matters, is interrupting the evaluation of a 'map' expression in which the mapped fn is slow to evaluate (throwing an InterruptedException), because I am not interested in its result any more. Then later I re-evaluate it because I am interested in the result after all, however with above bug the lazy sequence terminates instead of recommencing where it left off.

(UPDATE: This use case is similar to the kind of ersatz continuations that Jetty does (RetryRequest runtime exception) or even Clojure itself when barging STM transactions with the RetryEx exception.)

Comment by Hank [ 03/Dec/12 8:45 AM ]

Related to CLJ-457 according to Christophe. His patch fixes this,too.

Comment by Hank [ 04/Dec/12 5:02 AM ]

Sorry Christophe's patch doesn't work for me here. It avoids evaluating the LazySeq a second time by prematurely throwing an exception. However a LazySeq might evaluate properly the second time around b/c the situation causing the exception was transient. As per comment above an evaluation might get interrupted, throwing InterruptedException the first time around but not the second time.

Also the observation with the closure and macro need explanation IMHO.

Comment by Hank [ 08/Dec/12 3:51 AM ]

further insight: 'delay' exhibits the same behavior and is a more simple case to examine. the macro suspicion is a red herring: as demoed below it is actually the closed over variable magically turns to nil, the when-let macro simply turned that into a nil for the whole expression.

(def delayed
  (let [a true]
    (delay
      (print "a=" a)
      (throw (Exception.)))))

(try
  (print "delayed 1:")
  (force delayed)
  (catch Exception ex (println ex)))

(try
  (print "delayed 2:")
  (force delayed)
  (catch Exception ex (println ex)))

prints:

delayed 1:a= true#<Exception java.lang.Exception>
delayed 2:a= nil#<Exception java.lang.Exception>

Comment by Hank [ 09/Dec/12 1:31 AM ]

The above leads to dodgy outcomes such as: The following expression leads to an Exception on 1st evaluation and to "w00t!" on subsequent ones.

(def delayed
  (let [a true]
    (delay
      (if a
        (throw (Exception.))
        "w00t!"))))

Try it like this:

(try
  (print "delayed 1:" )
  (println (force delayed))
  (catch Exception ex (println ex)))

(try
  (print "delayed 2:")
  (println (force delayed))
  (catch Exception ex (println ex)))

Results in:
delayed 1:#<Exception java.lang.Exception>
delayed 2:w00t!

This code shows that the problem is tied to the :once flag as suspected.

(def test-once
  (let [a true]
    (^{:once true} fn* foo []
	    (println "a=" a)
	    (throw (Exception.)))))

Invoking the fn twice will show 'a' turning from 'true' to 'nil', try it like this:

(try
  (print "test-once 1:")
  (test-once)
  (catch Exception ex (println ex)))

(try
  (print "test-once 2:")
  (test-once)
  (catch Exception ex (println ex)))

Results in:
test-once 1:a= true
#<Exception java.lang.Exception>
test-once 2:a= nil
#<Exception java.lang.Exception>

That doesn't happen when the ^{:once true} is removed. Now one could argue that above fn is invoked twice, which is exactly what one is not supposed to do when decorated with the :once flag, but I argue that an unsuccessful call doesn't count as invocation towards the :once flag. The delay and lazy-seq macros agree with me there as the resulting objects are not considered realized (as per realized? function) if the evaluation of the body throws an exception, and realization/evaluation of the body is therefore repeated on re-evaluation of the delay/lazy-seq.

Try this using

(realized? delayed)
after the first evaluation in the code above. In the implementation this can be seen e.g. here for clojure.lang.Delay (similarly for LazySeq), the body-fn is set to null (meaning realized) after the invocation returns successfully only.

The :once flag affects this part of the compiler only. Some field is set to nil there in the course of a function invocation, for the good reason of letting the garbage compiler collect objects, however this should to be done after the function successfully completes only. Can this be changed?

Comment by Hank [ 16/Dec/12 4:02 AM ]

A workaround for the case of the 'map' function as described in the 1st comment, works as this: The original map function, if you take out the cases for several colls, the performance enhancements for chunked seqs and forcing the coll argument to a seq, looks like this:

(defn map [f s]
  (lazy-seq
    (cons (f (first s)) (map f (rest s)))))

In my workaround I evaluate f twice:

(defn map [f s]
  (lazy-seq
    (f (first s))
    (cons (f (first s)) (map f (rest s)))))

Because the downstream functions that are slow to evaluate are all of the deref kind that cache their result (more lazy-seqs, delays, futures, promises), the InterruptedException can only happen during the 1st evaluation, while the tail call optimization that sets closed-over variables to nil (it pays to read this here: http://clojure.org/lazy) only happens on the second call. The first still creates an fn that captures the head of the sequence 's', however this is not being held onto as it is not returned.

I use this special version of map (and other, similarly rewritten functions based on lazy-seq such as iterate) when I want interruptible, restartable seq evaluations.

Comment by Hank [ 06/Aug/13 9:06 AM ]

Instead of above hack, implementing map and all other combinators using a lazy Y-combinator, and removing the :once meta-data tag in the lazy-seq definition fixes things properly. Since the compiler sort-of-hack that clears closed-over variables that's triggered by the :once meta-data tag is basically only for cases of recursion that can be implemented using the Y-combinator, that won't be needed anymore either.

Comment by Alex Miller [ 08/Aug/13 3:20 AM ]

Re the Delay reference above, this is covered (and addressed) in CLJ-1175 which is waiting for a final screen.





[CLJ-1435] 'numerator and 'denominator fail to handle integral values (i.e. N/1) Created: 30/May/14  Updated: 27/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: None

Approval: Triaged

 Description   

Because ratio values reduce to lowest terms and, for integral values where the lowest term is N/1, are auto-converted to BigInts (and formerly Longs), the current behavior of clojure.core/numerator and clojure.core/denominator yield unexpected results.

user=> (numerator 1/3)
1
user=> (numerator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3306)
user=> (denominator 1/3)
3
user=> (denominator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/denominator (core.clj:3314)
user=>

The auto-conversion to Longs is not really the problem in my mind. I'd like to see numerator return the original value when presented with a BigInt and denominator always return 1 when presented with a BigInt. It seems reasonable to request the same for Longs.

If desired, I'd be happy to produce a patch.



 Comments   
Comment by Andy Fingerhut [ 30/May/14 6:35 PM ]

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

Comment by Aaron Brooks [ 30/May/14 7:44 PM ]

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.

Comment by Gary Fredericks [ 27/Aug/15 10:38 AM ]

I've definitely written the helper functions Andy describes on several occasions.





[CLJ-1803] Enable destructuring of sequency maps Created: 22/Aug/15  Updated: 26/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring

Attachments: Text File 0001-Enable-destructuring-of-seq-map-types.patch    
Patch: Code
Approval: Triaged

 Description   

At present, Clojure's destructuring implementation will create a hash-map from any encountered value satisfying clojure.core/seq? This has the I argue undesirable side effect of making it impossible to employ destructuring on a custom Associative type which is also a Seq. This came up when trying to destructure instances of a tagged value class which for the purpose of pattern matching behave as [k v] seqs, but since the v is known to be a map, are also associative on the map part so as to avoid the syntactic overhead of updates preserving the tag.

;; A sketch of such a type
(deftype ATaggedVal [t v]
  clojure.lang.Indexed
  (nth [self i]    (nth self i nil))
  (nth [self i o] (case i (0) t (1) v o))

  clojure.lang.Sequential
  clojure.lang.ISeq
  (next  [this]     (seq this))
  (first [this]     t)
  (more  [this]     (.more (seq this)))
  (count [this]     2)
  (equiv [this obj] (= (seq this) obj))
  (seq   [self]     (cons t (cons v nil)))

  clojure.lang.Associative
  (entryAt [self key] (.entryAt v key))
  (assoc [_ sk sv]    (ATaggedVal. t (.assoc v sk sv)))

  clojure.lang.ILookup
  (valAt [self k]   (.valAt v k))
  (valAt [self k o] (.valAt v k o))

  clojure.lang.IPersistentMap
  (assocEx [_ sk sv] (ATaggedVal. t (.assocEx v sk sv)))
  (without [_ sk]    (ATaggedVal. t (.without v sk))))

So using such a thing,

(let [{:keys [x]} (ATaggedVal. :foo {:x 3 :y 4})] x)
;; expect 3
=> nil

Since for any type T such that clojure.core/get will behave, T should satisfy clojure.core/map? it should be correct simply to change the behavior of destructure to only build a hash-map if map? isn't already satisfied.

The attached patch makes this change.



 Comments   
Comment by Alex Miller [ 22/Aug/15 1:21 PM ]

Probably worth watching CLJ-1778 too which might cause this not to apply anymore.

Could you add an example of what doesn't work to the description?

Comment by Ragnar Dahlen [ 24/Aug/15 1:20 AM ]

The current patch for CLJ-1778 does not address this issue.

The idea seems sound to me, if we're map destructuring a value that's
already a map (as determined by map?), we don't need to create a
new one by calling seq and HashMap/create, unless there's a
really good reason it should be exactly that map implementation (I
don't see one).

I don't think the current patch is OK though as it makes an
(unneccessary) breaking change to the behaviour of map destructuring.
Previously, destructuring a non-seqable value returned nil, but with
patch, seq is always called on value and for non-seqble types this
will instead throw an exception. It should be trivial to change the
patch to retain the original behaviour.

1.8.0-master:

user=> (let [{:keys [x]} (java.util.Date.)] x)
nil

with 0001-Enable-destructuring-of-seq-map-types.patch:

user=> (let [{:keys [x]} (java.util.Date.)] x)
IllegalArgumentException Don't know how to create ISeq from: java.util.Date  clojure.lang.RT.seqFrom (RT.java:528)
Comment by Reid McKenzie [ 26/Aug/15 1:15 PM ]

I contend that the behavior broken is, at best, undefined behavior consequent from the implementation and that the failure to cast exception is at least clearer than the silent nil behavior of the original implementation.

I would personally prefer to extend the destructuring checks logically to (cond map? x seq? (hash-map (seq x)) :else (throw "Failed to destructure non-map type") but I think that change is sufficiently large that it would meaningfully decrease the chances of this patch being accepted.





[CLJ-1804] take transducer optimization Created: 25/Aug/15  Updated: 26/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Karsten Schmidt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, transducers

Attachments: Text File clj-1804-1.patch     Text File clj-1804-2.patch    
Patch: Code
Approval: Triaged

 Description   

A basic refactoring to remove the let form and only requires a single counter check for each iteration, yields an 25% performance increase. With the patch, 2 checks are only required for the last iteration (in case counter arg was <= 0)...

;; master
(quick-bench (into [] (take 1000) (range 2000)))
WARNING: Final GC required 34.82584189073624 % of runtime
Evaluation count : 13050 in 6 samples of 2175 calls.
             Execution time mean : 46.921254 µs
    Execution time std-deviation : 1.904733 µs
   Execution time lower quantile : 45.124921 µs ( 2.5%)
   Execution time upper quantile : 49.427201 µs (97.5%)
                   Overhead used : 2.367243 ns

;; w/ patch
(quick-bench (into [] (take 1000) (range 2000)))
WARNING: Final GC required 34.74448252054369 % of runtime
Evaluation count : 18102 in 6 samples of 3017 calls.
             Execution time mean : 34.301193 µs
    Execution time std-deviation : 1.714105 µs
   Execution time lower quantile : 32.341349 µs ( 2.5%)
   Execution time upper quantile : 37.046851 µs (97.5%)
                   Overhead used : 2.367243 ns


 Comments   
Comment by Karsten Schmidt [ 25/Aug/15 10:35 AM ]

Proposed patch, passes all existing tests

Comment by Alex Miller [ 25/Aug/15 11:28 AM ]

From a superficial skim, I see checks for pos? and then neg? which both exclude 0 and that gives me doubts about that branch. That may actually be ok but I will have to read it more closely.

Comment by Karsten Schmidt [ 25/Aug/15 11:58 AM ]

Hi Alex, try running the tests... AFAICT it's all still working as advertised: For (take 0) or (take -1) then the pos? check fails, but we must ensure to not call rf for that iteration. For all other (take n) cases only the pos? check is executed apart from the last iteration (which causes a single superfluous neg? call) The current path/impl always does 2 checks for all iterations and hence is (much) slower.

Comment by Alex Miller [ 25/Aug/15 7:22 PM ]

The only time that neg? case will be hit is if take is passed n <= 0, so I think this is correct. However, you might consider some different way to handle that particular case - for example, it could be pulled out of the transducer function entirely and be created as a separate transducer function entirely. I'm not sure that's worth doing, but it's an idea.

Comment by Karsten Schmidt [ 26/Aug/15 6:01 AM ]

Good idea, Alex! This 2nd patch removes the neg? check and adds a quick bail transducer for cases where n <= 0. It also made it slightly faster still:

(quick-bench (into [] (take 1000) (range 2000)))
Evaluation count : 20370 in 6 samples of 3395 calls.
             Execution time mean : 30.302673 µs

(now ~35% faster than original)





[CLJ-1671] Clojure socket server Created: 09/Mar/15  Updated: 25/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File clj-1671-10.patch     Text File clj-1671-11.patch     Text File clj-1671-2.patch     Text File clj-1671-3.patch     Text File clj-1671-4.patch     Text File clj-1671-5.patch     Text File clj-1671-6.patch     Text File clj-1671-7.patch     Text File clj-1671-8.patch     Text File clj-1671-9.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Programs often want to provide REPLs to users in contexts when a) network communication is desired, b) capturing stdio is difficult, or c) when more than one REPL session is desired. In addition, tools that want to support REPLs and simultaneous conversations with the host are difficult with a single stdio REPL as currently provided by Clojure.

Tooling and users often need to enable a REPL on a program without changing the program, e.g. without asking author or program to include code to start a REPL host of some sort. Thus a solution must be externally and declaratively configured (no user code changes). A REPL is just a special case of a socket service. Rather than provide a socket server REPL, provide a built-in socket server that composes with the existing repl function.

For design background, see: http://dev.clojure.org/display/design/Socket+Server+REPL

Start a socket server by supplying an extra system property (classpath and clojure.main here, but this would generally be starting your own app instead - we won't use the repl it starts):

java -cp target/classes -Dclojure.server.repl="{:port 5555 :accept clojure.core.server/repl}" clojure.main

where options are:

  • address = host or address, defaults to loopback
  • port = port, required
  • accept = namespaced function to invoke on socket accept, required
  • args = sequential collection of args to pass to accept
  • bind-err = defaults to true, binds err to out stream
  • server-daemon = defaults to true, socket server thread doesn't block exit
  • client-daemon = defaults to true, socket client threads don't block exit

Run a repl client using telnet:

$ telnet 127.0.0.1 5555
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
user=> (println "hello")
hello
nil
user=> clojure.core.server/*session*
{:server "repl", :client "1"}
user=> (ns foo)
nil
foo=> (+ 1 1)
2
foo=>

Now open a 2nd telnet client and "attach" to the session of the first client:

telnet 127.0.0.1 5555
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
user=> :repl/sessions
[{:server "repl", :client "1"} {:server "repl", :client "2"}]
user=> :repl/attach {:server "repl", :client "1"}
true
1|foo=> *ns*
#object[clojure.lang.Namespace 0x6023edaa "foo"]
1|foo=> *1
2
1|foo=> :repl/detach
true
user=> *1
true
user=> :repl/quit
Connection closed by foreign host.

Note that when the session is attached, the prompt changes, showing the client id and namespace of the attached session instead. When attached to another session, your expressions are evaluated in the context of the attached session, so you can grab the current namespace or even the *1/*2/*3 results. This is an example of how a tool could attach and be "inside" the context of the user's repl.

Patch: clj-1671-11.patch



 Comments   
Comment by Timothy Baldridge [ 09/Mar/15 5:50 PM ]

Could we perhaps keep this as a contrib library? This ticket simply states "The goal is to provide a simple streaming socket repl as part of Clojure." What is the rationale for the "part of Clojure" bit?

Comment by Alex Miller [ 09/Mar/15 7:33 PM ]

We want this to be available as a Clojure.main option. It's all additive - why wouldn't you want it in the box?

Comment by Timothy Baldridge [ 09/Mar/15 10:19 PM ]

It never has really been too clear to me why some features are included in core, while others are kept in contrib. I understand that some are simply for historical reasons, but aside from that there doesn't seem to be too much of a philosophy behind it.

However it should be noted that since patches to clojure are much more guarded it's sometimes nice to have certain features in contrib, that way they can evolve with more rapidity than the one release a year that clojure has been going through.

But aside from those issues, I've found that breaking functionality into modules forces the core of a system to become more configurable. Perhaps I would like to use this repl socket feature, but pipe the data over a different communication protocol, or through a different serializer. If this feature were to be coded as a contrib library it would expose extension points that others could use to add additional functionality.

So I guess, all that to say, I'd prefer a tool I can compose rather than a pre-built solution.

Comment by Rich Hickey [ 10/Mar/15 6:25 AM ]

Please move discussions on the merits of the idea to the dev list. Comments should be about the work of resolving the ticket, approach taken by the patch, quality/perf issues etc.

Comment by Colin Jones [ 11/Mar/15 1:33 PM ]

I see that context (a) of the rationale is that network communication is desired, which sounds to me like users of this feature may want to communicate across hosts (whether in VMs or otherwise). Is that the case?

If so, it seems like specifying the address to bind to (e.g. "0.0.0.0", "::", "127.0.0.1", etc.) may become important as well as the existing port option. This way, someone who wants to communicate across hosts (or conversely, lock down access to local-only) can make that decision.

Comment by Alex Miller [ 11/Mar/15 2:07 PM ]

Colin - agreed. There are many ways to potentially customize what's in there so we need to figure out what's worth doing, both in the function and via the command line.

I think address is clearly worth having via the function and possibly in the command line too.

Comment by Kevin Downey [ 11/Mar/15 5:49 PM ]

I find the exception printing behavior really odd. for a machine you want an exception as data, but you also want some indication of if the data is an error or not, for a human you wanted a pretty printed stacktrace. making the socket repl default to printing errors this way seems to optimize for neither.

Comment by Rich Hickey [ 12/Mar/15 12:29 PM ]

Did you miss the #error tag? That indicates the data is an error. It is likely we will pprint the error data, making it not bad for both purposes

Comment by Alex Miller [ 13/Mar/15 11:29 AM ]

New -4 patch changes:

  • clojure.core/throwable-as-map now public and named clojure.core/Throwable->map
  • catch and ignore SocketException without printing in socket server repl (for client disconnect)
  • functions to print as message and as data are now: clojure.main/err-print and clojure.main/err->map. All defaults and docs updated.
Comment by David Nolen [ 18/Mar/15 12:44 PM ]

Is there any reason to not allow supplying :eval in addition to :use-prompt? In the case of projects like ClojureCLR + Unity eval generally must happen on the main thread. With :eval as something which can be configured, REPL sessions can queue forms to be eval'ed with the needed context (current ns etc.) to the main thread.

Comment by Kevin Downey [ 20/Mar/15 2:12 PM ]

I did see the #error tag, but throwables print with that tag regardless of if they are actually thrown or if they are just the value returned from a function. Admittedly returning exceptions as values is not something generally done, but the jvm does distinguish between a return value and a thrown exception. Having a repl that doesn't distinguish between the two strikes me as an odd design. The repl you get from clojure.main currently prints the message from a thrown uncaught throwable, and on master prints with #error if you have a throwable value, so it distinguishes between an uncaught thrown throwable and a throwable value. That obviously isn't great for tooling because you don't get a good data representation in the uncaught case.

It looks like the most recent patch does pretty print uncaught throwables, which is helpful for humans to distinguish between a returned value and an uncaught throwable.

Comment by Kevin Downey [ 25/Mar/15 1:10 PM ]

alex: saying this is all additive, when it has driven changes to how things are printed, using the global print-method, rings false to me

Comment by Sam Ritchie [ 25/Mar/15 1:15 PM ]

This seems like a pretty big last minute addition for 1.7. What's the rationale for adding it here vs deferring to 1.8, or trying it out as a contrib first?

Comment by Alex Miller [ 25/Mar/15 2:13 PM ]

Kevin: changing the fallthrough printing for things that are unreadable to be readable should be useful regardless of the socket repl. It shouldn't be a change for existing programs (unless they're relying on the toString of objects without print formats).

Sam: Rich wants it in the box as a substrate for tools.

Comment by Alex Miller [ 26/Mar/15 10:03 AM ]

Marking incomplete, pending at least the repl exit question.

Comment by Laurent Petit [ 29/Apr/15 2:18 PM ]

Hello, I intend to work on this, if it appears it still has a good probability of being included in clojure 1.7.
There hasn't been much visible activity on it lately.
What is the current status of the pending question, and do you think it will still make it in 1.7?

Comment by Alex Miller [ 29/Apr/15 2:29 PM ]

This has been pushed to 1.8 and is on my plate. The direction has diverged quite a bit from the original description and we don't expect to modify clojure.main as is done in the prior patches. So, I would recommend not working on it as described here.

Comment by Laurent Petit [ 01/May/15 7:24 AM ]

OK thanks for the update.

Is the discussion about the new design / goal (you say the direction has diverged) available somewhere so that I can keep in touch with what the Hammock Time is producing? Because on my own hammock time I'm doing some mental projections for CCW support of this, based on what is publicly available here -

Also, as soon as you have something available for testing please don't hesitate to ping me, I'll see what I can do to help depending on my schedule. Cheers.

Comment by Alex Miller [ 01/May/15 8:44 AM ]

Some design work is here - http://dev.clojure.org/display/design/Socket+Server+REPL.

Comment by Laurent Petit [ 05/May/15 11:41 AM ]

Thanks for the link. It seems that the design is totally revamped indeed. Better to wait then.

Comment by Andy Fingerhut [ 04/Jul/15 1:21 PM ]

Alex, just a note that the Java method getLoopbackAddress [1] appears to have been added with Java 1.7, so your patches that use that method do not compile with Java 1.6. If the plan was for the next release of Clojure to drop support for Java 1.6 anyway, then no worries.

[1] http://docs.oracle.com/javase/7/docs/api/java/net/InetAddress.html#getLoopbackAddress%28%29

Comment by Stuart Halloway [ 08/Jul/15 8:31 AM ]

Lifecycle concerns

1. atoms are weak when actions (starting threads / sockets) are coordinated with recorded state (the map)
2. I see why the plumbing needs access to socket, but what is the motivation for expoosing it to outside code, seems like opportunity to break stuff
3. stating a server has a race condition
4. what happens if somebody wants to call start-server explicitly – how do they know whether that happens before or after config-driven process launches
5. what guarantees about when config-driven launch happens, vis-a-vis other startup-y things
6. is there a use for stop-servers other than shutdown?
7. does Clojure now need a shutdown-clojure-resources? I don't want to have to remember shutdown-agents, plus stop-servers, plus whatever we add next year
8. what happens on misconfiguration (e.g. nonexistent namespace)? will other servers still launch? what thread dies, and where does is report? will the app main process die before even reaching the user?

Comment by Alex Miller [ 08/Jul/15 9:12 AM ]

1. it's not in the patch, but the intention is that in the runtime on startup there is a call to (start-servers (System/getProperties)) and generally you're not starting servers on the fly (although it's broken out to make that possible).
2. just trying to make resources available, not sure how much locking down we want/need
3. I'm expecting this to be a startup thing primarily
4. I'm assuming the config-driven process will start in the runtime and thus it will happen first. Not sure how much we need to support the manual stuff - it just seemed like a good idea to make it possible.
5. dunno, haven't looked at where to do that yet. Probably somewhere similar to data_readers stuff?
6. no
7. the threads are daemons by default meaning that it will shut down regardless. If you set the daemon properties to false, then you're in control and need to call stop-servers where it's appropriate.
8. I thought about these questions and do not have good answers. Lots more of that stuff needs to be handled.

Comment by Alex Miller [ 10/Aug/15 10:55 AM ]

More feedback forwarded from Stu:

1. Need to switch from atom to locking around the server/client state map.
2. It is not clear to me exactly what gets pushed by :repl/push – what does "like ns" mean?
3. :repl/quit feels categorically different from push and pull. People may come to expect this on all Clojure REPLs, should we install it at the bottom instead of in the REPL server?
4. The design anticipates the possibility of using multiple connections sharing state to implement a higher level "session". I think we should implement an example of this, both to flesh out the idea and as an example for users.
5. I am opposed to adding variety to the config options more work for unclear payoff and would prefer to leave the file-based option and REPL convenience as a possibility for later
6. We have one name too few - it doesn't make sense for client id to be bound to a tuple that includes server-name and client id the word 'coordinates' has been used in some places for the bigger thing
7. Explicitly enumerate where exceptions end up may be sufficient to document that UncaughtExceptionHandler gets all of them
8. Should programs have direct access to sockets? pro: more control (e.g. design says you can grab socket from server state map and close it). con: leaking abstraction

Comment by Alex Miller [ 10/Aug/15 11:22 AM ]

1. Updating patch (but geez does this feel weird to do in Clojure)
2. I'm not entirely sure what info a tooling connection needs about a user's state. *ns* is an obvious one. Maybe *e or the other repl vars too? In some ways this is a similar problem to binding conveyance - giving one thread another's dynamic "state".
3. Agreed on :repl/quit. I had a version of this patch that actually added the "command" capability to the base repl and installed :repl/quit as a default command. I think that's a better answer but I did it this way to make the patch additive and easier to evaluate, idea-wise. So, in other words, agreed and that's do-able. Repl invokers would then need a way to install new commands, which the socket repl could do.
4. I think the "session" notion is a more useful way of saying this than I have done so far. With the benefit of time, I'm not happy with the push/pull stuff and I don't think it really solves the problem. In particular, it would be better if no explicit actions were required for the "push" part. I will do some more design thinking on this on the design page.
5. ok
6. Again, bound up in the push/pull stuff and maybe not necessary.
7. Will add to design page.
8. Given the locking and other stuff, I think I'd now say no - there should be an API for closing etc but not direct access to these.

Comment by Laurent Petit [ 14/Aug/15 4:09 AM ]

Just to check, I'd like to understand if we share the same detail view of how the `:accept` key will work.

Its default value is `clojure.repl/repl`.

I expect to be able to specify a value like `my.not.yet.loaded.namespace/my-repl-fn` and that the server code will try to require `my.not.yet.loaded.namespace` from the java classpath.

Also, when will the resolution of `:accept` take place? Before or after all the -e and -i expressions / files have been evaluated? That is, will it be possible to load `my.not.yet.loaded.namespace` via a -i option or in the list of files to load, instead of having to tweak the classpath? (Some tools / IDEs abstracting away the notion of classpath, program arguments, jvm parameters make it difficult if possible at all to tweak the classpath, but really easy to add JVM parameters / program arguments before launching.

Comment by Alex Miller [ 14/Aug/15 8:11 AM ]

Hey Laurent,

Keep in mind this is a work in progress still. The idea here is that you can pass these system properties when running ANY Clojure program. The Clojure runtime will scan system properties and start socket servers as needed (that hook is not yet installed).

So you can do clojure.main if you like but if you're passing the server system properties the socket server will be launched. I have not determined yet exactly where that RT loading will happen but I would expect it to be prior to the -i or -e being evaluated.

Comment by Laurent Petit [ 14/Aug/15 9:08 AM ]

Sure, I know this is WIP, and that's why I find it valuable to add my 2 cents now, rather that when things are almost wrapped up.

So it will be triggered not by the fact that clojure.main is invoked, but during the initialization of the clojure runtime ?

Then it seems indeed difficult to ask to wait for clojure.main to have finished starting.

My first remark still stands, though:

  • the fully qualified var symbol String passed as value of `:accept` parameter should not require that its namespace be already loaded, or this will really limit to choice of vars that can be passed on. Some dynamic `require` call and var `resolve` should be taking place here.
Comment by Alex Miller [ 14/Aug/15 9:43 AM ]

Oh sorry, I thought you were just confirming what was already there - the current patch does execute a require for the namespace containing the accept function.

Comment by Alex Miller [ 19/Aug/15 3:27 PM ]

The -8 patch integrates the server startup into the Clojure runtime. It also improves validation and error-handling on the :repl commands and attached sessions now report the attached session ns as the repl prompt and properly allow access to *1, *2, *3.

Comment by Alex Miller [ 19/Aug/15 3:27 PM ]

Oh, and I got rid of the use of getLoopbackAddress() so this works in jdk 1.6.

Comment by Alex Miller [ 25/Aug/15 9:51 AM ]

The -10 patch defers requiring the accept ns until the socket makes a connection - this should allow a lot more freedom for the app to initialize code as needed.

Comment by Ragnar Dahlen [ 25/Aug/15 10:14 AM ]

Two comments on latest patch:

  • Swallowing the exception without notice in stop-server might be confusing for a user. If stop-server actually fails (but seemingly succeeds), calling start-server might fail too because port is already open, for example, but as a user it would be hard to understand why.
  • Would be helpful document return values in public API (if there is one), for example start-server/stop-server
Comment by Alex Miller [ 25/Aug/15 3:37 PM ]

Ragnar, good comments. I've updated the patch. That stop code was broken in a couple other ways too but should be good now.





[CLJ-1778] let-bound namespace-qualified bindings should throw (if not map destructuring) Created: 16/Jul/15  Updated: 24/Aug/15

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Ragnar Dahlen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-1778-2-with-tests.patch     Text File clj-1778.patch    
Patch: Code and Test
Approval: Screened

 Description   

Seen in a tweet...

user=> (let [a/x 42] x) ; throws CompilerException "Can't let qualified name ..."
user=> (let [a/x 42, [y] [1]] x) ;=> 42

The second one should throw like the first one (also see CLJ-1318 where support for map destructuring of namespaced symbols was added).

Approach: Rather than allowing namespaced symbols to be returned from the map destructuring (the pmap fn), convert those symbols before returning them, so that any namespaced symbols encountered in the main cond of pb are an error and can be handled uniformly.

Patch: clj-1778-2-with-tests.patch

Screened by: Alex Miller



 Comments   
Comment by Ragnar Dahlen [ 16/Jul/15 11:41 AM ]

I can confirm that this is a regression from CLJ-1318. After that change, namespaces were removed from symbols regardless of whether they were used as part of a map destruring or not. The only reason the exception is caught in the first test case is because that binding form has no destructuring at all, in which case the destructuring logic is bypassed.

I've attached a patch that moves the namespace removal (and keyword handling) into `pmap` instead as it's really a special case for map destructuring only.

Comment by Ragnar Dahlen [ 16/Jul/15 3:53 PM ]

Updated patch with two changes:

1. Removed initial check for keywords in binding keys as that only looked for keywords at top-level and failed to catch cases like:

(let [[:x] [1]] x)
;; => 1

Keywords in non-map destructuring binding positions (like the example) will now fail with "Unsupported binding form: :x" instead. This is a change from the current behaviour where only top-level keywords would be caught, but the current exception is the slightly more specific "Unsupported binding key: :x". If a better, more specific exception is required, I'd be happy to update the patch.

2. Tests: added test for regression, updated/amended existing tests.

Comment by Rich Hickey [ 24/Aug/15 9:37 AM ]

Is this the smallest change that can be made?

Comment by Ragnar Dahlen [ 24/Aug/15 4:51 PM ]

I thought so, but in writing this reply I realised a smaller change
might be possible, depending on the desired behaviour/outcome.

CLJ-1318 ("Support destructuring maps with namespaced keywords")
introduced two potentially undesired behaviours:

1. Namespaces were removed from symbols occurring in any binding key
position regardless of whether they were used in map destructuring
or not. This lead to the behaviour initially reported in this
issue; the illusion of being able to bind a qualified name in a
non-map-destructuring context:

user=> (let [a/x 1, [y] [2]] x) ;=> 1

This currently "works" because namespaces are unconditionally removed
from symbols. However:

user=> (let [a/x 42] x) ; throws CompilerException "Can't let qualified name ..."

throws because destructuring logic is bypassed if every binding key is
a symbol.

2. Keywords were allowed in any binding key positions. Keywords are
converted to symbols (retaining namespace) and treated according to
the rules of symbols in the rest of the destructuring logic. From
what I understand the idea was to allow keywords only in map
destructuring, but again the change change was effected for any
binding key. This leads the following:

(let [[:a :b :c] [1 2 3]] a) ;=> 1

A top-level check was put in place to (from my understanding) try and
prevent the usage of keywords in such positions:

(if-let [kwbs (seq (filter #(keyword? (first %)) bents))]
  (throw (new Exception (str "Unsupported binding key: " (ffirst kwbs))))
  (reduce1 process-entry [] bents))

However, this check only checks the top level binding entries, and
fails the recursive nature of destructuring (as demonstrated by
example above).

The current patch makes the following changes:

1. Revert problematic changes introduced by CLJ-1318:
1.1 revert unconditional removal of namespace from any namespaced symbol encountered
1.2 revert acceptence of keywords in any binding key position
1.3 revert insufficient check for keywords used in "illegal" positions.
2. Re-implement support for namespaced symbols, and keywords, but only
in the context of map destructuring.

If that is what we want to accomplish I believe the patch is the
smallest possible. However, if the keyword behaviour is actually
desired (basically allowing keywords being used interchangably with
symbols for binding keys) then the patch could be smaller.

Please excuse the rather lengthy reply.

Comment by Alex Miller [ 24/Aug/15 5:05 PM ]

Ragnar, I do not believe we wish to use keywords interchangeably with symbols for binding keys.

Furthermore, I think this is a good patch that solves the problems introduced in CLJ-1318 (I'll take the blame for those!).





[CLJ-1609] Fix an edge case in the Reflector's search for a public method declaration Created: 05/Dec/14  Updated: 24/Aug/15

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

Type: Defect Priority: Major
Reporter: Jeremy Heiler Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop

Attachments: Text File clj-1609-2.patch     Text File clj-1609-3.patch     Text File clj-1609-test.patch     Text File reflector_method_bug.patch    
Patch: Code and Test
Approval: Ok

 Description   
// all: package demo;

public interface IBar {
  void stuff();
}

class Bar {
  public void stuff() { System.out.println("stuff"); }
}

class SubBar extends Bar implements IBar {
  // implements IBar, via non-public superclass Bar
}

public class Factory {
  public static IBar get() { return new SubBar(); }
}

Can't invoke from Clojure:

(import '[demo Factory IBar])
(def inst (Factory/get))  ;; instance of SubBar, inferred as IBar
(.stuff inst)
;; IllegalArgumentException Can't call public method of non-public class: public void demo.Bar.stuff()  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:88)

Cause: The Reflector is not taking into account that a non-public class can implement an interface, and have a non-public parent class contain an implementation of a method on that interface.

Approach: The solution I took is to pass in the target object's class instead of the declaring class of the method object.

Patch: clj-1609-3.patch + clj-1609-test.patch

Screened by: Fogus



 Comments   
Comment by Alex Miller [ 05/Dec/14 8:48 AM ]

Probably a dupe of CLJ-259. I'll probably dupe that one to this though.

Comment by Jeremy Heiler [ 05/Dec/14 1:33 PM ]

Thanks. Sorry for not finding that myself. CLJ-259 refers to CLJ-126, which I think is covered with this patch.

Comment by Jeremy Heiler [ 06/Dec/14 12:37 PM ]

I was looking into whether or not the target could be null, and it can be when invoking a static method. However, I don't think that code path would make it to the target.getClass() because non-public static methods aren't returned by getMethods().

Comment by Stuart Halloway [ 31/Jul/15 9:24 AM ]

Jeremy,

The compile-tests phase of the build provides a point for compiling Java classes that that tests can then consume. Does that provide the hook you would need to add a test?

Comment by Alex Miller [ 07/Aug/15 9:10 AM ]

afaict, this ticket is incomplete pending a test from Stu's last comment so moving there

Comment by Alex Miller [ 18/Aug/15 4:58 PM ]

clj-1609-2.patch is identical to previous patch, just adds jira id to beginning of commit message

clj-1609-test.patch adds a reflection test that fails without the patch and passes with it.

Comment by Alex Miller [ 18/Aug/15 4:58 PM ]

should be ready to screen now

Comment by Fogus [ 21/Aug/15 12:53 PM ]

The patch and test are straight-forward and clean.

Comment by Rich Hickey [ 24/Aug/15 9:54 AM ]

Can I get some more context in this diff please?

Comment by Alex Miller [ 24/Aug/15 12:11 PM ]

Added clj-1609-3.patch which is identical to prior but has 15 lines of context instead.





[CLJ-1449] Add clojure.string functions for portability to ClojureScript Created: 19/Jun/14  Updated: 24/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Bozhidar Batsov Assignee: Nola Stowe
Resolution: Unresolved Votes: 29
Labels: ft, string

Attachments: Text File add_functions_to_strings-2.patch     Text File add_functions_to_strings-3.patch     Text File add_functions_to_strings-4.patch     Text File add_functions_to_strings-5.patch     Text File add_functions_to_strings-6.patch     Text File add_functions_to_strings-7.patch     Text File add_functions_to_strings.patch     Text File clj-1449-more-v1.patch    
Patch: Code and Test
Approval: Ok

 Description   

It would be useful if a few common functions from Java's String were available as Clojure functions for the purposes of increasing portability to other Clojure platforms like ClojureScript.

The functions below also cover the vast majority of cases where Clojure users currently drop into Java interop for String calls - this tends to be an issue for discoverability and learning. While the goal of this ticket is increased portability, improving that is a nice secondary benefit.

Proposed clojure.string fn java.lang.String method
index-of indexOf
last-index-of lastIndexOf
starts-with? startsWith
ends-with? endsWith
includes? contains

Patch:

  • clj-1449-7.patch - uses nil to indicate not-found in index-of

Performance: Tested the following with criterium for execution mean time (all times in ns).

Java Clojure Java Clojure (-4 patch) Clojure (-6 patch)
(.indexOf "banana" "n") (clojure.string/index-of "banana" "n") 8.70 9.03 9.27
(.indexOf "banana" "n" 1) (clojure.string/index-of "banana" "n" 1) 7.29 7.61 7.66
(.indexOf "banana" (int \n)) (clojure.string/index-of "banana" \n) 5.34 6.20 6.20
(.indexOf "banana" (int \n) 1) (clojure.string/index-of "banana" \n 1) 5.38 6.19 6.24
(.startsWith "apple" "a") (clojure.string/starts-with? "apple" "a") 4.84 5.71 5.65
(.endsWith "apple" "e") (clojure.string/ends-with? "apple" "e") 4.82 5.68 5.70
(.contains "baked apple pie" "apple") (clojure.string/includes? "baked apple pie" "apple") 10.78 11.99 12.17

Screened by: Stu, who prefers nil over -1 - add_functions_to_strings-6.patch

Note: In both Java and JavaScript, indexOf will return -1 to indicate not-found, so this is (at least in these two cases) the status quo. The benefit of nil return in Clojure is that it can be used as a found/not-found condition. The benefit of a -1 return is that it matches Java/JavaScript and that the return type can be hinted as a primitive long, allowing you to feed it into some other arithmetic or string operation without boxing.



 Comments   
Comment by Alex Miller [ 19/Jun/14 12:53 PM ]

Re substring, there is a clojure.core/subs for this (predates the string ns I believe).

clojure.core/subs
([s start] [s start end])
Returns the substring of s beginning at start inclusive, and ending
at end (defaults to length of string), exclusive.

Comment by Jozef Wagner [ 20/Jun/14 3:21 AM ]

As strings are collection of characters, you can use Clojure's sequence facilities to achieve such functionality:

user=> (= (first "asdf") \a)
true
user=> (= (last "asdf") \a)
false
Comment by Alex Miller [ 20/Jun/14 8:33 AM ]

Jozef, String.startsWith() checks for a prefix string, not just a prefix char.

Comment by Bozhidar Batsov [ 20/Jun/14 9:42 AM ]

Re substring, I know about subs, but it seems very odd that it's not in the string ns. After all most people will likely look for string-related functionality in clojure.string. I think it'd be best if `subs` was added to clojure.string and clojure.core/subs was deprecated.

Comment by Pierre Masci [ 01/Aug/14 5:27 AM ]

Hi, I was thinking the same about starts-with and .ends-with, as well as (.indexOf s "c") and (.lastIndexOf "c").

I read the whole Java String API recently, and these 4 functions seem to be the only ones that don't have an equivalent in Clojure.
It would be nice to have them.

Andy Fingerhut who maintains the Clojure Cheatsheet told me: "I maintain the cheatsheet, and I put .indexOf and .lastIndexOf on there since they are probably the most common thing I saw asked about that is in the Java API but not the Clojure API, for strings."
Which shows that there is a demand.

Because Clojure is being hosted on several platforms, and might be hosted on more in the future, I think these functions should be part of the de-facto ecosystem.

Comment by Andy Fingerhut [ 30/Aug/14 3:39 PM ]

Updating summary line and description to add contains? as well. I can back this off if it changes your mind about triaging it, Alex.

Comment by Andy Fingerhut [ 30/Aug/14 3:40 PM ]

Patch clj-1449-basic-v1.patch dated Aug 30 2014 adds starts-with? ends-with? contains? functions to clojure.string.

Patch clj-1449-more-v1.patch is the same, except it also replaces several Java method calls with calls to these Clojure functions.

Comment by Andy Fingerhut [ 05/Sep/14 1:02 PM ]

Patch clj-1449-basic-v1.patch dated Sep 5 2014 is identical to the patch I added recently called clj-1149-basic-v1.patch. It is simply renamed without the typo'd ticket number in the file name.

Comment by Yehonathan Sharvit [ 02/Dec/14 3:09 PM ]

What about an implementation that works also in cljs?

Comment by Bozhidar Batsov [ 02/Dec/14 3:11 PM ]

Once this is added to Clojure it will be implemented in ClojureScript as well.

Comment by Yehonathan Sharvit [ 02/Dec/14 3:22 PM ]

Great! Any idea when it will be added to Clojure?
Also, will it be automatically added to Clojurescript or someone will have to write a particular code for it.
The suggested patch relies on Java so I am curious to understand who is going to port the patch to cljs.

Comment by Bozhidar Batsov [ 02/Dec/14 3:27 PM ]

No idea when/if this will get merged. Upvote the ticket to improve the odds of this happening sooner.
Someone on the ClojureScript team will have to implement this in terms of JavaScript.

Comment by Alex Miller [ 02/Dec/14 4:01 PM ]

Some things that would be helpful:

1) It would be better to combine the two patches into a single patch - I think changing current uses into new users is a good thing to include. Also, please keep track of the "current" patch in the description.
2) Patch needs tests.
3) Per the instructions at the top of the clojure.string ns (and the rest of the functions), the majority of these functions are implemented to take the broader CharSequence interface. Similar to those implementations, you will need to provide a CharSequence implementation while also calling into the String functions when you actually have a String.
4) Consider return type hints - I'm not sure they're necessary here, but I would examine bytecode for typical calling situations to see if it would be helpful.
5) Check performance implications of the new versions vs the old with a good tool (like criterium). You've put an additional var invocation and (soon) type check in the calling path for these functions. I think providing a portable target is worth a small cost, but it would be good to know what the cost actually is.

I don't expect we will look at this until after 1.7 is released.

Comment by Andy Fingerhut [ 02/Dec/14 8:05 PM ]

Alex, all your comments make sense.

If you think a ready-and-waiting patch that does those things would improve the odds of the ticket being vetted by Rich, please let us know.

My guess is that his decision will be based upon the description, not any proposed patches. If that is your belief also, I'll wait until he makes that decision before working on a patch. Of course, any other contributor is welcome to work on one if they like.

Comment by Alex Miller [ 02/Dec/14 8:40 PM ]

Well nothing is certain of course, but I keep a special report of things I've "screened" prior to vetting that makes possible moving something straight from Triaged all the way through into Screened/Ok when Rich is able to look at them. This is a good candidate if things were in pristine condition.

That said, I don't know whether Rich will approve it or not, so it's up to you. I think the argument for portability is a strong one and complements the feature expression.

Comment by Alex Miller [ 14/May/15 8:55 AM ]

I'd love to have a really high-quality patch on this ticket to consider for 1.8 that took into account my comments above.

Also, it occurred to me that I don't think this should be called "contains?", overlapping the core function with a different meaning (contains value vs contains key). Maybe "includes?"?

Comment by Andy Fingerhut [ 14/May/15 9:14 AM ]

clojure.string already has 2 name conflicts with clojure.core, for which most people probably do something like (require '[clojure.string :as str]) to avoid that:

user=> (use 'clojure.string)
WARNING: reverse already refers to: #'clojure.core/reverse in namespace: user, being replaced by: #'clojure.string/reverse
WARNING: replace already refers to: #'clojure.core/replace in namespace: user, being replaced by: #'clojure.string/replace
nil
Comment by Alex Miller [ 14/May/15 10:05 AM ]

I'm not concerned about overlapping the name. I'm concerned about overlapping it and meaning something different, particularly vs one of the most confusing functions in core already. clojure.core/contains? is better than linear time key search. clojure.string/whatever will be a linear time subsequence match.

Comment by Alex Miller [ 14/May/15 10:18 AM ]

Ruby uses "include?" for this.

Comment by Devin Walters [ 19/May/15 4:56 PM ]

I agree with Alex's comment about the overlap. Personally, I prefer the way "includes?" reads over "include?", but IMO either one is better than adding to the "contains?" confusion.

Comment by Bozhidar Batsov [ 20/May/15 12:10 AM ]

I'm fine with `includes?`. Ruby is famous for the bad English used in its core library.

Comment by Andrew Rosa [ 17/Jun/15 12:29 PM ]

Andy Fingerhut, since you are the author of the original patch do you desire to continue the work on it? If you prefer (or even need) to focus on different stuff, I would like to tackle this issue.

Comment by Andy Fingerhut [ 17/Jun/15 1:08 PM ]

Andrew Rosa, I am perfectly happy if someone else wants to work on a revised patch for this. If you are interested, go for it! And from Alex's comments, I believe my initial patch covers such a small fraction of what he wants, do not worry about keeping my name associated with any patch you submit – if yours is accepted, my patch will not have helped you in getting there.

Comment by Nola Stowe [ 03/Jul/15 9:01 AM ]

Andrew Rosa, I am interested on working on this. Are you currently working it?

Comment by Andrew Rosa [ 03/Jul/15 12:08 PM ]

Thanks Andy Fingerhut, didn't saw the answer here. I'm happy that Nola Stowe could take this one.

Comment by Nola Stowe [ 08/Jul/15 12:46 PM ]

Updated patch to rename methods as specified and add tests.

Comment by Bozhidar Batsov [ 08/Jul/15 3:26 PM ]

A few remarks:

1. added should say 1.8, not 1.9.
2. The docstrings could be improved a bit - normally they should end with a . and refer to all the params of the function in question.

Comment by Nola Stowe [ 08/Jul/15 3:29 PM ]

Thanks Bozhidar Batsov .. I will, and I also got feedback from the slack clojure-dev group (Alex, Ghadi, AndrewHr) so I will be submitting an improved patch soon

Comment by Andy Fingerhut [ 09/Jul/15 11:57 AM ]

Also, I am not sure, but Alex Miller made this comment earlier: "Per the instructions at the top of the clojure.string ns, functions should take the broader CharSequence interface instead of String. Similar to existing clojure.string functions, you will need to provide a CharSequence implementation while also calling into the String functions when you actually have a String."

Looking at the Java classes and interfaces, one of the classes that implements CharSequence is CharBuffer, and it has no indexOf or lastIndexOf methods. If one were to call the proposed index-of or last-index-of with a CharBuffer, I believe you would get an exception for an unknown method.

I don't know if Alex's sentence is essential to any patch to be considered, but wanted to point out that it seems like it effectively means writing your own implementation of each of these methods for some of the classes implementing the CharSequence interface.

Comment by Nola Stowe [ 09/Jul/15 4:15 PM ]

Andy, yes you are right. Alex helped me with my current changes (not posted yet) and where needed I cast the value to a string so that it would have the indexOf methods.

Comment by Nola Stowe [ 10/Jul/15 7:43 AM ]

Added type hints, using StringBuffer instead of string in tests.

Comment by Nola Stowe [ 10/Jul/15 11:53 PM ]

Use critium and did some bench marking, I am not familiar enough to say "hey its good or no, too slow". I tried an optimization but it didn't make much difference.

notes here:

https://www.evernote.com/l/ABLYZbDr8ElIs6fkODAd3poBfIm_Bfr5kLo

Appreciate any feedback

Comment by Alex Miller [ 13/Jul/15 10:37 AM ]

Nola, some comments:

1) In index-of and last-index-of, I think we should use (instance? Character value) instead of char? - this will avoid a function invocation.

2) In index-of and last-index-of, in the branches where you know you have a Character, we should just invoke the proper function on the character instance directly rather than calling int. In this case that is (.charValue ^Character value). However, this function returns char so you still want the ^int hint.

3) In index-of and last-index-of, we should do a couple of things to maximally utilize primitive support for from-index. We should type-hint the incoming value as a ^long (only long and double are specialized cases in Clojure, not int). We should then use the function unchecked-int to optimally convert the long primitive to an int primitive.

4) We should type-hint the return value to a primitive long to avoid boxing the return.

Putting 1+2+3+4 together:

(defn index-of
  "Return index of value (string or char) in s, optionally searching forward from from-index."
  {:added "1.8"}
  (^long [^CharSequence s value]
   (if (instance? Character value)
     (.indexOf (.toString s) ^int (.charValue ^Character value))
     (.indexOf (.toString s) ^String value)))
  (^long [^CharSequence s value ^long from-index]
  (if (instance? Character value)
    (.indexOf (.toString s) ^int (.charValue ^Character value) (unchecked-int from-index))
    (.indexOf (.toString s) ^String value (unchecked-int from-index)))))

Similar changes in last-index-of.

5) In starts-with? and ends-with?, I would move the type hint for substr to the parameter declaration. This is more of a style preference on my part, no functional difference I believe.

6) On includes? I would do the same as #5, except .contains actually takes a CharSequence for the substr, so I would change the type hint from ^String to the broader ^CharSequence.

7) Can you edit the description and add a table with the "execution time mean" number for direct Java vs new Clojure fn for the patch after these changes? Please add tests for the from-index variants of index-of and last-index-of as well.

Comment by Nola Stowe [ 14/Jul/15 5:46 PM ]

optimizations as suggested by Alex Miller

Comment by Andy Fingerhut [ 14/Jul/15 5:50 PM ]

Nola, not a big deal, but Rich has requested that patch file names end with ".patch" or ".diff", I think because whatever he uses to view them recognizes that suffix and puts it in a different viewing/editing mode.

Comment by Nola Stowe [ 14/Jul/15 6:03 PM ]

Andy, so no need to keep around patches when used in comparisons?

Comment by Andy Fingerhut [ 14/Jul/15 8:31 PM ]

It is ok to have multiple versions of patches attached, but names like add_functions_to_strings-2.patch etc. would be preferred over names like add_functions_to_strings.patch-2, so the suffix is ".patch".

Comment by Nola Stowe [ 14/Jul/15 8:55 PM ]

original patch submitted by Nola

Comment by Nola Stowe [ 14/Jul/15 8:56 PM ]

Optimizations suggested by Alex

Comment by Alex Miller [ 14/Jul/15 9:44 PM ]

Nola, I re-did the timings on my machine - these were done with Java 1.8 and no Leiningen involved.

Comment by Alex Miller [ 14/Jul/15 9:57 PM ]

-4 patch is identical to -3 but removes one errant ^long type hint

Otherwise, looks good to me!

Comment by Alex Miller [ 17/Jul/15 10:24 PM ]

Rich, since you didn't put any comments on here, I'm not sure if there's anything else you wanted, so marking screened.

Comment by Stuart Halloway [ 19/Jul/15 7:36 AM ]

I dislike the approach here for several reasons:

  • floats Java-isms (-1 for missing instead of nil?) up into the Clojure API
  • makes narrow string fns out of things that could/should be seq fns

Stepping back, this is all library work. Why should it be in core? If this started in a contrib, it could evolve much more freely.

Comment by Alex Miller [ 19/Jul/15 8:49 AM ]

re Java-isms: totally fair comment worth changing

re narrow string fns: if these were seq fns, they would presumably take seqables of chars and return seqables of chars. When I have strings, I want to work with strings (as with clojure.string/join and clojure.string/reverse vs their seq equivalents).

re library: these particular functions are the most commonly used string functions where current users drop to Java interop. Note that all 5 of these functions have over time been added to the cheatsheet (http://clojure.org/cheatsheet) because they are commonly used. I believe there is significant value in including them in core and standardizing their name and behavior across to CLJS.

Comment by Bozhidar Batsov [ 19/Jul/15 10:29 AM ]

I'm obviously biased, but I totally agree with everything Alex said. Having one core string library and a separate contrib string library is just impractical. With clojure.string already part of the language it's much more sensible to extend it where/when needed instead of encouraging the proliferation of tons of alternative libraries (there are already a couple of those out there, mostly because essential functions are missing). Initial designs are rarely perfect, there's nothing wrong in revisiting and improving them.

I believe this is one of the top voted tickets, which clearly indicates that the Clojure community is quite supportive of this additions.

Comment by Michael Blume [ 19/Jul/15 12:26 PM ]

None of these functions return Strings so they could very easily be seq functions which use the String methods as a fast path when called with Strings, the trouble is then they wouldn't necessarily belong in clojure.string.

Comment by Bozhidar Batsov [ 19/Jul/15 1:49 PM ]

Such an argument can be made for existing functions like clojure.core/subs and clojure.string/reverse - we could have just went with generic functions that operate on seqs and converted the results to strings, but we didn't. It might be just me, but I'm really thinking this is being overanalysed. Sometimes it's really preferable to deal with some things as strings (not to mention more efficient).

Comment by Rich Hickey [ 20/Jul/15 9:39 AM ]

I completely agree with Alex and Bozhidar re: strings vs seqs. Other langs that have only lists and use them for strings are really bad at strings. So - string-specific fns are ok. What about the Java-isms?

Comment by Bozhidar Batsov [ 20/Jul/15 10:08 AM ]

We should address them for sure. Let's wait for a couple of days for Nola to update the patch. If she's too busy I'll do the update myself.

Comment by Nola Stowe [ 20/Jul/15 10:50 AM ]

I will be happy to work on it Saturday. Is nil an acceptable response when index-of "apple" "p" is not found? The other functions return booleans and I think those are ok.

Comment by Bozhidar Batsov [ 20/Jul/15 10:55 AM ]

Yep, we should go with nil.

Comment by Nola Stowe [ 25/Jul/15 4:33 PM ]

Changed index-of and last-index-of to return nil instead of -1 (java-ism), thus had to remove the type hint for the return value.

Seems to have slowed down the function compared to having the function return -1. My benchmarks: https://www.evernote.com/l/ABL2wead73BJZYAkpH5yxsfX9JM8cpUiNhw

Comment by Nola Stowe [ 25/Jul/15 9:05 PM ]

Updated patch to include return value in doc string for index-of last-index-of, otherwise same as last update.

Comment by Alex Miller [ 26/Jul/15 8:40 AM ]

Removing the hint causes the return type to be Object, which forces boxing in the "found" cases.

Comment by Nola Stowe [ 26/Jul/15 8:56 AM ]

Is it worth it to remove the java-ism of returning -1 when not found? Personally, I don't really like a function returning one of two things... found index or nil ... I prefer it being an int at all times.

Comment by Alex Miller [ 26/Jul/15 9:00 AM ]

There is one whitespace issue in the -5 patch:

[~/code/clojure (master)]$ git apply ~/Downloads/add_functions_to_strings-5.patch
/Users/alex/Downloads/add_functions_to_strings-5.patch:34: trailing whitespace.
  (let [result
warning: 1 line adds whitespace errors.

I updated the timings in the description to include both -4 and -5 - the boxing there does have a significant impact.

Comment by Nola Stowe [ 26/Jul/15 9:28 AM ]

Fixed whitespace issue.

Comment by Alex Miller [ 26/Jul/15 9:32 AM ]

Added -6 patch and timings that address most of the performance impact.

Comment by Stuart Halloway [ 27/Jul/15 9:35 AM ]

Alex: Note that all of these functions return numbers and booleans, not strings nor seqs of anything. This is partially why they were left out of the original string implementation.

It isn't clear to me why index-of should be purely a string fn, I regularly need index-of with other things, and both my book and JoC use index-of as an example of a nice-to-have.

Comment by Nicola Mometto [ 27/Jul/15 9:44 AM ]

Stuart, I don't think the fact that those functions don't return string is really relevant, the important fact is that they operate on strings (just like c.set/superset? or subset? don't return sets) and are frequently used on them.

Comment by Bozhidar Batsov [ 27/Jul/15 9:51 AM ]

I'm with Nicola on this one - we have to be practical. There's a reason why this is the top-voted JIRA ticket - Clojure programmers want those functions. That's says plenty.

Anyways, having a generic `index-of` is definitely not a bad idea.

Comment by Stuart Halloway [ 31/Jul/15 11:27 AM ]

How does this compare to making them all seq fns and adding them to clojure.core?

Comment by Andy Fingerhut [ 31/Jul/15 11:39 AM ]

Given Rich's comment earlier "I completely agree with Alex and Bozhidar re: strings vs seqs. Other langs that have only lists and use them for strings are really bad at strings. So - string-specific fns are ok.", do we need to write a separate patch that makes these all into seq fns and do performance comparisons between those vs. the string-specific ones?

Comment by Stuart Halloway [ 31/Jul/15 1:26 PM ]

Andy,

Definitely not! I want to have a design conversation.

We have protocols, and could use them to provide core fns that are specialized to strings, so it isn't clear to me why we would have to be really bad at it.

Comment by Stuart Halloway [ 07/Aug/15 2:19 PM ]

Discussed with Rich. He does not want sequence fns with these performance characteristics, so string-specific fns it shall be.

Comment by Rich Hickey [ 08/Aug/15 10:11 AM ]

So which patch is screened?

Comment by Alex Miller [ 08/Aug/15 11:40 AM ]

Stu screened add_functions_to_strings-6.patch where index-of returns nil on not-found. The add_functions_to_strings-4.patch is almost identical but returns -1 from index-of (which matches Java and JavaScript index-of behavior and may be slightly faster for callers that can be monomorphic on primitive longs?).

Comment by Rich Hickey [ 24/Aug/15 11:27 AM ]

nil-returning index fns should document that return when not found

Comment by Nola Stowe [ 24/Aug/15 11:56 AM ]

adds better doc string to indicated nil is return when not found for index-of and last-index-of.

Comment by Alex Miller [ 24/Aug/15 11:59 AM ]

Added clj-1449-7.patch that is identical to -6 but adds statement of return expectation on index-of and last-index-of.

Comment by Alex Miller [ 24/Aug/15 12:19 PM ]

jinx - same patch! Replacing with Nola's.





[CLJ-1586] Compiler doesn't preserve metadata for LazySeq literals Created: 12/Nov/14  Updated: 24/Aug/15

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, metadata, typehints

Attachments: Text File 0001-CLJ-1586-Compiler-doesn-t-preserve-metadata-for-lazy.patch     Text File 0001-Compiler-doesn-t-preserve-metadata-for-lazyseq-liter.patch     Text File clj-1586-2.patch    
Patch: Code and Test
Approval: Ok

 Description   

The analyzer in Compiler.java forces evaluation of lazyseq literals, but loses the compile time original metadata of that form, meaning that a type hint will be lost.

Example demonstrating this issue:

user=> (set! *warn-on-reflection* true)
true
user=> (list '.substring (with-meta (concat '(identity) '("foo")) {:tag 'String}) 0)
(.substring (identity "foo") 0)
user=> (eval (list '.substring (with-meta (concat '(identity) '("foo")) {:tag 'String}) 0))
Reflection warning, NO_SOURCE_PATH:6:1 - call to method substring on java.lang.Object can't be resolved (no such method).
"foo"

Forcing the concat call to an ASeq rather than a LazySeq fixes this issue:

user=> (eval (list '.hashCode (with-meta (seq (concat '(identity) '("foo"))) {:tag 'String})))
101574

This ticket blocks http://dev.clojure.org/jira/browse/CLJ-1444 since clojure.core/sequence might return a lazyseq.

This bug affected both tools.analyzer and tools.reader and forced me to commit a fix in tools.reader to work around this issue, see: http://dev.clojure.org/jira/browse/TANAL-99

The proposed patch trivially preserves the form metadata after realizing the lazyseq

Approach: Keep a copy of the original form, and apply its metadata to the realized lazyseq
Patch: clj-1586-2.patch
Screened by: Alex Miller



 Comments   
Comment by Stuart Halloway [ 31/Jul/15 9:19 AM ]

Please add a test case.

Comment by Nicola Mometto [ 31/Jul/15 9:32 AM ]

Updated patch with testcase

Comment by Alex Miller [ 11/Aug/15 10:55 AM ]

clj-1586-2.patch is same as prior, just updated to apply to current master





[CLJ-1793] Clear 'this' before calls in tail position Created: 05/Aug/15  Updated: 24/Aug/15

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

1.8.0-alpha2 - 1.8.0-alpha4


Attachments: Text File 0001-Clear-this-before-calls-in-tail-position.patch     Text File clj-1793-2.patch    
Patch: Code and Test
Approval: Vetted

 Description   

(This ticket started life as CLJ-1250, was committed in 1.8.0-alpha2, pulled out after alpha4, and this is the new version that fixes the logic about whether in a tail call as well as addresses direct linking added in 1.8.0-alpha3.)

Problem: Original example was with reducers holding onto the head of a lazy seq:

(time (reduce + 0 (map identity (range 1e8))))    ;; works
(time (reduce + 0 (r/map identity (range 1e8))))  ;; oome from holding head of range

Example of regression from CLJ-1250 that now works correctly (doesn't clear this in nested loop) with CLJ-1793:

(let [done (atom false)
        f (future-call
            (fn inner []
              (while (not @done)
                (loop [found []]
                  (println (conj found 1))))))]
    (doseq [elem [:a :b :c :done]]
      (println "queue write " elem))
    (reset! done true)
    @f)

Approach: When invoking a method in a tail call, clear this prior to invoking.

The criteria for when a tail call is a safe point to clear 'this':

1) Must be in return position
2) Not in a try block (might need 'this' during catch/finally)
3) When not direct linked

Return position (#1) isn't simply (context == C.RETURN) because loop bodies are always parsed in C.RETURN context

A new dynvar METHOD_RETURN_CONTEXT tracks whether an InvokeExpr in tail position can directly leave the body of the compiled java method. It is set to RT.T in the outermost parsing of a method body and invalidated (set to null) when a loop body is being parsed where the context for the loop expression is not RETURN parsed. Added clear in StaticInvokeExpr as that is now a thing with direct linking again.

Removes calls to emitClearLocals(), which were a no-op.

Patch: clj-1793-2.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 05/Aug/15 12:16 PM ]

The this ref is cleared prior to the println, but the next time through the while loop it needs the this ref to look up the closed over done field (via getfield).

Adding an additional check to the inTailCall() method to not include tail call in a loop addresses this case:

static boolean inTailCall(C context) {
-    return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null);
+    return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null) && (LOOP_LOCALS.deref() == null);
}

But want to check some more things before concluding that's all that's needed.

Comment by Alex Miller [ 05/Aug/15 1:36 PM ]

This change undoes the desired behavior in the original CLJ-1250 (new tests don't pass). For now, we are reverting the CLJ-1250 patch in master.

Comment by Ghadi Shayban [ 05/Aug/15 3:12 PM ]

Loop exit edges are erroneously being identified as places to clear 'this'. Only exits in the function itself or the outermost loop are safe places to clear.

Comment by Ghadi Shayban [ 05/Aug/15 8:43 PM ]

Patch addresses this bug and the regression in CLJ-1250.

See the commit message for an extensive-ish comment.

Comment by Alex Miller [ 18/Aug/15 12:33 PM ]

New patch is same as old, just adds jira id to beginning of commit message.

Comment by Rich Hickey [ 24/Aug/15 10:00 AM ]

Not doing this for 1.8, more thought needs to go into whether this is the right solution to the problem. And, what is the problem? This title of this patch is just something to do.

Comment by Alex Miller [ 24/Aug/15 10:21 AM ]

changing to vetted so this is at a valid place in the jira workflow

Comment by Ghadi Shayban [ 24/Aug/15 10:45 AM ]

Rich the original context is in CLJ-1250 which was a defect/problem. It was merged and revert because of a problem in the impl. This ticket is the continuation of the previous one, but unfortunately the title lost the context and became approach-oriented and not problem-oriented. Blame Alex. (I kid, it's an artifact of the mutable approach to issue management.)





[CLJ-1232] Functions with non-qualified return type hints force import of hinted classes when called from other namespace Created: 18/Jul/13  Updated: 24/Aug/15

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: compiler, typehints

Attachments: Text File 0001-auto-qualify-arglists-class-names.patch     Text File 0001-auto-qualify-arglists-class-names-v2.patch     Text File 0001-auto-qualify-arglists-class-names-v3.patch     Text File 0001-auto-qualify-arglists-class-names-v4.patch     Text File 0001-CLJ-1232-auto-qualify-arglists-class-names.patch     Text File 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch    
Patch: Code and Test
Approval: Ok

 Description   

You can add a type hint to function arglists to indicate the return type of a function like so.

user> (import '(java.util List))
java.util.List
user> (defn linkedlist ^List [] (java.util.LinkedList.))
#'user/linkedlist
user> (.size (linkedlist))
0

The problem is that now when I call `linkedlist` exactly as above from another namespace, I'll get an exception because java.util.List is not imported in there.

user> (in-ns 'user2)
#<Namespace user2>
user2> (refer 'user)
nil
user2> (.size (linkedlist))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: List, compiling:(NO_SOURCE_PATH:1:1)
user2> (import '(java.util List)) ;; Too bad, need to import List here, too.
java.util.List
user2> (.size (linkedlist))
0

There are two workarounds: You can import the hinted type also in the calling namespace, or you always use fully qualified class names for return type hints. Clearly, the latter is preferable.

Approach: Resolve the tags in the defn macro.

Patch: 0001-CLJ-1232-auto-qualify-arglists-class-names.patch

Screened by: Fogus

Alternate approach: Make the compiler resolve the return tags when necessary (tag is not a string, primitive tag (^long) or array tag (^longs)) and update the Var's :arglist appropriately. Patch: 0001-auto-qualify-arglists-class-names-v4.patch. Note that this patch had problems with Hystrix, which was using non-conformant arglists - Hystrix has since been patched.



 Comments   
Comment by Andy Fingerhut [ 16/Apr/14 3:47 PM ]

To make sure I understand, Nicola, in this ticket you are asking that the Clojure compiler change behavior so that the sample code works correctly with no exceptions, the same way as it would work correctly without exceptions if one of the workarounds were used?

Comment by Tassilo Horn [ 17/Apr/14 12:18 AM ]

Hi Andy. Tassilo here, not Nicola. But yes, the example should work as-is. When I'm allowed to use type hints with simple imported class names for arguments, then doing so for return values should work, too.

Comment by Rich Hickey [ 10/Jun/14 10:41 AM ]

Type hints on function params are only consumed by the function definition, i.e. in the same module as the import/alias. Type hints on returns are just metadata, they don't get 'compiled' and if the metadata is not useful to consumers in other namespaces, it's not a useful hint. So, if it's not a type in the auto-imported set (java.lang), it should be fully qualified.

Comment by Alex Miller [ 10/Jun/14 11:55 AM ]

Based on Rich's comment, this ticket should probably morph into an enhancement request on documentation, probably on http://clojure.org/java_interop#Java Interop-Type Hints.

Comment by Andy Fingerhut [ 10/Jun/14 3:13 PM ]

I would suggest something like the following for a documentation change, after this part of the text on the page Alex links in the previous comment:

For function return values, the type hint can be placed before the arguments vector:

(defn hinted
(^String [])
(^Integer [a])
(^java.util.List [a & args]))

-> #user/hinted

If the return value type hint is for a class that is outside of java.lang, which is the part auto-imported by Clojure, then it must be a fully qualified class name, e.g. java.util.List, not List.

Comment by Nicola Mometto [ 10/Jun/14 4:02 PM ]

I don't understand why we should enforce this complexity to the user.
Why can't we just make the Compiler (or even defn itself) update all the arglists tags with properly resolved ones? (that's what I'm doing in tools.analyzer.jvm)

Comment by Alexander Kiel [ 19/Jul/14 10:02 AM ]

I'm with Nicola here. I also think that defn should resolve the type hint according the imports of the namespace defn is used in.

Comment by Max Penet [ 22/Jul/14 7:06 AM ]

Same here, I was bit by this in the past. The current behavior is clearly counterintuitive.

Comment by Nicola Mometto [ 28/Aug/14 12:58 PM ]

Attached two patches implementing two different solutions:

  • 0001-auto-qualify-arglists-class-names.patch makes the compiler automatically qualify all the tags in the :arglists
  • 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch makes the compiler throw an exception for all public defs whose return tag is a symbol representing a non-qualified class that is not in the auto-import list (approach proposed in IRC by Alex Miller)
Comment by Tassilo Horn [ 29/Aug/14 1:49 AM ]

For what it's worth, I'd prefer the first patch because the second doesn't help in situations where the caller lives in a namespace where the called function's return type hinted class is `ns-unmap`-ed. And there a good reasons for doing that. For example, Process is a java.lang class and Process is a pretty generic name. So in some namespace, I want to define my own Process deftype or defrecord. Without unmapping 'Process first to get rid of the java.lang.Process auto-import, I'd get an exception:

user> (deftype Process [])
IllegalStateException Process already refers to: class java.lang.Process in namespace: user  clojure.lang.Namespace.referenceClass (Namespace.java:140)

Now when I call some function from some library that has a `^Process` return type hint (meaning java.lang.Process there), I get the same exception as in my original report.

I can even get into troubles when only using standard Clojure functions because those have `^String` and `^Class` type hints. IMO, Class is also a pretty generic name I should be able to name my custom deftype/defrecord. And I might also want to have a custom String type/record in my astrophysics system.

Comment by Andy Fingerhut [ 30/Sep/14 4:39 PM ]

Not sure whether the root cause of this behavior is the same as the example in the description or not, but seems a little weird that even for fully qualified Java class names hinting the arg vector, it makes a difference whether it is done with defn or def:

Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3
Comment by Alex Miller [ 30/Sep/14 6:21 PM ]

Andy, can you file that as a separate ticket?

Comment by Andy Fingerhut [ 30/Sep/14 9:08 PM ]

Created ticket CLJ-1543 for the issue raised in my comment earlier on 30 Sep 2014.

Comment by Andy Fingerhut [ 01/Oct/14 12:38 PM ]

Tassilo (or anyone), is there a reason to prefer putting the tag on the argument vector in your example? It seems that putting it on the Var name instead avoids this issue:

user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (import '(java.util List))
java.util.List
user=> (defn ^List linkedlist [] (java.util.LinkedList.))
#'user/linkedlist
user=> (.size (linkedlist))
0
user=> (ns user2)
nil
user2=> (refer 'user)
nil
user2=> (.size (linkedlist))
0

I suppose that only allows a single type tag, rather than an independent one for each arity.

Comment by Tassilo Horn [ 02/Oct/14 3:16 AM ]

I wasn't aware of the fact that you can put it on the var's name. That's not documented at http://clojure.org/java_interop#Java Interop-Type Hints. But IMHO the documented version with putting the tag on the argument vector is more general since it supports different return type hints for the different arity version. In any case, if both forms are permitted then they should be equivalent in the case the function has only one arity.

Comment by Rich Hickey [ 16/Mar/15 12:02 PM ]

Please work on the simplest patch that resolves the names

Comment by Alex Miller [ 16/Mar/15 4:34 PM ]

Nicola, in this:

if (tag != null &&
                        !(tag instanceof String) &&
                        primClass((Symbol)tag) == null &&
                        !tagClass((Symbol) tag).getName().startsWith("["))
                        {
                            argvec = (IPersistentVector)((IObj)argvec).withMeta(RT.map(RT.TAG_KEY, Symbol.intern(tagClass((Symbol)tag).getName())));
                        }

doesn't tagClass already handle most of these cases properly already? Can this be simplified? Is there an optimization case in avoiding lookup for a dotted name?

Comment by Nicola Mometto [ 16/Mar/15 5:10 PM ]

Patch 0001-auto-qualify-arglists-class-names-v2.patch avoids doing unnecessary lookups (dotted names, special tags (primitive tags, array tags)) and adds a testcase

Comment by Michael Blume [ 20/May/15 1:13 PM ]

I'm seeing an odd failure with this patch and hystrix defcommands, will post a small reproduction shortly

Comment by Michael Blume [ 20/May/15 1:20 PM ]

https://github.com/MichaelBlume/hystrix-demo

passes lein check with 1.7 beta3, fails with v3 patch applied

Comment by Nicola Mometto [ 20/May/15 1:40 PM ]

During analysis the compiler understands only arglists in the form of (quote ([..]*)) (see https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L557-L558), hystrix emits arglists in the form of (list (quote [..])*).

Not sure what to do about this.

Comment by Michael Blume [ 20/May/15 1:51 PM ]

Possibly just ask Hystrix not to do that?

Comment by Alex Miller [ 20/May/15 2:30 PM ]

test.generative uses non-standard arglists too. I haven't looked at the patch, but if it's sensitive to that, it's probably not good enough.

Comment by Nicola Mometto [ 20/May/15 6:51 PM ]

test.generative uses non-standard :tag, not :arglists

Comment by Alex Miller [ 20/May/15 10:22 PM ]

ah, yes. sorry.

Comment by Stuart Halloway [ 10/Jul/15 12:15 PM ]

Opened https://github.com/Netflix/Hystrix/issues/831 to see what is up with hystrix.

Comment by Stuart Halloway [ 31/Jul/15 9:16 AM ]

Can somebody with context update this patch to apply to latest master?

Comment by Nicola Mometto [ 31/Jul/15 9:37 AM ]

patch updated

Comment by Matt Jacobs [ 06/Aug/15 12:59 PM ]

I've released hystrix-clj 1.4.14, with a fix for https://github.com/Netflix/Hystrix/issues/831: https://github.com/Netflix/Hystrix/releases/tag/v1.4.14. If anything else is needed on the Hystrix side, please open another issue.

Comment by Fogus [ 07/Aug/15 11:03 AM ]

Applied the 0001-CLJ-1232-auto-qualify-arglists-class-names.patch to master (1.8) and ran through the example scenario to check the veracity of the change. Additionally, I ran a modified snippet using def and verified that it too worked. Finally, after checking the code it seems reasonable in implementation and scope.

Comment by Rich Hickey [ 08/Aug/15 9:56 AM ]

I'd rather not change the compiler and it seems hystrix was broken. Also please make it clear what the single strategy and patch is at the top of the ticket.

Comment by Alex Miller [ 11/Aug/15 5:01 PM ]

Made preferred approach clear in the description and moved back to vetted. I believe the other ticket was screened by Fogus so this needs to be re-screened with the preferred approach ticket.

Comment by Alex Miller [ 11/Aug/15 5:08 PM ]

Nicola and Fogus, would appreciate an eye as to whether I just made the proper alterations in the description.

Comment by Nicola Mometto [ 12/Aug/15 6:17 AM ]

Alex Miller Reading Fogus' last comment, it seems to me that he actually screened the current patch (the one who resolves tags in defn rather than in the compiler) "Applied the 0001-CLJ-1232-auto-qualify-arglists-class-names.patch to master [..]"

Comment by Fogus [ 14/Aug/15 9:18 AM ]

It would be better if the description said the word "preferred," but based on my reading resolving the tags in defn is the winner (0001-CLJ-1232-auto-qualify-arglists-class-names.patch). Thankfully that's the one that I screened as I agree that non-trivial changes to the compiler are to be avoided. That said, of course that patch does modify the compiler but since it's a change that pulls out some code into a useful method I let it through. To my mind that was a trivial (and justifiable change). Of course, if we want to avoid any compiler changes then this patch would have to be reworked.

Comment by Alex Miller [ 18/Aug/15 5:03 PM ]

Moving back to screened as fogus screened the preferred (by Rich) version.





[CLJ-1766] Serializing+deserializing lists breaks their hash Created: 21/Jun/15  Updated: 24/Aug/15

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

Type: Defect Priority: Major
Reporter: Chris Vermilion Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop
Environment:

Tested on OS X 10.10.3, Clojure 1.6.0, Java 1.8.0_45-b14, but don't think this is a factor.


Attachments: Text File clj-1766-2.patch     Text File CLJ-1766.patch     File serialization_test_mod.diff     File serialize-test.clj    
Patch: Code and Test
Approval: Ok

 Description   

clojure.lang.PersistentList implements Serializable but a serialization roundtrip results in a hash of 0. This appears to be caused by ASeq marking its _hash property as transient; other collection base classes don't do this. I don't know if there is a rationale for the distinction, but the result is that serializing, then deserializing, a list results in its _hash property being 0 instead of either its previous, calculated value, or -1, which would trigger recalculation.

This means that any code that relies on a list's hash value can break in subtle ways.

Examples:

(import '[java.io ByteArrayInputStream ByteArrayOutputStream ObjectInputStream ObjectOutputStream])

(defn obj->bytes [obj]
  (let [bytestream (ByteArrayOutputStream.)]
    (doto (ObjectOutputStream. bytestream) (.writeObject obj))
    (.toByteArray bytestream)))

(defn bytes->obj [bs]
  (.readObject (ObjectInputStream. (ByteArrayInputStream. bs))))

(defn round-trip [x] (bytes->obj (obj->bytes x)))
user=> (hash '(1))
-1381383523
user=> (hash (round-trip '(1)))
0
user=> #{'(1) (round-trip '(1))}
#{(1) (1)}
user=> (def m {'(1) 1})
#'user/m
user=> (= m (round-trip m))
true
user=> (= (round-trip m) m)
false

Approach: Use 0 as the "uncomputed" hash value, not -1. The auto initialized value on serialization construction will then automatically trigger a re-computation.

Alternate approaches:

  • Implement a readObject method that sets the _hash property to -1. This is somewhat complicated in the face of subclasses, etc.

Note: Also need to consider other classes that use -1 instead of 0 as the uncomputed hash value: APersistentMap, APersistentSet, APersistentVector, PersistentQueue - however I believe in those cases they are not transient and thus may avoid the issue. Perhaps they should be transient though.

Patch: clj-1766-2.patch
Screened by: Alex Miller



 Comments   
Comment by Chris Vermilion [ 21/Jun/15 1:10 PM ]

Yikes, sorry about the formatting above; JIRA newbie and looks like I can't edit. Also, I should have noted that the above functions require an import: (import '[java.io ByteArrayInputStream ByteArrayOutputStream ObjectInputStream ObjectOutputStream]).

Comment by Alex Miller [ 22/Jun/15 7:55 AM ]

Yes, this is a bug. My preference would be to use 0 to indicate "not computed" and thus sidestep this issue. The downside of changing from -1 to 0 is that serialization/deserialization is then broken between versions of Clojure (although maybe if it was already broken, that's not an issue). -1 is used like this for lazily computed hashes in many classes in Clojure so the issue should really be fixed across the board.

Comment by Alex Miller [ 22/Jun/15 8:22 AM ]

There are some serialization round-trip tests in clojure.test-clojure.serialization - seems like those should be updated to compare the .hashCode and hash before/after, which should catch this. I attached a diff (not a proper patch) with that change just as a demonstration, which indeed produces a bunch of failures. That should be incorporated into any fix, possibly along with other failures.

Comment by Alex Miller [ 31/Jul/15 9:04 AM ]

Also, don't worry about crediting me on that test change, please just incorporate it into whatever gets made here.

Comment by Andrew Rosa [ 31/Jul/15 9:31 AM ]

Alex Miller I hope to work on this issue this weekend. There are some conflict with the alpha release schedule?

Comment by Alex Miller [ 31/Jul/15 9:47 AM ]

No conflict - when it's ready we'll look at it.

Comment by Andrew Rosa [ 04/Aug/15 8:21 AM ]

As Alex Miller recomended, I followed the change-default-to-zero path.
Not only it sidesteps the serialization issue, it looks more aligned with
the semantics of transient. Of course, there is no guarantees
about how different JVM implementations will deal with it - sometimes
they will be serialized, sometimes they will not.

So, together with the patch I've made some manual tests between some versions. The script used is attached for further inspection.

Running on a 1.6 REPL has shown on the original described issue:

serialize-test=> *clojure-version*
{:major 1, :minor 6, :incremental 0, :qualifier nil}
serialize-test=> (def f "seq-1-6.dump")
#'serialize-test/f
serialize-test=> (def x {'(1) 1})
#'serialize-test/x
serialize-test=> (dump-bytes f (serialize x))
nil
serialize-test=> (deserialize (slurp-bytes f))
{(1) 1}
serialize-test=> (hash x)
202919476
serialize-test=> (hash (deserialize (serialize x)))
202919476
serialize-test=> (hash (deserialize (slurp-bytes f)))
-1619826937
serialize-test=> (= x (deserialize (slurp-bytes f)))
true
serialize-test=> (= (deserialize (slurp-bytes f)) x)
false

Running on 1.8 master. Despite of the = behavior looking ok, the
hashes are different between roundtrips, like we saw on 1.6:

serialize-test=> *clojure-version*
{:major 1, :minor 8, :incremental 0, :qualifier "master", :interim true}
serialize-test=> (def f "seq-1-8-master.dump")
#'serialize-test/f
serialize-test=> (def x {'(1) 1})
#'serialize-test/x
serialize-test=> (dump-bytes f (serialize x))
nil
serialize-test=> (deserialize (slurp-bytes f))
{(1) 1}
serialize-test=> (hash x)
202919476
serialize-test=> (hash (deserialize (serialize x)))
202919476
serialize-test=> (hash (deserialize (slurp-bytes f)))
-1619826937
serialize-test=> (= x (deserialize (slurp-bytes f)))
true
serialize-test=> (= (deserialize (slurp-bytes f)) x)
true

Running on 1.8 after patch the hashes are correctly computed on both cases:

serialize-test=> *clojure-version*
{:major 1, :minor 8, :incremental 0, :qualifier "master", :interim true}
serialize-test=> (def f "seq-1-8-patch.dump")
#'serialize-test/f
serialize-test=> (def x {'(1) 1})
#'serialize-test/x
serialize-test=> (dump-bytes f (serialize x))
nil
serialize-test=> (deserialize (slurp-bytes f))
{(1) 1}
serialize-test=> (hash x)
202919476
serialize-test=> (hash (deserialize (serialize x)))
202919476
serialize-test=> (hash (deserialize (slurp-bytes f)))
202919476
serialize-test=> (= x (deserialize (slurp-bytes f)))
true
serialize-test=> (= (deserialize (slurp-bytes f)) x)
true

Please note I've dumped each serialized data into different files, so I could test the interoperability between those versions. What I found:

  • 1.6 serialization is already incompatible with 1.8, on both directions;
  • 1.8 data could be exchange between master and patched versions, not affecting version behavior (hashes are computed only on patched REPL).

Did I miss something or everything looks correct for you too? I'm open to do some more exhaustive testing if anyone could give me some directions about where to explore.

Comment by Rich Hickey [ 08/Aug/15 9:46 AM ]

This patch both fixes the serialization problem and changes the implementation for no reason related to the problem. The implementation works in place in order to avoid head-holding, which the implementation change reintroduces. Screeners - please make sure patches contain the minimal code to address the problem and don't incorporate other extraneous 'improvements'.

Comment by Ghadi Shayban [ 08/Aug/15 11:59 AM ]

Rich Hickey The only change I see is the removal of a commented-out impl.

Comment by Alex Miller [ 08/Aug/15 10:33 PM ]

I agree with Ghadi - there is no change in implementation here, just some commented (non-used) code was removed. Moving back to Screened.

Comment by Alex Miller [ 18/Aug/15 12:26 PM ]

Latest patch is identical, just does not remove the commented out code.





Generated at Sun Aug 30 02:58:08 CDT 2015 using JIRA 4.4#649-r158309.