<< Back to previous view

[CLJ-1330] Class name clash between top-level functions and defn'ed ones Created: 22/Jan/14  Updated: 24/Jan/14

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: aot, compiler

Attachments: Text File 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch     File demo1.clj    
Patch: Code
Approval: Triaged

 Description   

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Demonstration script: demo1.clj

Patch: 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.

See also: This patch also fixes the issue reported in CLJ-1227.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 22/Jan/14 11:12 AM ]

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Comment by Nicola Mometto [ 22/Jan/14 11:35 AM ]

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Comment by Nicola Mometto [ 22/Jan/14 11:39 AM ]

Cleaned up patch

Comment by Alex Miller [ 22/Jan/14 12:43 PM ]

It looks like the patch changes indentation of some of the code - can you fix that?

Comment by Nicola Mometto [ 22/Jan/14 3:57 PM ]

Updated patch without whitespace changes

Comment by Alex Miller [ 22/Jan/14 4:15 PM ]

Thanks, that's helpful.

Comment by Alex Miller [ 24/Jan/14 10:03 AM ]

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Comment by Nicola Mometto [ 24/Jan/14 11:39 AM ]

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.





[CLJ-1250] Reducer (and folder) instances hold onto the head of seqs Created: 03/Sep/13  Updated: 20/Mar/14

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: memory, reducers

Attachments: Text File after-change.txt     Text File before-change.txt     Text File CLJ-1250-20131211.patch     Text File CLJ-1250-AllInvokeSites-20140204.patch     Text File CLJ-1250-AllInvokeSites-20140320.patch    
Patch: Code
Approval: Triaged

 Description   

Problem Statement
A shared function #'clojure.core.reducers/reducer holds on to the head of a reducible collection, causing it to blow up when the collection is a lazy sequence.

Cause: #'reducer closes over a collection when in order to reify CollReduce, and the closed-over is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

Reproduction steps:
Compare the following calls:

(time (reduce + 0 (map identity (range 1e8))))
(time (reduce + 0 (r/map identity (range 1e8))))

The second call should fail on a normal or small heap.

(If reducers are faster than seqs, increase the range.)

Approaches:

1) Reimplement the #'reducer (and #'folder) transformation fns similar to the manner that Christophe proposes here:

(defrecord Reducer [coll xf])

(extend-protocol 
  clojure.core.protocols/CollReduce
  Reducer
      (coll-reduce [r f1]
                   (clojure.core.protocols/coll-reduce r f1 (f1)))
      (coll-reduce [r f1 init]
                   (clojure.core.protocols/coll-reduce (:coll r) ((:xf r) f1) init)))

(def rreducer ->Reducer) 

(defn rmap [f coll]
  (rreducer coll (fn [g] 
                   (fn [acc x]
                     (g acc (f x))))))

Advantages: Relatively non-invasive change.
Disadvantages: Not evident code. Additional protocol dispatch, though only incurred once

2) Clear the reference to 'this' on the stack just before a tail call occurs

When a callee is in return position, clear the local variable reference to 'this' in the caller's stack frame, which will make the caller and all its closed-overs eligible for reclamation.

Patch 1211 takes this approach for InvokeExpr call sites in return position. Patch 1214 takes the same approach for InvokeExprs and also static and instance interop calls.

Here is the code that performs the clearing excerpted from the 1214 patch:

void emitClearThis(GeneratorAdapter gen) {
		gen.visitInsn(Opcodes.ACONST_NULL);
		gen.visitVarInsn(Opcodes.ASTORE, 0);
	}

Tail calls wrapped inside a try/catch/finally clause cannot have 'this' cleared, because closed-overs/locals may need to be emitted for exception handling blocks. Both patches consider and handle this edge case.

Advantages: Fixes this case with no user code changes. Enables GC to do reclaim closed-overs references earlier.
Disadvantages: A compiler change.

3) Alternate approach

from Christophe Grand:
Another way would be to enhance the local clearing mechanism to also clear "this" but it's complex since it may be needed to keep a reference to "this" for caches long after obvious references to "this" are needed.

Advantages: Fine-grained
Disadvantages: Complex, invasive, and the compiler is hard to hack on.

Mitigations
Avoid reducing on lazy-seqs and instead operate on vectors / maps, or custom reifiers of CollReduce or CollFold. This could be easier with some implementations of common collection functions being available (like iterate and partition).

See https://groups.google.com/d/msg/clojure-dev/t6NhGnYNH1A/2lXghJS5HywJ for previous discussion.



 Comments   
Comment by Gary Fredericks [ 03/Sep/13 8:53 AM ]

Fixed indentation in description.

Comment by Ghadi Shayban [ 11/Dec/13 11:08 PM ]

Adding a patch that clears "this" before tail calls. Verified that Christophe's repro case is fixed.

Will upload a diff of the bytecode soon.

Any reason this juicy bug was taken off 1.6?

Comment by Ghadi Shayban [ 11/Dec/13 11:17 PM ]

Here's the bytecode for the clojure.core.reducers/reducer reify before and after the change... Of course a straight diff isn't useful because all the line numbers changed. Kudos to Gary Trakhman for the no.disassemble lein plugin.

Comment by Christophe Grand [ 12/Dec/13 6:58 AM ]

Ghadi, I'm a bit surprised by this part of the patch: was the local clearing always a no-op here?

-		if(context == C.RETURN)
+		if(shouldClear)
 			{
-			ObjMethod method = (ObjMethod) METHOD.deref();
-			method.emitClearLocals(gen);
+                            gen.visitInsn(Opcodes.ACONST_NULL);
+                            gen.visitVarInsn(Opcodes.ASTORE, 0);
 			}

The problem with this approach (clear this on tail call) is that it adds yet another special case. To me the complexity stem from having to keep this around even if the user code doesn't refer to it.

Comment by Ghadi Shayban [ 12/Dec/13 7:19 AM ]

Thank you - I failed to mention this in the commit message: it appears that emitClearLocals() belonging to both ObjMethod and FnMethod (its child) are empty no-ops. I believe the actual local clearing is on line 4855.

I agree re: another special case in the compiler.

Comment by Alex Miller [ 12/Dec/13 8:56 AM ]

Ghadi re 1.6 - this ticket was never in the 1.6 list, it has not yet been vetted by Rich but is ready to do so when we open up again after 1.6.

Comment by Ghadi Shayban [ 12/Dec/13 8:59 AM ]

Sorry I confused the critical list with the Rel1.6 list.

Comment by Ghadi Shayban [ 14/Dec/13 11:16 AM ]

New patch 20131214 that handles all tail invoke sites (InvokeExpr + StaticMethodExpr + InstanceMethodExpr). 'StaticInvokeExpr' seems like an old remnant that had no active code path, so that was left as-is.

The approach taken is still the same as the original small patch that addressed only InvokeExpr, except that it is now using a couple small helpers. The commit message has more details.

Also a 'try' block with no catch or finally clause now becomes a BodyExpr. Arguably a user error, historically accepted, and still accepted, but now they are a regular BodyExpr, instead of being wrapped by a the no-op try/catch mechanism. This second commit can be optionally discarded.

With this patch on my machine (4/8 core/thread Ivy Bridge) running on bare clojure.main:
Christophe's test cases both run i 3060ms on a artificially constrained 100M max heap, indicating a dominant GC overhead. (But they now both work!)

When max heap is at a comfortable 2G the reducers version outpaces the lazyseq at 2100ms vs 2600ms!

Comment by Ghadi Shayban [ 13/Jan/14 10:48 AM ]

Updating stale patch after latest changes to master. Latest is CLJ-1250-AllInvokeSites-20140113

Comment by Ghadi Shayban [ 04/Feb/14 3:50 PM ]

Updating patch after murmur changes

Comment by Tassilo Horn [ 13/Feb/14 4:52 AM ]

Ghadi, I suffer from the problem of this issue. Therefore, I've applied your patch CLJ-1250-AllInvokeSites-20140204.patch to the current git master. However, then I get lots of "java.lang.NoSuchFieldError: array" errors when the clojure tests are run:

     [java] clojure.test-clojure.clojure-set
     [java] 
     [java] java.lang.NoSuchFieldError: array
     [java] 	at clojure.core.protocols$fn__6026.invoke(protocols.clj:123)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$fn__6023.invoke(protocols.clj:147)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$seq_reduce.invoke(protocols.clj:31)
     [java] 	at clojure.core.protocols$fn__6017.invoke(protocols.clj:48)
     [java] 	at clojure.core.protocols$fn__5968$G__5963__5981.invoke(protocols.clj:13)
     [java] 	at clojure.core$reduce.invoke(core.clj:6213)
     [java] 	at clojure.set$difference.doInvoke(set.clj:61)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:442)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050$fn__1083.invoke(clojure_set.clj:109)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050.invoke(clojure_set.clj:109)
     [java] 	at clojure.test$test_var$fn__7123.invoke(test.clj:704)
     [java] 	at clojure.test$test_var.invoke(test.clj:704)
     [java] 	at clojure.test$test_vars$fn__7145$fn__7150.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars$fn__7145.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars.invoke(test.clj:718)
     [java] 	at clojure.test$test_all_vars.invoke(test.clj:727)
     [java] 	at clojure.test$test_ns.invoke(test.clj:746)
     [java] 	at clojure.core$map$fn__2665.invoke(core.clj:2515)
     [java] 	at clojure.lang.LazySeq.sval(LazySeq.java:40)
     [java] 	at clojure.lang.LazySeq.seq(LazySeq.java:49)
     [java] 	at clojure.lang.Cons.next(Cons.java:39)
     [java] 	at clojure.lang.RT.boundedLength(RT.java:1655)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:130)
     [java] 	at clojure.core$apply.invoke(core.clj:619)
     [java] 	at clojure.test$run_tests.doInvoke(test.clj:761)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$run_all_tests$fn__527.invoke(runner.clj:255)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519$fn__523.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests.invoke(runner.clj:253)
     [java] 	at clojure.test.generative.runner$test_dirs.doInvoke(runner.clj:304)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$_main.doInvoke(runner.clj:312)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at user$eval564.invoke(run_tests.clj:3)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:6657)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7084)
     [java] 	at clojure.lang.Compiler.loadFile(Compiler.java:7040)
     [java] 	at clojure.main$load_script.invoke(main.clj:274)
     [java] 	at clojure.main$script_opt.invoke(main.clj:336)
     [java] 	at clojure.main$main.doInvoke(main.clj:420)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at clojure.lang.Var.invoke(Var.java:379)
     [java] 	at clojure.lang.AFn.applyToHelper(AFn.java:154)
     [java] 	at clojure.lang.Var.applyTo(Var.java:700)
     [java] 	at clojure.main.main(main.java:37)
Comment by Ghadi Shayban [ 13/Feb/14 8:23 AM ]

Can you give some details about your JVM/environment that can help reproduce? I'm not encountering this error.

Comment by Tassilo Horn [ 13/Feb/14 9:41 AM ]

Sure. It's a 64bit ThinkPad running GNU/Linux.

% java -version
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.5) (ArchLinux build 7.u51_2.4.5-1-x86_64)
OpenJDK 64-Bit Server VM (build 24.51-b03, mixed mode)
Comment by Ghadi Shayban [ 13/Feb/14 10:19 AM ]

Strange, that is exactly my mail env, OpenJDK7 on Arch, 64-bit. I have also tested on JDK 6/7/8 on OSX mavericks. Are you certain that the git tree is clean besides the patch? (Arch users unite!)

Comment by Tassilo Horn [ 14/Feb/14 1:13 AM ]

Yes, the tree is clean. But now I see that I get the same error also after resetting to origin/master, so it's not caused by your patch at all. Oh, now the error vanished after doing a `mvn clean`! So problem solved.

Comment by Nicola Mometto [ 19/Feb/14 12:32 PM ]

Ghandi, FnExpr.parse should bind IN_TRY_BLOCK to false before analyzing the fn body, consider the case

(try (do something (fn a [] (heap-consuming-op a))) (catch Exception e ..))

Here in the a function the this local will never be cleared even though it's perfectly safe to.
Admittedly this is an edge case but we should cover this possibility too.

Comment by Ghadi Shayban [ 19/Feb/14 2:06 PM ]

You may have auto-corrected my name to Ghandi instead of Ghadi. I wish I were that wise =)

I will update the patch for FnExpr (that seems reasonable), but maybe after 1.6 winds down and the next batch of tickets get scrutiny. It would be nice to get input on a preferred approach from Rich or core after it gets vetted – or quite possibly not vetted.

Comment by Nicola Mometto [ 19/Feb/14 6:11 PM ]

hah, sorry for the typo on the name

Seems reasonable to me, in the meantime I just pushed to tools.analyzer/tools.emitter complete support for "this" clearing, I'll test this a bit in the next few days to make sure it doesn't cause unexpected problems.

Comment by Andy Fingerhut [ 24/Feb/14 12:13 PM ]

Patch CLJ-1250-AllInvokeSites-20140204.patch no longer applies cleanly to latest master as of Feb 23, 2014. It did on Feb 14, 2014. Most likely some of its context lines are changed by the commit to Clojure master on Feb 23, 2014 – I haven't checked in detail.

Comment by Ghadi Shayban [ 20/Mar/14 4:39 PM ]

Added a patch that 1) applies cleanly, 2) binds the IN_TRY_EXPR to false initially when analyzing FnExpr and 3) uses RT.booleanCast





[CLJ-1152] PermGen leak in multimethods and protocol fns when evaled Created: 30/Jan/13  Updated: 10/Dec/13

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

Type: Defect Priority: Critical
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: memory, protocols

Approval: Triaged

 Description   

There is a PermGen memory leak that we have tracked down to protocol methods and multimethods called inside an eval, because of the caches these methods use. The problem only arises when the value being cached is an instance of a class (such as a function or reify) that was defined inside the eval. Thus extending IFn or dispatching a multimethod on an IFn are likely triggers.

My fellow LonoClouder, Jeff Dik describes how to reproduce and work around the problem:

The easiest way that I have found to test this is to set "-XX:MaxPermSize" to a reasonable value so you don't have to wait too long for the PermGen space to fill up, and to use "-XX:+TraceClassLoading" and "-XX:+TraceClassUnloading" to see the classes being loaded and unloaded.

leiningen project.clj
(defproject permgen-scratch "0.1.0-SNAPSHOT"
  :dependencies [[org.clojure/clojure "1.5.0-RC1"]]
  :jvm-opts ["-XX:MaxPermSize=32M"
             "-XX:+TraceClassLoading"
             "-XX:+TraceClassUnloading"])

You can use lein swank 45678 and connect with slime in emacs via M-x slime-connect.

To monitor the PermGen usage, you can find the Java process to watch with "jps -lmvV" and then run "jstat -gcold <PROCESS_ID> 1s". According to the jstat docs, the first column (PC) is the "Current permanent space capacity (KB)" and the second column (PU) is the "Permanent space utilization (KB)". VisualVM is also a nice tool for monitoring this.

Multimethod leak

Evaluating the following code will run a loop that eval's (take* (fn foo [])).

multimethod leak
(defmulti take* (fn [a] (type a)))

(defmethod take* clojure.lang.Fn
  [a]
  '())

(def stop (atom false))
(def sleep-duration (atom 1000))

(defn run-loop []
  (when-not @stop
    (eval '(take* (fn foo [])))
    (Thread/sleep @sleep-duration)
    (recur)))

(future (run-loop))

(reset! sleep-duration 0)

In the lein swank session, you will see many lines like below listing the classes being created and loaded.

[Loaded user$eval15802$foo__15803 from __JVM_DefineClass__]
[Loaded user$eval15802 from __JVM_DefineClass__]

These lines will stop once the PermGen space fills up.

In the jstat monitoring, you'll see the amount of used PermGen space (PU) increase to the max and stay there.

-    PC       PU        OC          OU       YGC    FGC    FGCT     GCT
 31616.0  31552.7    365952.0         0.0      4     0    0.000    0.129
 32000.0  31914.0    365952.0         0.0      4     0    0.000    0.129
 32768.0  32635.5    365952.0         0.0      4     0    0.000    0.129
 32768.0  32767.6    365952.0      1872.0      5     1    0.000    0.177
 32768.0  32108.2    291008.0     23681.8      6     2    0.827    1.006
 32768.0  32470.4    291008.0     23681.8      6     2    0.827    1.006
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258

A workaround is to run prefer-method before the PermGen space is all used up, e.g.

(prefer-method take* clojure.lang.Fn java.lang.Object)

Then, when the used PermGen space is close to the max, in the lein swank session, you will see the classes created by the eval'ing being unloaded.

[Unloading class user$eval5950$foo__5951]
[Unloading class user$eval3814]
[Unloading class user$eval2902$foo__2903]
[Unloading class user$eval13414]

In the jstat monitoring, there will be a long pause when used PermGen space stays close to the max, and then it will drop down, and start increasing again when more eval'ing occurs.

-    PC       PU        OC          OU       YGC    FGC    FGCT     GCT
 32768.0  32767.9    159680.0     24573.4      6     2    0.167    0.391
 32768.0  32767.9    159680.0     24573.4      6     2    0.167    0.391
 32768.0  17891.3    283776.0     17243.9      6     2   50.589   50.813
 32768.0  18254.2    283776.0     17243.9      6     2   50.589   50.813

The defmulti defines a cache that uses the dispatch values as keys. Each eval call in the loop defines a new foo class which is then added to the cache when take* is called, preventing the class from ever being GCed.

The prefer-method workaround works because it calls clojure.lang.MultiFn.preferMethod, which calls the private MultiFn.resetCache method, which completely empties the cache.

Protocol leak

The leak with protocol methods similarly involves a cache. You see essentially the same behavior as the multimethod leak if you run the following code using protocols.

protocol leak
(defprotocol ITake (take* [a]))

(extend-type clojure.lang.Fn
  ITake
  (take* [this] '()))

(def stop (atom false))
(def sleep-duration (atom 1000))

(defn run-loop []
  (when-not @stop
    (eval '(take* (fn foo [])))
    (Thread/sleep @sleep-duration)
    (recur)))

(future (run-loop))

(reset! sleep-duration 0)

Again, the cache is in the take* method itself, using each new foo class as a key.

A workaround is to run -reset-methods on the protocol before the PermGen space is all used up, e.g.

(-reset-methods ITake)

This works because -reset-methods replaces the cache with an empty MethodImplCache.



 Comments   
Comment by Chouser [ 30/Jan/13 9:10 AM ]

I think the most obvious solution would be to constrain the size of the cache. Adding an item to the cache is already not the fastest path, so a bit more work could be done to prevent the cache from growing indefinitely large.

That does raise the question of what criteria to use. Keep the first n entries? Keep the n most recently used (which would require bookkeeping in the fast cache-hit path)? Keep the n most recently added?

Comment by Jamie Stephens [ 18/Oct/13 9:35 AM ]

At a minimum, perhaps a switch to disable the caches – with obvious performance impact caveats.

Seems like expensive LRU logic is probably the way to go, but maybe don't have it kick in fully until some threshold is crossed.

Comment by Alex Miller [ 18/Oct/13 4:28 PM ]

A report seeing this in production from mailing list:
https://groups.google.com/forum/#!topic/clojure/_n3HipchjCc

Comment by Adrian Medina [ 10/Dec/13 11:43 AM ]

So this is why we've been running into PermGen space exceptions! This is a fairly critical bug for us - I'm making extensive use of multimethods in our codebase and this exception will creep in at runtime randomly.





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

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

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

Attachments: Text File 0001-FIX-CLJ-1388.patch    
Patch: Code and Test
Approval: Triaged

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

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

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

Patch: 0001-FIX-CLJ-1388.patch

Screened by:






[CLJ-1362] Reduce broken on some primitive vectors Created: 18/Feb/14  Updated: 19/Feb/14

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

Type: Defect Priority: Major
Reporter: Nathan Davis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File clj-1362-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

In some cases, reduce over a sequence from a primitive vector created with vector-of will return incorrect answers:

user=> (into [] (drop 32 (into [] (range 33))))
[32]
user=> (into [] (drop 32 (into (vector-of :int) (range 33))))
[0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32]

Second call should return [32] just like the first one.

Cause: VecSeq (seq on primitive Vec obtained with vector-of) maintains two flags: i is the total number of elements prior to the current node in this seq. offset is the offset in the current anode. When using internal-reduce on a VecSeq, the starting index for the reduce was using offset and ignoring i.

Solution: Use (+ i offset) as the starting index.

Patch: clj-1362-v1.patch

Screened by:



 Comments   
Comment by Alex Miller [ 18/Feb/14 10:18 PM ]

We did some debugging on this at the St. Louis Clojure Meetup tonight and suspect the problem is happening when drop walks through the chunked seq over the vector. Specifically, in the VecSeq's implementation of IChunkedSeq.chunkedNext() at https://github.com/clojure/clojure/blob/master/src/clj/clojure/gvec.clj#L116 particularly the offset 0 at the end.

Comment by Alex Miller [ 19/Feb/14 2:41 PM ]

Upon further review, the VecSeq seems to be created properly during chunking. The real issue is in internal-reduce where the starting index is improperly computed.





[CLJ-1322] doseq with several bindings causes "ClassFormatError: Invalid Method Code length" Created: 10/Jan/14  Updated: 10/Jan/14

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

Type: Defect Priority: Major
Reporter: Miikka Koskinen Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Clojure 1.5.1, java 1.7.0_25, OpenJDK Runtime Environment (IcedTea 2.3.10) (7u25-2.3.10-1ubuntu0.12.04.2)


Approval: Triaged

 Description   
user=> (def a1 (range 10))
#'user/a1
user=> (doseq [x1 a1 x2 a1 x3 a1 x4 a1 x5 a1 x6 a1 x7 a1 x8 a1] (do))
CompilerException java.lang.ClassFormatError: Invalid method Code length 69883 in class file user$eval1032, compiling:(NO_SOURCE_PATH:2:1)

While this example is silly, it's a problem we've hit a couple of times. It's pretty surprising when you have just a couple of lines of code and suddenly you get the code length error.






[CLJ-1274] Unable to set compiler options via system properties except for AOT compilation Created: 02/Oct/13  Updated: 14/Feb/14

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

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: compiler

Attachments: Text File CLJ-1274.patch    
Patch: Code
Approval: Triaged

 Description   

The code that converts JVM system properties into keys under the *compiler-options* var is present only inside the clojure.lang.Compile class. This is a problem when using a debugger inside an IDE and not AOT compiling; specifying -Dclojure.compiler.disable-locals-clearing=true has no effect here when it would be most useful!



 Comments   
Comment by Howard Lewis Ship [ 02/Oct/13 4:52 PM ]

Obviously, that's supposed to be *compiler-options*.

Comment by Howard Lewis Ship [ 02/Dec/13 4:03 PM ]

Changes initialization of *compiler-options* to occur statically inside Compiler; now available to all forms of Clojure, not just AOT compilation; however, the initial *compiler-options* value is now defined as a root binding, rather than a per-thread binding, which has slightly different semantics.





[CLJ-1192] vec function is substantially slower than into function Created: 06/Apr/13  Updated: 11/Apr/14

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

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Approval: Triaged

 Description   

(vec coll) and (into [] coll) do exactly the same thing. However, due to into using transients, it is substantially faster. On my machine:

(time (dotimes [_ 100] (vec (range 100000))))
"Elapsed time: 732.56 msecs"

(time (dotimes [_ 100] (into [] (range 100000))))
"Elapsed time: 491.411 msecs"

This is consistently repeatable.

Since vec's sole purpose is to transform collections into vectors, it should do so at the maximum speed available.



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 5:50 PM ]

I am pretty sure that Clojure 1.5.1 also uses transient vectors for (vec (range n)) (probably also some earlier versions of Clojure, too).

Look at vec in core.clj. It checks whether its arg is a java.util.Collection, which lazy seqs are, so calls (clojure.lang.LazilyPersistentVector/create coll).

LazilyPersistentVector's create method checks whether its argument is an ISeq, which lazy seqs are, so it calls PersistentVector.create(RT.seq(coll)).

All 3 of PersistentVector's create() methods use transient vectors to build up the result.

I suspect the difference in run times are not because of transients or not, but because of the way into uses reduce, and perhaps may also have something to do with the perhaps-unnecessary call to RT.seq in LazilyPersistentVector's create method (in this case, at least – it is likely needed for other types of arguments).

Comment by Alan Malloy [ 14/Jun/13 2:17 PM ]

I'm pretty sure the difference is that into uses reduce: since reducers were added in 1.5, chunked sequences know how to reduce themselves without creating unnecessary cons cells. PersistentVector/create doesn't use reduce, so it has to allocate a cons cell for each item in the sequence.

Comment by Gary Fredericks [ 08/Sep/13 1:55 PM ]

Is there any downside to (defn vec [coll] (into [] coll)) (or the inlined equivalent)?

Comment by Ghadi Shayban [ 11/Apr/14 5:13 PM ]

While I agree that there are improvements and possibly low-hanging fruit, FWIW https://github.com/clojure/tools.analyzer/commit/cf7dda81a22f4c9c1fe64c699ca17e7deed61db4#commitcomment-5989545

showed a 5% slowdown from a few callsites in tools.analyzer.

This ticket's benchmark is incomplete in that it covers a single type of argument (chunked range), and flawed as it timing the expense of realizing the range. (That could be a legit benchmark case, but it shouldn't be the only one).

Sorry to rain on a parade. I promise like speed too!





[CLJ-887] Error when calling primitive functions with destructuring in the arg vector Created: 29/Nov/11  Updated: 03/Apr/14

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

Type: Defect Priority: Major
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-don-t-remove-meta-from-arg-vector-in-maybe-destructu.patch    
Patch: Code
Approval: Triaged

 Description   

If one defines a primitive-taking function with destructuring, calling that function will result in a ClassCastException, IFF the primitive return-type hint is present.

Clojure 1.4.0-master-SNAPSHOT
user=> (defn foo [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
0
user=> (defn foo ^long [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL  user/eval9 (NO_SOURCE_FILE:4)
user=> (pst)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL
	user/eval9 (NO_SOURCE_FILE:4)
	clojure.lang.Compiler.eval (Compiler.java:6493)
	clojure.lang.Compiler.eval (Compiler.java:6459)
	clojure.core/eval (core.clj:2796)
	clojure.main/repl/read-eval-print--5967 (main.clj:244)
	clojure.main/repl/fn--5972 (main.clj:265)
	clojure.main/repl (main.clj:265)
	clojure.main/repl-opt (main.clj:331)
	clojure.main/main (main.clj:427)
	clojure.lang.Var.invoke (Var.java:397)
	clojure.lang.Var.applyTo (Var.java:518)
	clojure.main.main (main.java:37)
nil


 Comments   
Comment by Nicola Mometto [ 03/Apr/14 1:35 PM ]

This was happening because maybe-destructured returned the arg vector without the type hint, so the function was getting compiled to a IFn$OLLO rather than a IFn$OLLL but the :arglists vector in the var meta was still tagged, so the compiler thought that foo was a IFn$OLLL.

This patch addresses this by preserving the original meta on the fn arglist

Comment by Alex Miller [ 03/Apr/14 1:40 PM ]

Weirdly, I saw this happen today in my own code.





[CLJ-1361] Pretty printing code using pprint with code-dispatch incorrectly prints a simple ns macro call. Created: 18/Feb/14  Updated: 18/Feb/14

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

Type: Defect Priority: Minor
Reporter: Russ Olsen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print
Environment:

java version "1.6.0_65"
Java(TM) SE Runtime Environment (build 1.6.0_65-b14-462-11M4609)
Java HotSpot(TM) 64-Bit Server VM (build 20.65-b04-462, mixed mode)

Mac OS X 10.9.1


Attachments: Text File simple-ns-pprint-fix.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Pretty printing code using pprint with code-dispatch incorrectly prints a simple ns macro call.
The problem is that "nil" is added to the output after the namespace name.

user> (use 'clojure.pprint)
nil
user> (def code '(ns foo.bar))
#'user/code
user> (with-out-str (with-pprint-dispatch code-dispatch (pprint code)))
"(ns foo.barnil)\n"   ;; Expected: {{"(ns foo.bar)\n"}}

Cause: In clojure.pprint/pprint-ns-reference, reference is printed regardless, but may be nil.
Solution: Omit printing reference if nil.
Patch: simple-ns-pprint-fix.patch
Screened by:






[CLJ-1313] Correct a few unit tests Created: 23/Dec/13  Updated: 04/Feb/14

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

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

Attachments: File clj-1313-v1.diff     File clj-1313-v2.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Several unit tests do not test what they appear to have been intended to test, because of missing is statements around (= expr1 expr2) expressions, or because of use of (is (thrown? ...)) instead of (is (thrown-with-msg? ...))

Found with pre-release version of Eastwood Clojure lint tool.



 Comments   
Comment by Andy Fingerhut [ 23/Dec/13 3:11 AM ]

Patch clj-1313-v1.diff wraps (is ...) around several = expressions in unit tests that appeared to have been missing them, and changes several thrown? to thrown-with-msg? when there were regexes that were unused.

Comment by Stuart Halloway [ 31/Jan/14 12:36 PM ]

please update to apply cleanly on master

Comment by Andy Fingerhut [ 31/Jan/14 3:29 PM ]

clj-1313-v2.diff is identical to clj-1313-v1.diff except that it removes the portion that conflicts with the latest Clojure master. That portion needs updating for a different reason anyway (ticket CLJ-1328), and is probably best put into a patch for that ticket.





[CLJ-1254] Incorrect long quot result involving Long/MIN_VALUE Created: 06/Sep/13  Updated: 23/Nov/13

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math

Attachments: File clj-1254-2.diff    
Patch: Code and Test
Approval: Triaged

 Description   
user=> (quot Long/MIN_VALUE -1)
-9223372036854775808

Similar issue to CLJ-1222 and CLJ-1253, with the same root cause as described for CLJ-1225. Ticket filed separately from CLJ-1253 for long division / because the desired fix may be quite different in this case.

Rich Hickey stated in a comment on CLJ-1225 that this case should throw an exception.

Question: For inc (which throws when given input Long/MAX_VALUE) there is an auto-promoting inc' and an unchecked-inc. quot now throws an exception in this case. Should there be an auto-promoting quot' and an unchecked-quot?



 Comments   
Comment by Andy Fingerhut [ 06/Sep/13 10:55 AM ]

Patch clj-1254-v1.txt causes (quot Long/MIN_VALUE -1) to throw an exception due to overflow of the result, if the arguments are both long.

Unlike inc, which has auto-promoting version inc' and unchecked version unchecked-inc, there is no auto-promoting quot' and unchecked unchecked-quot. This patch does not add one.

Should quot' and unchecked-quot be added? If so, this ticket or a separate one?

Comment by Andy Fingerhut [ 23/Nov/13 12:59 AM ]

Patch clj-1254-2.diff is identical to clj-1254-v1.txt except it applies cleanly to latest master. The only changes were in the context of the lines that were changed, due to a recent commit made.





[CLJ-1253] Incorrect long division involving Long/MIN_VALUE Created: 06/Sep/13  Updated: 04/Oct/13

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math

Attachments: Text File clj-1253-1.txt    
Patch: Code and Test
Approval: Triaged

 Description   
user=> (/ Long/MIN_VALUE -1)
-9223372036854775808

Similar issue to CLJ-1222, with the same root cause as described for CLJ-1225.



 Comments   
Comment by Andy Fingerhut [ 06/Sep/13 8:56 AM ]

Patch clj-1253-1.txt corrects LongOps method divide for the case of args Long/MIN_VALUE and -1. It returns a BigInt in this case, not a Long, but most other pairs of values passed to this function return a Ratio exact answer, so it seems reasonable in this one case to return a BigInt exact answer when it will not fit in a Long.





[CLJ-1241] NPE when AOTing overrided clojure.core functions Created: 30/Jul/13  Updated: 31/Mar/14

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

Type: Defect Priority: Minor
Reporter: Phil Hagelberg Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: aot, compiler

Attachments: Text File 0001-fix-CLJ-1241.patch    
Patch: Code
Approval: Triaged

 Description   

When performing AOT compilation on a namespace that overrides a clojure.core function without excluding the original clojure.core function from the ns, you get a NullPointerException.

To reproduce:

$ lein new aot-get
$ cd aot-get
$ sed -i s/foo/get/
$ lein compile :all
WARNING: get already refers to: #'clojure.core/get in namespace: aot-get.core, being replaced by: #'aot-get.core/get
Exception in thread "main" java.lang.NullPointerException
	at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4858)
	at clojure.lang.Compiler$DefExpr.emit(Compiler.java:428)
	at clojure.lang.Compiler.compile1(Compiler.java:7152)
	at clojure.lang.Compiler.compile(Compiler.java:7219)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)

Cause: DefExpr.parse does not call registerVar for vars overridding clojure.core ones, thus when AOT compiling the var is not registered in the constant table.

Proposed: The attached patch makes DefExpr.parse call registerVar for vars overridding clojure.core ones.

Patch: 0001-fix-CLJ-1241.patch



 Comments   
Comment by Nicola Mometto [ 30/Jul/13 7:29 PM ]

DefExpr.parse was not calling registerVar for vars overridding clojure.core ones.

Comment by Alex Miller [ 31/Jul/13 12:25 AM ]

Verified on Clojure 1.5.1.

Comment by Javier Neira Sanchez [ 27/Aug/13 8:34 AM ]

Reproduced with `key` function without `(:refer-clojure :exclude [key])`

Comment by Rich Hickey [ 05/Sep/13 8:32 AM ]

This doesn't meet triage guidelines - i.e. there is this problem, therefore we will fix it by _____ so it then does _____

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

This is still present in the 1.6 release. I think it's mis-classified as low priority.

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

See for instance the cascalog mailing list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Comment by Andy Fingerhut [ 26/Mar/14 1:07 PM ]

It may help if someone could clarify Rich's comment.

Does it mean that the ticket should include a plan of the form "therefore we will fix it by _____ so it then does _____", but this ticket doesn't have that?

Or perhaps it means that the ticket should not include a plan of that form, but this ticket does? If so, I don't see it, except perhaps the very last sentence of the description. If that is a problem for vetting a ticket, perhaps we could just delete that sentence and proceed from there?

Something else?

Comment by Nicola Mometto [ 26/Mar/14 1:13 PM ]

Andy, I added the two last lines in the description after reading Rich's comment to explain why this bug happens and how the patch I attached works around this.

I don't know if this is what he was asking for though.

Comment by Alex Miller [ 27/Mar/14 11:00 AM ]

I think Rich meant that a ticket should have a plan of that form but does not. My own take on "triaged" is that it should state actual and expected results demonstrating a problem - I don't think it needs to actually describe the solution (as that can happen later in development). It is entirely possible that Rich and I differ in our interpretation of that. I will see if I can rework the description a bit to match what I've been doing elsewhere.

Comment by Andy Fingerhut [ 31/Mar/14 9:34 AM ]

Alex, I have looked through the existing wiki pages on the ticket tracking process, and do not recall seeing anything about this desired aspect of a triaged ticket. Is it already documented somewhere and I missed it? Not that it has to be documented, necessarily, but Rich saying "triage guidelines" makes it sound like a filter he applies that ticket creators and screeners maybe should know about.

Comment by Alex Miller [ 31/Mar/14 11:57 AM ]

To me, Triage (and Vetting) is all about having good problem statements. For a defect, it is most important to demonstrate the problem (what happens now) and what you expect to happen instead. I do not usually expect there to necessarily be "by ____" in the ticket - to me that is part of working through the solution (although it is typical to have this in an enhancement). This ticket, as it stands now, seems to have both a good problem statement and a good cause/solution statement so seems to exceed Triaging standards afaik.

Two places where I have tried to write about these things in the past are http://dev.clojure.org/display/community/Creating+Tickets and in the Triage process on the workflow page http://dev.clojure.org/display/community/JIRA+workflow.





[CLJ-1225] quot overflow issues around Long/MIN_VALUE for BigInt Created: 25/Jun/13  Updated: 04/Oct/13

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math

Attachments: Text File clj-1225-2.txt     Text File clj-1225-fix-division-overflow-patch-v1.txt    
Patch: Code and Test
Approval: Triaged

 Description   

In Clojure 1.5.1, see the following undesirable behavior regarding incorrect quot results for BigInts:

user=> (quot Long/MIN_VALUE -1N)
-9223372036854775808N
user=> (quot (bigint Long/MIN_VALUE) -1)
-9223372036854775808N

Similar issue to CLJ-1222. The root cause is that Java division of longs gives a numerically incorrect answer of Long.MIN_VALUE for (Long.MIN_VALUE / -1), because the numerically correct answer does not fit in a long. I believe this is the only pair of arguments for long division that gives a numerically incorrect answer, because division with a denominator having an absolute value of 2 or more gives a result closer to 0 than the numerator, and everything works fine for a denominator of 1 or -1, except this one case.

Related issues: CLJ-1222 for multiply, CLJ-1253 for / on longs, CLJ-1254 for quot on longs



 Comments   
Comment by Andy Fingerhut [ 25/Jun/13 11:03 AM ]

Patch clj-1225-fix-division-overflow-patch-v1.txt dated Jun 25 2013 may be one good way to address this issue. It modifies quot and / to return the numerically correct (BigInt) answer when given args Long/MIN_VALUE and -1.

It also removes the quotient intrinsic that does a JVM LDIV operation on longs for quot, since that operation is one of those that gives the incorrect result. I have not done any performance testing with this patch yet, but I have verified that it does not introduce any new reflection warnings when compiling Clojure itself.

Comment by Andy Fingerhut [ 25/Jun/13 11:13 AM ]

Another possible approach would be to create unchecked-quotient and quot', which together with quot would correspond to the existing unchecked-multiply, *' and *. That is a more significant change. One potential concern it addresses that patch clj-1225-fix-division-overflow-patch-v1.txt does not is that patch leaves a Clojure developer with no way to do a primitive Java long division except by writing Java code.

Comment by Rich Hickey [ 05/Sep/13 8:39 AM ]

this is two separate issues, one with longs and one with bigints. long problem should throw

Comment by Andy Fingerhut [ 06/Sep/13 12:37 AM ]

Updating description for BigInt issue only. Will create separate ticket for incorrect behavior of / and quot on long type args Long/MIN_VALUE and -1.

Comment by Andy Fingerhut [ 06/Sep/13 12:41 AM ]

Patch clj-1225-2.txt fixes this issue with quot on BigInts, with tests for quot and / on these values. / on BigInt worked fine before, but added the tests in case someone decides to change the implementation and forgets this corner case.





[CLJ-1185] `reductions should respect `reduced Created: 16/Mar/13  Updated: 14/Apr/14

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File CLJ-1181-v001.patch    
Patch: Code
Approval: Triaged

 Description   

This returns 16:

(reduce (fn [acc x]
          (let [x' (* x x)]
            (if (> x' 10)
              (reduced x')
              x')))
        (range))

But replacing `reduce with `reductions will never terminate:

(reductions (fn [acc x]
              (let [x' (* x x)]
                (if (> x' 10)
                  (reduced x')
                  x')))
            (range))

This is because `reductions ignores 'clojure.lang.Reduced, it never tests for `reduced?

I know that I only just discovered the `reduced, function, but `reductions is a big part of my debugging process, so it's unfortunate that they don't work together.



 Comments   
Comment by Brandon Bloom [ 16/Mar/13 6:10 PM ]

Attaching patch

Comment by Satshabad Khalsa [ 13/Apr/14 1:53 AM ]

Would love some progress on this!

Comment by Andy Fingerhut [ 13/Apr/14 11:37 AM ]

It isn't guaranteed to help, but it can't hurt to vote on the ticket, and encourage anyone else you know who wants this fixed to vote on it.





[CLJ-1134] star-directive in clojure.pprint/cl-format with an at-prefix ("~n@*") do not obey its specifications Created: 18/Dec/12  Updated: 14/Apr/14

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

Type: Defect Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Attachments: Text File clj-1134-star-directive-in-cl-format.txt    
Patch: Code and Test
Approval: Triaged

 Description   

The star-directive in clojure.pprint/cl-format with an at-prefix (~n@*) does not obey its specifications according to Common Lisp the Language, 2nd Edition. There are two bugs within ~n@* as of right now:

  1. When ~n@* is supposed to jump forward over more than one argument, it jumps one step backward as if it had seen ~:*. For instance, (cl-format nil "~D ~3@*~D" 0 1 2 3) will return "0 0" and not "0 3" as expected.
  2. When ~@* is seen, the formatter is supposed to jump to the first argument (as n defaults to 0, see specification linked above). However, whenever a ~@*-directive is seen, the formatter jumps to the second argument instead.

What (small set of) steps will reproduce the problem?

Inside a clean Clojure repl, perform these steps:

user=> (require '[clojure.pprint :refer [cl-format]])
nil
user=> (cl-format nil "~D ~3@*~D" 0 1 2 3)
"0 0"                                           ;; Expected: "0 3"
user=> (cl-format nil "~D~D~D~D ~@*~D" 0 1 2 3)
"0123 1"                                        ;; Expected: "0123 0"

What is the expected output? What do you see instead?

The expected output is "0 3" and "0123 0", but is "0 0" and "0123 1" as shown above.

What version are you using?

Tested on both 1.4.0 and 1.5.0-beta2, both have the defect described.

Please provide any additional information below.

The format strings which reproduces the problem has been compared with the format function from the Common Lisp implementations SBCL, CLisp and Clozure. All of them print the expected output.



 Comments   
Comment by Jean Niklas L'orange [ 18/Dec/12 9:28 PM ]

Patch attached.

It may be easier to read the changes the patch does from within JIRA instead from the commit message, so I've added it here:

This solves two issues as specified by #CLJ-1134. Issue #1 is solved by doing a
relative jump forward within absolute-reposition in cl_format.clj, line 114 by
switching (- (:pos navigator) position) with (- position (:pos navigator)).

Issue #2 is handled by changing the default n-parameter to * depending on
whether the @-prefix is placed or not. If it is placed, then n defaults to
0, otherwise it defaults to 1.

In addition, new tests have been appended to test_cl_format.clj to ensure the
correctness of this patch. The tests have been tested on the Common Lisp
implementation GNU CLISP 2.49, which presumably handle the ~n@*
correctly. This patch and GNU CLISP returns the same output for each format
call, sans case for printed symbols; Common Lisp has case-insensitive symbols,
whereas Clojure has not.

Comment by Tom Faulhaber [ 14/Apr/14 11:12 AM ]

I walked through this patch and it looks just right. Thanks!

Let's get it applied for 1.7.





[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 10/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Approval: Triaged

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch:

Screened by:






[CLJ-1384] clojure.core/set should use transients Created: 15/Mar/14  Updated: 11/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: performance

Attachments: Text File CLJ-1384-p1.patch     File set-bench.tar    
Patch: Code
Approval: Triaged

 Description   

clojure.core/vec calls (more or less) PersistentVector.create(...), which uses a transient vector to build up the result.

clojure.core/set on the other hand, calls PersistentHashSet.create(...), which repeatedly calls .cons on a PersistentHashSet, with all the associated speed/GC issues.

Operation count now w/transients
set 5 1.771924 µs 1.295637 µs
into 5 1.407925 µs 1.402995 µs
set 1000000 2.499264 s 1.196653 s
into 1000000 0.977555 s 1.006951 s


 Comments   
Comment by Gary Fredericks [ 15/Mar/14 10:13 PM ]

PersistentHashSet has six methods for creating sets – one for each combination of {with check, without check} and {array (varargs), List, ISeq}. Each of them does not use transients but could.

I believe clojure.core/set only depends on the (without check, ISeq) version.

Should all of these be changed? Three of them? One of them?

Comment by Andy Fingerhut [ 15/Mar/14 10:21 PM ]

I believe that the 'with check' versions are only intended to be used when reading set literals in Clojure source code, and give an error if there are duplicate elements. If you find examples where those set creation functions are called in other situations, I would be interested to learn about them to find out where my misunderstanding lies, or whether that is a problem with the current code.

If the belief above is correct, I would suggest not changing the 'with check' versions, since their speed isn't as critical.

Comment by Gary Fredericks [ 15/Mar/14 10:23 PM ]

Thanks Andy, I'll submit a patch that changes the three non-checked methods.

Comment by Gary Fredericks [ 15/Mar/14 10:46 PM ]

Attached CLJ-1384-p1.patch, which updates the three non-check create methods.

I also added benchmarks. It's about 2-3 times faster for large collections.

Comment by Ambrose Bonnaire-Sergeant [ 11/Apr/14 11:15 AM ]

Added benchmark suite (set-bench.tar).

FWIW results are similar to gfrederick's on my machine:

Clojure 1.6

Small collections (5 elements)

set averages 1.220601 µs
into averages 1.597991 µs

Large collections (1,000,000 elements)

set averages 2.429066 sec
into averages 1.006249 sec

After transients

Small collections (5 elements)

set averages 999.248325 ns
into averages 1.162889 µs

Large collections (1,000,000 elements)

set averages 1.003792 sec
into averages 889.993185 ms

Add full output to the tar.

Comment by Ghadi Shayban [ 11/Apr/14 11:35 AM ]

CLJ-1192 is related to this, but and Andy seems to be indicating the use of reduce as the means to better performance there.

Comment by Gary Fredericks [ 11/Apr/14 11:41 AM ]

Oh that's a good point about reduce. The difference should only apply to chunked seqs, right? It's worth noting that the benchmarks above used range which creates chunked seqs, so that might be why into looks faster on the large collections?

So this change only makes set act like vec; I think whether either/both of them should use reduce is a different question.





[CLJ-1378] Hints don't work with #() form of function Created: 11/Mar/14  Updated: 12/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, typehints

Attachments: File clj-1378.diff     File clj-1378-v2.diff    
Patch: Code
Approval: Triaged

 Description   
;; WORKS
(deftest test-add-job
  (let [pool (java.util.concurrent.Executors/newFixedThreadPool 10)
        counter 50000
        f (fn [])]
    (dotimes [i counter]
      (.submit pool ^Runnable  f ))
    (Thread/sleep 1000)
    (is (= counter (count (all-jobs))))))

;; FAILS
(deftest test-add-job
  (let [pool (java.util.concurrent.Executors/newFixedThreadPool 10)
        counter 50000]
    (dotimes [i counter]
      (.submit pool ^Runnable  #()))
    (Thread/sleep 1000)
    (is (= counter (count (all-jobs))))))

Caused by: java.lang.IllegalArgumentException: More than one matching method found: submit
	at clojure.lang.Compiler.getMatchingParams(Compiler.java:2380)
	at clojure.lang.Compiler$InstanceMethodExpr.<init>(Compiler.java:1412)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:952)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6560)
	... 87 more


 Comments   
Comment by Jozef Wagner [ 12/Mar/14 4:03 AM ]

Functions do have metadata, but Compiler does not look in them for type hints.

user=> (with-meta #() {:foo :bar})
#<clojure.lang.AFunction$1@779325ee>

When compiler is determining which native method to use, it matches method signature with classes of given args. There is a getJavaClass() method in Compiler.java which returns a class for given expression. Vars expressions and local bindings use :tag metadata to override this class, but most other expressions don't. Compiler parses #() into a FnExpr, which always return AFunction as its class.

Most of time this approach is OK, as AFunction implements Runnable and Callable so there is no need for type hint. However, in this particular case, there are overrides for both Runnable and Callable, and as AFunction can be either of them, the expression is ambiguous.

Comment by Jozef Wagner [ 12/Mar/14 4:17 AM ]

Patch added, following expression will now run without error

(.submit (java.util.concurrent.Executors/newCachedThreadPool) ^Runnable #())
Comment by Alex Miller [ 12/Mar/14 9:34 AM ]

Could you add a test to the patch?

Comment by Jozef Wagner [ 12/Mar/14 2:53 PM ]

Attached patch clj-1378-v2.diff which contains both fix and test.





[CLJ-1373] LazySeq should utilize cached hash from its underlying seq. Created: 09/Mar/14  Updated: 20/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, performance
Environment:

1.6.0 master SNAPSHOT


Attachments: File clj-1373.diff    
Patch: Code
Approval: Triaged

 Description   

Even if underlying seq contains a cached hash, LazySeq computes it every time.

user=> (def a (range 100000))
#'user/a
user=> (time (hash a))
"Elapsed time: 46.904273 msecs"
375952610
user=> (time (hash a)) ;; RECOMPUTE
"Elapsed time: 10.879098 msecs"
375952610
user=> (def b (seq a))
#'user/b
user=> (time (hash b))
"Elapsed time: 10.572522 msecs"
375952610
user=> (time (hash b)) ;; CACHED HASH
"Elapsed time: 0.024927 msecs"
375952610
user=> (def c (lazy-seq b))
#'user/c
user=> (time (hash c))
"Elapsed time: 12.207651 msecs"
375952610
user=> (time (hash c))
"Elapsed time: 10.995798 msecs"
375952610


 Comments   
Comment by Jozef Wagner [ 09/Mar/14 9:20 AM ]

Added patch which checks if underlying seq implements IHashEq and if yes, uses that hash instead of recomputing.





[CLJ-1315] Don't initialize classes when importing them Created: 28/Dec/13  Updated: 02/Jan/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.1, Release 1.2, Release 1.3, Release 1.4, Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: aot, compiler, interop

Attachments: Text File 0001-Don-t-initialize-classes-during-import.patch    
Patch: Code
Approval: Triaged

 Description   

Problem: When classes are imported in Clojure, the class is loaded using Class.forName(), which causes its static initialisers to be executed. This differs from Java where compilation does not cause classes to be loaded.

Motivation: In many cases when those classes are normally loaded by Java code during execution of a framework of some kind (IntelliJ in my case, and RoboVM is another culprit mentioned in that thread) the initialiser expects some infrastructure to be in place and will fail when it's not. This means that it's impossible to AOT compile namespaces importing these classes, which is a fairly serious limitation.

Approach: Modify ImportExpr to call RT.classForNameNonLoading() instead of Class.forName(), which will load the class but not initialise it. This change causes the Clojure test suite to fail, since clojure.test-clojure.genclass imports a gen-class'ed class which no longer loads its namespace on initialisation. I'm not sure if this is considered an incorrect use of such a class (IIRC with records it's required to import the class and require its namespace), but given that it's in the Clojure test case it's reasonable to assume that this fix would be a breaking change for more code out there. This test failure is also corrected in the attached patch.

Patch: 0001-Don-t-initialize-classes-during-import.patch

Alternative: This patch enables the change unconditionally, but depending on the extent of breakage it causes, it might need to be enabled with a configuration flag. I propose we make it unconditional in an early 1.7 beta and monitor the fall-out.

Background: This issue has been discussed in the following threads
https://groups.google.com/d/topic/clojure/tWSEsOk_pM4/discussion
https://groups.google.com/forum/#!topic/clojure-dev/qSSI9Z-Thc0



 Comments   
Comment by Alex Miller [ 29/Dec/13 12:23 PM ]

From original post:

This issue was originally reported by Zach Oakes and Colin Fleming and this patch was also tested by Colin.

I'm duplicating here my suggested release notes for this issue, which includes my current thoughts on potential breakage (it's also in the commit message of the patch):

    "import" no longer causes the imported class to be initialized. This
    change better matches Java's import behavior and allows the importing of
    classes that do significant work at initialization time which may fail.
    This semantics change is not expected to effect most code, but certain
    code may have depended on behavior that is no longer true.

    1) importing a Class defined via gen-class no longer causes its defining
    namespace to be loaded, loading is now deferred until first reference. If
    immediate loading of the namespace is needed, "require" it directly.
    2) Some code may have depended on import to initialize the class before it
    was used. It may now be necessary to manually call (Class/forName
    "org.example.Class") when initialization is needed. In most cases, this
    should not be necessary because the Class will be initialized
    automatically before first use.




[CLJ-1297] try to catch using - instead of _ in filenames so the compiler can give a better error message for people who don't know that you need to use _ in file names Created: 19/Nov/13  Updated: 28/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: compiler, errormsgs

Attachments: File better-error-messages-for-require.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Problem: Clojure requires the files that back a namespace that has dashes in it to have the dashes replaced with underscores on the filesystem (ie a.b_c.clj for namespace a.b-c). If you require a file that has been mistakenly saved as b-c.clj instead, you will get an error message:

Exception in thread "main" java.io.FileNotFoundException: Could not locate a/b_c__init.class or a/b_c.clj on classpath:
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5018.invoke(core.clj:5530)
	at clojure.core$load.doInvoke(core.clj:5529)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5336)
	at clojure.core$load_lib$fn__4967.invoke(core.clj:5375)
	at clojure.core$load_lib.doInvoke(core.clj:5374)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$load_libs.doInvoke(core.clj:5413)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$require.doInvoke(core.clj:5496)

Proposed:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existence, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Patch: better-error-messages-for-require.diff



 Comments   
Comment by Joshua Ballanco [ 20/Nov/13 12:15 AM ]

A perhaps even better solution would be to simply allow the use of dashes in *.clj[s] filenames. I can't imagine the extra disk access per-namespace would be a huge performance burden, and (since dashes aren't allowed currently) I don't think there would be any issues with backwards compatibility.

Comment by Gary Fredericks [ 20/Nov/13 8:40 AM ]

It's worth mentioning the combinatorial explosion for namespaces with multiple dashes – if I (require 'foo-bar.baz-bang), should clojure search for all four possible filenames? Does the jvm have a way to search for files by regex or similar to avoid nasty degenerate cases (like (require 'foo-------------))?

Comment by Joshua Ballanco [ 20/Nov/13 11:08 AM ]

According to the docs, the FileSystem class's "getPathMatcher" method accepts path globs, so you'd merely have to replace each instance of "-" or "_" with "{-,_}". Actual runtime characteristics would likely depend on the underlying filesystem's implementation.

Comment by Alex Miller [ 20/Nov/13 12:02 PM ]

I don't think the FileSystem stuff applies when looking up classes on the classpath. Note that Java class names cannot contain "-".

Comment by Phil Hagelberg [ 21/Nov/13 12:05 PM ]

According to the spec, Java class names can't contain dashes (though IIRC OpenJDK and Oracle's JDK accept them anyway) but the requirement that Clojure source files have names which align with their AOT'd class file eqivalents is something we've imposed upon ourselves. Introducing the disconnect between .clj files and .class files makes way more sense than disconnecting namespaces and .clj files, but arguably it's too late to fix that mistake.

In any case a check for dashed files (resulting only in a more informative compiler error, not a more permissive compiler) which only triggers when a .clj file cannot be found imposes zero overhead in the case where things are already working.

Comment by scott tudd [ 09/Dec/13 2:19 PM ]

As Clojure seems to be idiomatic to have sometimes-dashed-namespace-and-function-names as opposed to the ubiquitous camelCaseFunctionNames in java ... I agree to have the compiler automagically handle 'knowing' to look in dir_struct AND dir-struct for requisite files.

or at the least print out a nice message explaining the quirk when files "can't" be found ... WHEN there are dashes and underscores involved... anything to aid in helping things "just work" as one would think they're supposed to.

Comment by Obadz [ 12/Dec/13 5:28 AM ]

I would have saved a few hours as well.

Comment by Alexander Redington [ 14/Feb/14 2:29 PM ]

This patch changes clojure.core/load such that:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existance, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Comment by Andy Fingerhut [ 20/Mar/14 1:16 PM ]

I do not know whether it handles all of the cases proposed in this discussion, but I encourage folks to check out the filename/namespace consistency checking in the latest Eastwood release (version 0.1.1) to see if it catches the cases they would hope to catch. It does a static check based on the files in a Leiningen project, nothing at run time. https://github.com/jonase/eastwood

Of course changes to Clojure itself to give warnings about such things can still be very useful, since not everyone will be using a 3rd party tool to check for such things.





[CLJ-1289] aset-* and aget perform poorly on multi-dimensional arrays even with type hints. Created: 01/Nov/13  Updated: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: arrays, performance
Environment:

Clojure 1.5.1.

Dependencies: criterium


Attachments: Text File CLJ-1289-p1.patch    
Patch: Code
Approval: Triaged

 Description   

Here's a transcript of the behavior. I don't know for sure that reflection is being done, but the performance penalty (about 1300x) suggests it.

user=> (use 'criterium.core)
nil
user=> (def b (make-array Double/TYPE 1000 1000))
#'user/b
user=> (quick-bench (aget ^"[[D" b 304 175))
WARNING: Final GC required 3.5198021166354323 % of runtime
WARNING: Final GC required 29.172288684474303 % of runtime
Evaluation count : 63558 in 6 samples of 10593 calls.
             Execution time mean : 9.457308 µs
    Execution time std-deviation : 126.220954 ns
   Execution time lower quantile : 9.344450 µs ( 2.5%)
   Execution time upper quantile : 9.629202 µs (97.5%)
                   Overhead used : 2.477107 ns

One workaround is to use multiple agets.

user=> (quick-bench (aget ^"[D" (aget ^"[[D" b 304) 175))
WARNING: Final GC required 40.59820310542545 % of runtime
Evaluation count : 62135436 in 6 samples of 10355906 calls.
             Execution time mean : 6.999273 ns
    Execution time std-deviation : 0.112703 ns
   Execution time lower quantile : 6.817782 ns ( 2.5%)
   Execution time upper quantile : 7.113845 ns (97.5%)
                   Overhead used : 2.477107 ns

Cause: The inlined version only applies to arity 2, and otherwise it reflects.



 Comments   
Comment by Gary Fredericks [ 08/Dec/13 9:28 PM ]

A glance at the source makes it obvious that the hypothesis is correct – the inlined version only applies to arity 2, and otherwise it reflects.

I thought this would be as simple as converting the inline function to be variadic (using reduce), but after trying it I realized this is tricky as you have to generate the correct type hints for each step. E.g., given ^"[[D" the inline function needs to type-hint the intermediate result with ^"[D". This isn't difficult if we're just dealing with strings that begin with square brackets, but I don't know for sure that those are the only possibilities.

Comment by Yaron Peleg [ 13/Feb/14 4:44 AM ]

Bump. I just got bitten bad by this.

There are two seperate issues here:
1) (aget 2d-array-doubles 0 0 ) doesn't emit a reflection warning.
2) It seems like the compiler has enough information to avoid the reflective call here.

Note this gets exp. worse as number of dimensions grows, i.e (get doubles3d 0 0 0)
will be 1M slower, etc' Not true, unless you iterate over all elements. it's
simply n_dims*1000x per lookup.

Nasty surprise, especially considering you often go to primitive arrays for speed,
and a common use case is an inner loop(s) that iterate over arrays.

Comment by Gary Fredericks [ 13/Feb/14 7:08 AM ]

I can probably take a stab at this.

Comment by Gary Fredericks [ 13/Feb/14 8:34 PM ]

I think the reflection warning problem is pretty much impossible to solve without changing code elsewhere in the compiler, because the reflection done in aget is a different kind than normal clojure reflection – it's explicitly in the function body rather than emitted by the compiler. Since the compiler isn't emitting it, it doesn't reasonably know it's even there. So even if aget is fixed for other arities, you still won't get the warning when it's not inlined.

I can imagine some sort of metadata that you could put on a function telling the compiler that it will reflect if not inlined. Or maybe a more generic not-inlined warning?

The global scope of adding another compiler flag seems about balanced by the seriousness of array functions not being able to warn on reflection.

Comment by Gary Fredericks [ 13/Feb/14 8:52 PM ]

Attached CLJ-1289-p1.patch which simply inlines variadic calls to aget. It assumes that if it sees a :tag on the array arg that is a string beginning with [, it can assume that the return value from one call to aget can be tagged with the same string with the leading [ stripped off.

I'm not a jvm expert, but having read through the spec a little bit I think this is a reasonable assumption.

Comment by Alex Miller [ 14/Feb/14 3:34 PM ]

I think this probably is actually true, but a more official way to ask that question would be to get the array class and ask for Class.getComponentType() (and less janky than string munging).

Comment by Gary Fredericks [ 14/Feb/14 3:40 PM ]

How would you get the array class based on the :tag type hint?

Comment by Gary Fredericks [ 14/Feb/14 7:05 PM ]

I see (-> s (Class/forName) (.getComponentType) (.getName)) does the same thing – is that route preferred, or is there another one?





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

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

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

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

 Description   

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

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

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

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



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

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





[CLJ-1191] Improve apropos to show some indication of namespace of symbols found Created: 04/Apr/13  Updated: 28/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: repl

Attachments: Text File clj-1191-patch-v1.txt    
Patch: Code
Approval: Triaged

 Description   

apropos does find all symbols in all namespaces that match the argument, but the return value gives no clue as to which namespace the found symbols are in. It can even return multiple occurrences of the same symbol, which only gives a clue that the symbol exists in more than one namespace, but not which ones. For example:

user=> (apropos "replace")
(postwalk-replace prewalk-replace replace re-quote-replacement replace replace-first)

It would be nice if the returned symbols could indicate the namespace, either always, or if the found symbol is not in the current namespace.



 Comments   
Comment by Andy Fingerhut [ 04/Apr/13 8:25 PM ]

Path clj-1191-patch-v1.txt enhances apropos to put a namespace/ qualifier before every symbol found that is not in the current namespace ns. It also finds the shortest namespace alias if there is more than one. Examples of output with patch:

user=> (apropos "replace")
(replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (require '[clojure.string :as str])
nil
user=> (apropos "replace")
(replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace str/re-quote-replacement str/replace str/replace-first)

user=> (in-ns 'clojure.string)
#<Namespace clojure.string>
clojure.string=> (clojure.repl/apropos "replace")
(re-quote-replacement replace replace-by replace-first replace-first-by replace-first-char replace-first-str clojure.core/replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

Comment by Colin Jones [ 05/Apr/13 1:34 PM ]

+1

apropos as it already stands is quite helpful for already-referred vars, but not for vars that are only in other nses.

This update includes the information someone would need to further investigate the output.





[CLJ-1169] Add filename and line number to defn parameter declaration error Created: 22/Feb/13  Updated: 28/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Andrei Kleschinski Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs
Environment:

Windows


Attachments: Text File 0001-CLJ-1169-proposed-patch.patch     Text File 0002-CLJ-1169-fix-unit-tests.patch     File defn_error_message.clj    
Patch: Code
Approval: Triaged

 Description   

When mistyping parameter list in defn declaration, e.g.

(defn test
(some-error))

error message shows name of parameter (without quotes), but not function name, filename or line number:

Exception in thread "main" java.lang.IllegalArgumentException: Parameter declaration some-error should be a vector
at clojure.core$assert_valid_fdecl.invoke(core.clj:6567)
at clojure.core$sigs.invoke(core.clj:220)
at clojure.core$defn.doInvoke(core.clj:294)
at clojure.lang.RestFn.invoke(RestFn.java:467)
at clojure.lang.Var.invoke(Var.java:427)
at clojure.lang.AFn.applyToHelper(AFn.java:172)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.lang.Compiler.macroexpand1(Compiler.java:6366)
at clojure.lang.Compiler.macroexpand(Compiler.java:6427)
at clojure.lang.Compiler.eval(Compiler.java:6495)
at clojure.lang.Compiler.load(Compiler.java:6952)
at clojure.lang.Compiler.loadFile(Compiler.java:6912)
at clojure.main$load_script.invoke(main.clj:283)
at clojure.main$init_opt.invoke(main.clj:288)
at clojure.main$initialize.invoke(main.clj:316)
at clojure.main$null_opt.invoke(main.clj:349)
at clojure.main$main.doInvoke(main.clj:427)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:419)
at clojure.lang.AFn.applyToHelper(AFn.java:163)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.main.main(main.java:37)



 Comments   
Comment by Andrei Kleschinski [ 22/Feb/13 5:39 AM ]

Proposed patch for issue
Process exceptions in macroexpand1 and wraps them in CompilerException with source,line,column information.

Also patch adds quotes around invalid symbol name in error message to make them more distinguishable from rest of message.

Comment by Andy Fingerhut [ 01/Mar/13 9:32 AM ]

Patch 0001-CLJ-1169-proposed-patch.patch dated Feb 22 2013 causes several tests to fail. Run "./antsetup.sh" then "ant" to see which ones (or "mvn package").

Comment by Andrei Kleschinski [ 01/Mar/13 10:25 AM ]

Fix for failed unit-tests





[CLJ-1149] Unhelpful error message from :use (and use function) when arguments are malformed Created: 17/Jan/13  Updated: 28/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs

Approval: Triaged

 Description   

the following exception happens when you have something like this(bad):

(ns runtime.util-test
(:use [midje.sweet :reload-all]))

as opposed to any of these(correct):

(ns runtime.util-test
(:use midje.sweet :reload-all))

(ns runtime.util-test
(:use [midje.sweet] :reload-all))

and the exception is:
Exception in thread "main" java.lang.IllegalArgumentException: No value supplied for key: true
at clojure.lang.PersistentHashMap.create(PersistentHashMap.java:77)
at clojure.core$hash_map.doInvoke(core.clj:365)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:617)
at clojure.core$load_lib.doInvoke(core.clj:5352)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:619)
at clojure.core$load_libs.doInvoke(core.clj:5403)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:621)
at clojure.core$use.doInvoke(core.clj:5497)

Note that this is similar to the equally unhelpful message shown in http://dev.clojure.org/jira/browse/CLJ-1140 although that is a different root cause.

Probably best to enhance the `use` function to validate its arguments before trying to apply hash-map?



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

I believe this applies to require as well.





[CLJ-1107] 'get' should throw exception on non-Associative argument Created: 13/Nov/12  Updated: 26/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 7
Labels: None

Attachments: Text File 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch     Text File 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch     Text File clj-1107-throw-on-unsupported-get-v4.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The implementation of clojure.core/get returns nil if its argument is not an associative collection.

This behavior can obscure common programmer errors such as:

(def a (atom {:a 1 :b 2})

(:foo a)   ; forgot to deref a
;;=> nil

Calling get on something which is neither nil nor an Associative collection is almost certainly a bug, and should be indicated by an exception.

CLJ-932 was accepted as a similar enhancement to clojure.core/contains?

Patch: 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch

Approach: Throw IllegalArgumentException as final fall-through case in RT.getFrom instead of returning nil.



 Comments   
Comment by Andy Fingerhut [ 24/May/13 12:31 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt dated May 24 2013 is identical to 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch dated Nov 13 2012, except it applies cleanly to latest master. A recent commit for CLJ-1099 changed many IllegalArgumentException occurrences to Throwable in the tests, which is the only thing changed in this updated patch.

Comment by Andy Fingerhut [ 30/Jan/14 5:01 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt applied cleanly to latest Clojure master as of Jan 23 2014, but no longer does with commits made to Clojure between then and Jan 30 2014. I have not checked to see how difficult or easy it may be to update this patch.

Comment by Stuart Sierra [ 11/Feb/14 7:23 AM ]

New patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch created from master at 5cc167a.

Comment by Andy Fingerhut [ 26/Mar/14 11:55 AM ]

Patch clj-1107-throw-on-unsupported-get-v4.patch dated Mar 26 2014 is identical to Stuart Sierra's patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch, and retains his authorship. The only difference is in one line of diff context required in order to make it apply cleanly to latest master.





[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Aaron Bedra
Resolution: Unresolved Votes: 3
Labels: performance

Attachments: Text File 0001-Use-transient-map-in-zipmap.2.patch     Text File 0001-Use-transient-map-in-zipmap.patch    
Patch: Code
Approval: Triaged

 Description   

The attached patch changes zipmap to use a transient map internally. The definition is also moved so that it resides below that of #'transient. The original definition is commented out (like that of #'into).



 Comments   
Comment by Aaron Bedra [ 14/Aug/12 9:24 PM ]

Why is the old implementation left and commented out? If we are going to move to a new implementation, the old one should be removed.

Comment by Michał Marczyk [ 15/Aug/12 4:17 AM ]

As mentioned in the ticket description, the previously attached patch follows the pattern of into whose non-transient-enabled definition is left in core.clj with a #_ in front – I wasn't sure if that's something desirable in all cases.

Here's a new patch with the old impl removed.

Comment by Andy Fingerhut [ 15/Aug/12 10:37 AM ]

Thanks for the updated patch, Michal. Sorry to raise such a minor issue, but would you mind using a different name for the updated patch? I know JIRA can handle multiple attached files with the same name, but my prescreening code isn't quite that talented yet, and it can lead to confusion when discussing patches.

Comment by Michał Marczyk [ 15/Aug/12 10:42 AM ]

Thanks for the heads-up, Andy! I've reattached the new patch under a new name.

Comment by Andy Fingerhut [ 16/Aug/12 8:24 PM ]

Presumptuously changing Approval from Incomplete back to None after the Michal's updated patch was added, addressing the reason the ticket was marked incomplete.

Comment by Aaron Bedra [ 11/Apr/13 5:32 PM ]

The patch looks good and applies cleanly. Are there additional tests that we should run to verify that this is providing the improvement we think it is. Also, is there a discussion somewhere that started this ticket? There isn't a lot of context here.

Comment by Michał Marczyk [ 11/Apr/13 6:19 PM ]

Hi Aaron,

Thanks for looking into this!

From what I've been able to observe, this change hugely improves zipmap times for large maps. For small maps, there is a small improvement. Here are two basic Criterium benchmarks (transient-zipmap defined at the REPL as in the patch):

;;; large map
user=> (def xs (range 16384))
#'user/xs
user=> (last xs)
16383
user=> (c/bench (zipmap xs xs))
Evaluation count : 13920 in 60 samples of 232 calls.
             Execution time mean : 4.329635 ms
    Execution time std-deviation : 77.791989 us
   Execution time lower quantile : 4.215050 ms ( 2.5%)
   Execution time upper quantile : 4.494120 ms (97.5%)
nil
user=> (c/bench (transient-zipmap xs xs))
Evaluation count : 21180 in 60 samples of 353 calls.
             Execution time mean : 2.818339 ms
    Execution time std-deviation : 110.751493 us
   Execution time lower quantile : 2.618971 ms ( 2.5%)
   Execution time upper quantile : 3.025812 ms (97.5%)

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 25.4675 % Variance is moderately inflated by outliers
nil

;;; small map
user=> (def ys (range 16))
#'user/ys
user=> (last ys)
15
user=> (c/bench (zipmap ys ys))
Evaluation count : 16639020 in 60 samples of 277317 calls.
             Execution time mean : 3.803683 us
    Execution time std-deviation : 88.431220 ns
   Execution time lower quantile : 3.638146 us ( 2.5%)
   Execution time upper quantile : 3.935160 us (97.5%)
nil
user=> (c/bench (transient-zipmap ys ys))
Evaluation count : 18536880 in 60 samples of 308948 calls.
             Execution time mean : 3.412992 us
    Execution time std-deviation : 81.338284 ns
   Execution time lower quantile : 3.303888 us ( 2.5%)
   Execution time upper quantile : 3.545549 us (97.5%)
nil

Clearly the semantics are preserved provided transients satisfy their contract.

I think I might not have started a ggroup thread for this, sorry.





[CLJ-888] defprotocol should throw error when signatures include variable number of parameters Created: 29/Nov/11  Updated: 05/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Greg Chapman Assignee: Stuart Halloway
Resolution: Unresolved Votes: 2
Labels: errormsgs, protocols

Attachments: Text File 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I tried to use & in the signature for a method in defprotocol. Apparently (see below), this is compiled so that & becomes a simple parameter name, and there is no special handling for variable number of parameters. I think the use of & in a protocol signature ought to be detected and immediately cause an exception (I also think this restriction on the signatures ought to be documented; I couldn't find it specified in the current documentation, though of course it is implied (as I later realized) by the fact that defprotocol creates a Java interface).

user=> (defprotocol Applier (app [this f & args]))
Applier
user=> (deftype A [] Applier (app [_ f & args] (prn f & args) (apply f args)))
user.A
user=> (app (A.) + 1 2)
#<core$PLUS clojure.core$PLUS@5d9d0d20> 1 2
IllegalArgumentException Don't know how to create ISeq from: java.lang.Long
clojure.lang.RT.seqFrom (RT.java:487)



 Comments   
Comment by Alex Coventry [ 21/Oct/13 4:21 PM ]

Patch with test code attached. I have it throwing a CompilerException so that it shows source code location. Not sure whether this is kosher in clojure code, but I wish more macros provided this in their error handling.

Comment by Tassilo Horn [ 22/Oct/13 6:26 AM ]

This issue has already been discussed in CLJ-1024. There I provided a patch that forbids varargs and destructuring forms at various places including defprotocol/definterface. My patch had been applied shortly before clojure 1.5 was released, but it had a bug (forbid too many uses), so it got reverted and the bug closed and declined.

I was told to bring up the issue again after 1.5 has been released.

So here is my patch again. This time it's much more relaxed and only forbids varargs in defprotocol/definterface method declarations, and in deftype/defrecord and reify method implementations.

Comment by Alex Coventry [ 22/Oct/13 7:30 AM ]

Thanks, Tassilo. If there's anywhere in the JIRA system where I could check for prior work like that for other similar issues, I'd be grateful for a pointer.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 7:39 AM ]

New version of my patch.

Now I use a CompilerException with proper file/line/column information like Alex did. I also added his test case (which passes).

Concerning your question, Alex: a search for "varargs" would have listed CLJ-1024, but probably you wouldn't have looked into it anyway, because it's a closed issue...

Comment by Tassilo Horn [ 22/Oct/13 7:44 AM ]

Alex, if you don't object could we remove your patch in favor of mine which covers a bit more cases?

Comment by Alex Coventry [ 22/Oct/13 10:57 AM ]

Yep. Just read through 1024 and the associated mailing list discussion. You should totally get the credit: Your patch is more comprehensive and you have been on this a long time. Thanks for folding in the good parts of my patch.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 12:15 PM ]

Ok, great.

It seems I don't have the permissions to delete other peoples' attachments, so could you please delete your patch yourself?

Comment by Alex Coventry [ 23/Oct/13 2:44 PM ]

Sure, Tassilo. It's done.

I think this also needs a regression test for the case hugod originally pointed out. I initially made the same mistake as you there, but amalloy pointed it out[1] before I submitted the patch, so it is a natural mistake to make and should probably be documented in the source code.

Best regards,
Alex

[1] http://logs.lazybot.org/irc.freenode.net/%23clojure/2013-10-21.txt search for 14:48:34.

Comment by Tassilo Horn [ 24/Oct/13 2:00 AM ]

Alex, I've added the regression test you suggested. Thanks for pointing that out.

Also, I added tests checking definterface method declarations, and tests checking inline method implementations made with defrecord, deftype, and reify.

However, there's a problem with the tests for deftype and reify I don't know how to fix. When I eval the macroexpand forms used in the tests in a REPL, I can see that the CompilerException is successfully thrown and printed. But it also seems to be caught somewhere in the middle, so that the macroexpand returns a form and the exception doesn't make it to the (is (thrown? ...)). Therefore, I've commented the these tests and added a big FIXME.

Comment by Tassilo Horn [ 24/Oct/13 2:28 AM ]

New version of the patch with now all tests uncommented and passing. Andy Fingerhut made me aware that for the 4 deftype and reify tests, I need eval instead of just macroexpand.

Comment by Andy Fingerhut [ 25/Oct/13 6:25 PM ]

I have not investigated the reason yet, but patch 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch no longer applies cleanly after the latest commits to Clojure master on Oct 25 2013.

Comment by Tassilo Horn [ 28/Oct/13 2:21 AM ]

I've rebased the patch onto the current master so that it applies cleanly again.

Comment by Tassilo Horn [ 28/Oct/13 2:25 AM ]

Stu, I've assigned this issue to you because you've been assigned to CLJ-1165 which I have closed as duplicate of this issue.

One minor difference between my patch to this issue and CLJ-1165 is that here I use a CompilerException with file/line/column info whereas in CLJ-1165 I've used `ex-info`. I think the CE is more appropriate/informative, as the error is already triggered during macro expansion.





[CLJ-825] Protocol implementation inconsistencies when overloading arity Created: 08/Aug/11  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4, Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Carl Lerche Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: protocols
Environment:

All


Attachments: Text File clj-825-1.patch     File scribbles.clj    
Patch: Code and Test
Approval: Triaged

 Description   

The forms required for implementing arity-overloaded protocol methods are inconsistent between the "extend-*" macros and "defrecord".

The "extend" family of macros requires overloaded method definitions to follow the form used by defn:

(method ([arg1] ...) ([arg1 arg2] ...))

However, "defrecord" requires implementations to be defined separately:

(method [arg1] ...)
(method [arg1 arg2] ...)

Furthermore, the error modes if you get it wrong are unhelpful.

If you use the "defrecord" form with "extend-*", it evals successfully, but later definitions silently overwrite lexically previous definitions.

If you use the "extend-*" form with "defrecord", it gives a cryptic error about "unsupported binding form" on the body of the method.

This is not the same issue as CLJ-1056: That pertains to the syntax for declaring a protocol, this problem is with the syntax for implementing a protocol.

(defprotocol MyProtocol
  (mymethod
    [this arg]
    [this arg optional-arg]))

(extend-protocol MyProtocol
  Object
  (mymethod
    ([this arg] :one-arg)
    ([this arg optional-arg] :two-args)))

;; BAD! Blows up with "Unsupported binding form: :one-arg"
(defrecord MyRecord []
  MyProtocol
  (mymethod
    ([this arg] :one-arg)
    ([this arg optional-arg] :two-args)))

;; Works...
(defrecord MyRecord []
  MyProtocol
  (mymethod [this arg] :one-arg)
  (mymethod [this arg optional-arg] :two-args))

;; Evals...
(extend-protocol MyProtocol
  Object
  (mymethod [this arg] :one-arg)
  (mymethod [this arg optional-arg] :two-args))

;; But then... Error! "Wrong number of args"
(mymethod :obj :arg)

;; 2-arg version is invokable...
(mymethod :obj :arg1 :arg2)


 Comments   
Comment by Paavo Parkkinen [ 17/Nov/13 6:02 AM ]

Attached a patch for this.

For defrecord, I check which style is used for defining methods, and transform into the original style if the new style is used. For the check I do what I believe defn does, which is (vector? (first fdecl)).

For extend-*, I skip the checking, and just transform everything into the same format.

Tests included for both.

All tests pass.





[CLJ-735] Improve error message when a protocol method is not found Created: 04/Feb/11  Updated: 28/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs

Attachments: File protocolerr.diff    
Patch: Code
Approval: Triaged

 Description   

If you call a protocol function but pass the wrong arity (forget an argument for example), you currently a message that says "No single method ... of interface ... found for function ... of protocol ...". The code in question is getting matching methods from the Reflector and creates this message if the number of matches != 1.

There are really two cases there:

  • matches == 0 - this happens frequently due to typos
  • matches > 1 - this presumably happens infrequently

I propose that the == 0 case instead should have slightly different text at the beginning and a hint as to the intended arity within it:

"No method: ... of interface ... with arity ... found for function ... of protocol ...".

The >1 case should have similar changes: "Multiple methods: ... of interface ... with arity ... found for function ... of protocol ...".

Patch is attached. I used case which presumably should have better performance than a nested if/else. I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I did not add a test as I wasn't sure whether checking error messages in tests was appropriate or not. Happy to add that if requested.



 Comments   
Comment by Chas Emerick [ 14/Jul/11 6:39 AM ]

I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I think it should be the latter. The message is emitted when the protocol methods are being invoked through the corresponding function, so it should be consistent with the errors emitted by regular functions.

+1 for some tests, too. There certainly are tests for reflection warnings and such.

FWIW, I'm happy to take this on if Alex is otherwise occupied.





[CLJ-440] java method calls cannot omit varargs Created: 27/Sep/10  Updated: 04/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: interop

Approval: Triaged

 Description   

From http://groups.google.com/group/clojure/browse_thread/thread/7d0d6cb32656a621

E.g., trying to call java.util.Collections.addAll(Collection c, T... elements)

user=> (Collections/addAll [] (object-array 0))
false
user=> (Collections/addAll [])
IllegalArgumentException No matching method: addAll  clojure.lang.Compiler$StaticMethodExpr.<init> (Compiler.java:1401)

The Method class provides an isVarArg() method, which could be used to inform the compiler to process things differently.



 Comments   
Comment by Assembla Importer [ 27/Sep/10 8:19 PM ]

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

Comment by Alexander Taggart [ 01/Apr/11 11:16 PM ]

Patch adds support for varargs. Builds on top of patch in CLJ-445.

Comment by Alexander Taggart [ 05/Apr/11 5:45 PM ]

Patch updated to current CLJ-445 patch.

Comment by Nick Klauer [ 29/Oct/12 8:12 AM ]

Is this ticket on hold? I find myself typing (.someCall arg1 arg2 (into-array SomeType nil)) alot just to get the right method to be called. This ticket sounds like it would address that extraneous into-array arg that I use alot.

Comment by Andy Fingerhut [ 29/Oct/12 10:45 AM ]

fixbug445.diff uploaded on Oct 29 2012 was written Oct 23 2010 by Alexander Taggart. I am simply copying it from the old Assembla ticket tracking system to here to make it more easily accessible. Not surprisingy, it doesn't apply cleanly to latest master. I don't know how much effort it would be to update it, but only a few hunks do not apply cleanly according to 'patch'. See the "Updating stale patches" section on the JIRA workflow page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 29/Oct/12 10:56 AM ]

Ugh. Deleted the attachment because it was for CLJ-445, or at least it was named that way. CLJ-445 definitely has a long comment history, so if one or more of its patches address this issue, then you can read the discussion there to see the history.

I don't know of any "on hold" status for tickets, except for one or two where Rich Hickey has explicitly said in a comment that he wants to wait a while before making the change. There are just tickets that contributors choose to work on and ones that screeners choose to screen.





[CLJ-1386] Add transient? predicate Created: 17/Mar/14  Updated: 23/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Devin Walters Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, transient
Environment:

N/A


Attachments: Text File 0001-Add-transient-predicate.patch     Text File 0002-Add-transient-predicate.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've encountered situations where I wanted to check whether something was transient in order to know whether I should call assoc! or assoc, conj! or conj, etc.

This patch adds `transient?` as a predicate fn.



 Comments   
Comment by Alex Miller [ 17/Mar/14 10:21 AM ]

Patch needs a docstring and a test.

Comment by Devin Walters [ 17/Mar/14 4:42 PM ]

Alex: I figured that would be the case! Sorry about that. I've updated the patch. It now includes a docstring and has tests of `transient?` for #{}, [], and {}.

Thanks!

Comment by Alex Miller [ 17/Mar/14 9:48 PM ]

Thanks - please don't use the labels "patch" or "test" - those are covered by the Patch field.

Comment by Devin Walters [ 18/Mar/14 9:17 AM ]

Ah, sorry for the mixup Alex. I assumed you removed "patch" as a label the first time around to flag this ticket as still needing a vetted patch. My mistake.

Comment by Andy Fingerhut [ 21/Mar/14 1:42 PM ]

Patch 0001-Add-transient-predicate.patch dated Mar 17, 2014 applies cleanly to latest Clojure master, but fails a test because the new function transient? has no :added metadata. See most other Clojure functions in clojure.core for examples.

It also generates a new warning while running tests:

WARNING: transient? already refers to: #'clojure.core/transient? in namespace: clojure.test-clojure.data-structures, being replaced by: #'clojure.test-clojure.data-structures/transient?

There is an older (but equivalent) definition of transient? in test file data_structures.clj that should be removed when adding it to clojure.core

Comment by Devin Walters [ 22/Mar/14 11:29 PM ]

@Andy, the reason I did not add :added metadata is because I do not know if/when this patch will be accepted, and as a result, I don't really know if it will sneak into 1.6.X or 1.7. For now, I've put it in as 1.7. If it's in the running to be added sooner than that, let me know and I'll adjust it.

RE: The warning. Good catch. I've submitted a new patch which removes the private version of transient? from data_structures.clj. All tests pass.

Edit to Add: The latest patch as of this comment is now 0002-Add-transient-predicate.patch.





[CLJ-1351] Clojure emits an unused "swapThunk" method for functions with keyword callsites Created: 14/Feb/14  Updated: 14/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-remove-unused-swapThunk-method-generation.patch    
Patch: Code
Approval: Triaged

 Description   

This method is no longer used, I did a quick git blame and it look like it was used for an earlier implementation of keyword callsites and forgot to be removed in this commit https://github.com/clojure/clojure/commit/c7af275d4ee33cdc1794c8df8fa1e6d39039ac84

Removing this should reduce a bit the size of compile functions.






[CLJ-1316] for doesn't support :let binding as its first seq-expr Created: 30/Dec/13  Updated: 16/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Jay Fields Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

jvm clojure


Approval: Triaged

 Description   
user> (for [y [2 3 4] 
            :let [x 1]]
        [x y])
([1 2] [1 3] [1 4])
user> (for [:let [x 1]
            y [2 3 4]]
        [x y])
IllegalStateException Can't pop empty vector  clojure.lang.PersistentVector.pop (PersistentVector.java:380)

Cause:

Solution:

Patch:
Screened by:



 Comments   
Comment by Andy Fingerhut [ 30/Dec/13 9:53 AM ]

Related (perhaps identical?) ticket CLJ-207 was declined.

Comment by Jay Fields [ 30/Dec/13 10:03 AM ]

It does look like a duplicate. I find it surprising that this doesn't work, but it does work for doseq:

main=> (doseq [:let [x 1] y [2 3 4]] (println x y))
1 2
1 3
1 4
nil

I think you'll keep getting this bug report as long as that inconsistency exists.

Comment by Jay Fields [ 30/Dec/13 10:05 AM ]

for completeness, I think it's worth mentioning that I can't simply change the ordering (like Alex's example above), due to the cost of the value I'm calculating. I only want it to occur once, and I have to use a separate 'let (as Rich recommended)

Comment by Gary Fredericks [ 05/Jan/14 3:37 PM ]

Brandon Bloom pointed out that one difference between for and doseq is that for is lazy, and so for an initial :let it's not clear whether it should be evaluated immediately or after the first item is requested. doseq doesn't have that ambiguity.

Comment by Jay Fields [ 08/Jan/14 10:42 AM ]

@Gary, I think that's a good question, but either choice would be better than the current inconsistency. If you made it lazy, I can't really think of a downside. Even if it wasn't lazy, that would match the current performance characteristics of code that's already wrapping the for in a let.





[CLJ-1295] Speed up dissoc on array-maps Created: 15/Nov/13  Updated: 15/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: File clj-1295-1.diff    
Patch: Code
Approval: Triaged

 Description   

In latest Clojure master as of Nov 15 2013, the method without() in PersistentArrayMap.java first searches for a matching key using indexOf(key) and saves the result in i.

If a matching key was found, the code then copies the old array to the new smaller one, but unnecessarily repeats the comparison of every key in the map to the key being removed, even though its location is already stored in i.



 Comments   
Comment by Andy Fingerhut [ 15/Nov/13 7:05 PM ]

The patch clj-1295-1.diff changes PersistentArrayMap's without() to use System.arraycopy to copy only the necessary parts from the current array to newArray, similar to PersistentHashMap's method removePair().

Benchmark 1 has strings for keys, which are relatively slow to compare to each other.

(def m1 (array-map "abcdef" 1 "abcdeg" 2 "abcdeh" 3 "abcdei" 4))
(time (dotimes [i 100000000] (dissoc m1 "abcdei")))

1.6.0-alpha2 with no changes:
"Elapsed time: 29663.443 msecs"
"Elapsed time: 29490.225 msecs"
"Elapsed time: 29600.138 msecs"
"Elapsed time: 29627.948 msecs"

1.6.0-alpha2 with patch clj-1295-1.diff:
"Elapsed time: 6362.006 msecs"
"Elapsed time: 6121.006 msecs"
"Elapsed time: 6163.377 msecs"
"Elapsed time: 6155.299 msecs"
"Elapsed time: 6395.224 msecs"

Averages about 21% of the run time before the change.

Benchmark 2 has keywords for keys, which are compared via Java ==, so as fast as comparison can get.

(def m2 (array-map :abcdef 1 :abcdeg 2 :abcdeh 3 :abcdei 4))
(time (dotimes [i 100000000] (dissoc m2 :abcdei)))

1.6.0-alpha2 with no changes:
"Elapsed time: 5033.863 msecs"
"Elapsed time: 5028.327 msecs"
"Elapsed time: 5045.019 msecs"
"Elapsed time: 5004.751 msecs"
"Elapsed time: 5039.143 msecs"

1.6.0-alpha2 with patch clj-1295-1.diff:
"Elapsed time: 2874.748 msecs"
"Elapsed time: 2862.878 msecs"
"Elapsed time: 2887.778 msecs"
"Elapsed time: 2874.196 msecs"
"Elapsed time: 2861.807 msecs"

Averages about 57% of the run time before the change.





[CLJ-1293] Portable "catch-all" mechanism Created: 05/Nov/13  Updated: 05/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1293-v001.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Design page: http://dev.clojure.org/display/design/Platform+Errors

CLJS ticket/patch: http://dev.clojure.org/jira/browse/CLJS-661

This patch is more permissive than my patch for CLJS: The CLJS patch ensures :default catch blocks occur between non-default catch blocks and finally blocks, if present. This patch just makes (catch :default ...) a synonym for (catch Throwable ...). I wanted to keep the change to the compiler minimum.






[CLJ-1282] The quote special form should throw an exception if passed more than one form to quote Created: 23/Oct/13  Updated: 16/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, reader

Attachments: Text File CLJ-1282-p1.patch     Text File CLJ-1282-p2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Quote currently ignores all but the first argument. In the case of being called accidentally with multiple values, it should throw an exception specifying the error.

user> (quote 1 2 3)
1

------- Original: --------

Every once in a while, you can just go down the rabbit hole.

I had an errant expression in my code:

(-> message get-message-values 'DESTINATION_MERCHANT_ID)

One would think this would work; it certainly would if the key was a keyword and not a symbol.

One would expect this to expand to:

('DESTINATION_MERCHANT_ID (get-message-values message))

however, the reader is involved, so it is as if the source were:

(-> message get-message-values (quote DESTINATION_MERCHANT_ID))

which expands to:

(quote (-> message get-message-values) DESTINATION_MERCHANT_ID))

... hilarity ensues! Because quote currently ignores extra parameters, my code gets the quoted value '(clojure.core/-> message get-message-values) rather than the expected string from the map; this shifts us from the "there's a bug in my code" to "the nature of reality is broken".

The correct expression is:

(-> message get-message-values (get 'DESTINATION_MERCHANT_ID))

This took quite a while to track down; if the

special form checked that it was passed exactly one form to quote and threw an exception otherwise, I think I would have caught this much earlier. It could even identify the expression it is quoting, which would provide a lot better understanding of where I went wrong.



 Comments   
Comment by Howard Lewis Ship [ 23/Oct/13 2:02 PM ]

Sorry, can't edit the description now to correct the formatting errors.

Comment by Howard Lewis Ship [ 24/Oct/13 6:09 PM ]

I just wanted to point out that your description is shorter, but makes it appear that such a use is unlikely and therefore unimportant; the detail of my description is to point out a reasonable situation where something explicable, but completely counterintuitive and confusing, does occur.

Comment by Alex Miller [ 24/Oct/13 8:28 PM ]

That's why I left the original in there too.

Comment by Gary Fredericks [ 09/Dec/13 7:07 AM ]

(quote) currently returns nil. Do we have an opinion about that?

Comment by Gary Fredericks [ 09/Dec/13 9:32 AM ]

Attached p1, which throws an IllegalArgumentException (wrapped in a CompilerException of course) for anything but 1 arg, and includes the number of args that were passed.

I can't think of any reason why (quote) would be useful, so I decided to throw on that too. Very easy to change of course.

Also added a test that (eval '(quote 1 2 3)) throws.

Comment by Stuart Halloway [ 31/Jan/14 12:46 PM ]

I recommend the following changes:

  • throw an ex-info that includes the offending form in its map {:form ...}
  • check only for the map data, not exception type or message, in the tests
Comment by Andy Fingerhut [ 31/Jan/14 6:31 PM ]

Patch CLJ-1282-p1.patch no longer applies cleanly after commits made to Clojure master on Jan 31 2014, probably due to the patch committed for CLJ-1318, and probably only because some lines of context changed in the test file. That would be trivial to update, but Stu's comments above suggest that more significant changes need to be made.

Comment by Gary Fredericks [ 01/Feb/14 9:19 AM ]

Throwing an ex-info is easy enough. I don't know how to avoid at least incidentally checking for the exception type, since the ExceptionInfo is wrapped in a CompilerException. I'll make a patch that keeps the class name in the test but doesn't do any checks on the cause aside from the ex-data. Let me know if I should do anything different.

Comment by Gary Fredericks [ 01/Feb/14 9:58 AM ]

Attached CLJ-1282-p2.patch which is off of the current master and addresses Stu's points.

Comment by Alex Miller [ 04/Feb/14 11:23 PM ]

Moving back to Triaged for more looks.

Comment by Nicola Mometto [ 16/Feb/14 12:12 PM ]

Currently (quote) returns nil, is it intended that this patch makes that an error or was this by accident?

Comment by Gary Fredericks [ 16/Feb/14 12:23 PM ]

I consciously chose to make (quote) an error – I made a comment about that earlier and didn't get any feedback, so I unilaterally decided to make it an error due to the fact that I couldn't think of any possible use for (quote).

It's an easy switch if somebody thinks differently.

Comment by Nicola Mometto [ 16/Feb/14 1:13 PM ]

I'm sorry I did not notice your previois comment.
I'm asking because I need to know whether I should throw on (quote) for tools.analyzer, currently it is allowed but I too think that (quote) should be an error.





[CLJ-1277] Speed up printing of time instants Created: 10/Oct/13  Updated: 10/Oct/13

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File clj-1277-1.txt    
Patch: Code
Approval: Triaged

 Description   

There are several occurrences of reflection in instant.clj that slow down the printing of time instants.

Clojure Google group conversation link: https://mail.google.com/mail/u/0/?shva=1#label/clojure/1419e1e6f6cc5b3d

The addition of a few type hints is enough to speed the printing of time instants by a factor of about 3 to 4.5, in a few small benchmarks.



 Comments   
Comment by Andy Fingerhut [ 10/Oct/13 12:07 AM ]

Patch clj-1277-1.txt adds 4 type hints that eliminate all reflection occurrences in source file instant.clj. Benchmarks show that it speeds up printing of java.util.Date and java.sql.Timestamp objects by a factor of about 3 to 4.5.

Latest Clojure master as of Oct 9 2013:

user=> (time (let [d (java.util.Date.)] (dotimes [i 3000000] (pr-str d))))
"Elapsed time: 24094.282 msecs"
user=> (import 'java.sql.Timestamp)
user=> (time (let [d (java.sql.Timestamp. 1300000000000)] (dotimes [i 2000000] (pr-str d))))
"Elapsed time: 20856.957 msecs"

That version of Clojure plus the patch clj-1277-1.txt:

user=> (time (let [d (java.util.Date.)] (dotimes [i 3000000] (pr-str d))))
"Elapsed time: 5085.847 msecs"
user=> (time (let [d (java.sql.Timestamp. 1300000000000)] (dotimes [i 2000000] (pr-str d))))
"Elapsed time: 7582.233 msecs"

Comment by Alexander Kiel [ 10/Oct/13 4:54 AM ]

Thanks for the patch Andy. But I don't like the (set! warn-on-reflection true). I think its better to use it only in the dev profile in leiningen. Not in real production code.

Comment by Andy Fingerhut [ 10/Oct/13 4:06 PM ]

Alexander, Leiningen is not used for building Clojure itself. The two supported choices are Maven and ant. Several Clojure source files, e.g. core/protocols.clj and core/reducers.clj, set warn-on-reflection to true, I believe so that if code is changed in such a way as to introduce a warning, it will be caught more quickly.

If the screeners or Rich think it is inappropriate, it is easy enough to remove.

Comment by Alex Miller [ 10/Oct/13 4:48 PM ]

Setting that is not uncommon in core clojure code and seems fine to me here.





[CLJ-1259] Speed up pprint Created: 09/Sep/13  Updated: 13/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, print

Attachments: Text File clj-1259-1.txt    
Patch: Code
Approval: Triaged

 Description   

There are many occurrences of reflection in the pprint implementation.

By eliminating all of them, I ran one benchmark of pprint'ing a Clojure map that resulted in a 300 Kbyte output. After eliminating reflection, the elapsed time to pprint was reduced by 18% (about 14.0 sec down to about 11.5 sec) on a recent model MacBook Pro.



 Comments   
Comment by Andy Fingerhut [ 09/Sep/13 11:36 PM ]

Patch clj-1259-1.txt eliminates all occurrences of reflection in pprint, and all files loaded from pprint.clj. It also sets warn-on-reflection to true for those files, in hopes of making it more obvious if a new use of reflection is added there.





[CLJ-1219] Make identical? variadic Created: 19/Jun/13  Updated: 28/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Irakli Gozalishvili Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJ-1219-make-identical-variadic.patch    
Patch: Code and Test
Approval: Triaged

 Description   

(= 1 1 1) ;; => true
(= 1 1 2) ;; => false
(== 1 1 1) ;; => true
(== 1 1 2) ;; => false
(identical? 1 1 1) ;; ArityException Wrong number of args (3) passed to: core$identical-QMARK- clojure.lang.AFn.throwArity (AFn.java:437)

I think it would make far more sense to make identical? consistent with all other comparison operators
and allow it to take variadic number of arguments.



 Comments   
Comment by Tassilo Horn [ 03/Sep/13 9:43 AM ]

Here's a patch that makes identical? variadic and adds a test for identical?.





[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 14/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff    
Patch: Code
Approval: Triaged

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.





[CLJ-1117] partition docstring should be more explicit about dropped or partial trailing elements Created: 29/Nov/12  Updated: 28/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: docstring
Environment:

OS X, 10.8


Attachments: Text File clj-1117.patch    
Patch: Code
Approval: Triaged

 Description   

The doc for partition states "In case there are not enough padding elements, return a partition with less than n items." However, the behavior of this function is as follows:

user=> (partition 3 (range 10))
((0 1 2) (3 4 5) (6 7 8))
user=> (partition 4 (range 10))
((0 1 2 3) (4 5 6 7))

Proposed: The docstring should be updated to make it clear that not providing a pad means that items are dropped, and to also see partition-all.

Patch: clj-1117.patch



 Comments   
Comment by Andy Fingerhut [ 29/Nov/12 2:15 PM ]

That would be a potentially breaking change for some people's code that uses partition. partition-all behaves as you wish.

Also, your concern with the documentation is for when there are padding elements specified as an argument, but your examples don't specify any padding elements.

Comment by Timothy Baldridge [ 29/Nov/12 2:55 PM ]

I agree, but I think the docs should then explicitly state: "if no padding is given, not all input elements may be returned in the output partitions" or something to that line.

Comment by Andy Fingerhut [ 29/Nov/12 4:43 PM ]

More precise documentation of current behavior is always welcome in my opinion.

Comment by Gabriel Horner [ 17/May/13 10:14 AM ]

I've uploaded a patch that calls out when and how partition drops tail elements:
"If a pad collection is not supplied, any tail elements that remain from dividing the input collection length by n will not be included in a partition."





[CLJ-1094] Add zero-arity versions of every-pred and some-fn Created: 25/Oct/12  Updated: 08/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-Add-zero-arity-variants-for-every-pred-and-some-fn.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Zero-arity versions of these fns are useful in situations like the following:

;; compute-preds-for may return zero or many predicate fns
(let [preds (compute-preds-for something)]
  (filter (apply every-pred preds) some-coll))

Approach: This patch adds zero-arity versions of every-pred and some-fn with these semantics:

(every-pred) === (constantly true)
(some-fn)    === (constantly nil)

Patch: 0001-Add-zero-arity-variants-for-every-pred-and-some-fn.patch

  • Patch adds zero-arity version
  • Patch reformats docstring to 80 character width (matching other core fns) - comments are identical other than the addition of a final sentence stating behavior with zero-arity.

Screened by:



 Comments   
Comment by Tassilo Horn [ 25/Oct/12 7:12 AM ]

This is the thread where Max Penet suggested to have 0-arity versions of the two fns:

https://groups.google.com/forum/?fromgroups=#!topic/clojure/IRlN-4LH_U0

Comment by Alex Miller [ 07/Nov/13 8:20 PM ]

Tassilo, can you undo the line break changes on this patch to minimize the patch differences?

Comment by Tassilo Horn [ 08/Nov/13 4:00 AM ]

Rebased patch.

Comment by Tassilo Horn [ 08/Nov/13 4:05 AM ]

@Alex: That I've reformatted the docstring so that it doesn't look like a comb and fits in 80 columns is a feature. Should I really revert to the original, ugly, less readable formatting only to make the patch 8 lines shorter?

Comment by Alex Miller [ 08/Nov/13 9:14 AM ]

My goal was to simplify the patch so it is easier to review. Since the comment is identical (except for the last line), we can instead note that as a feature of the patch.





[CLJ-1078] Add queue and queue? to clojure.core Created: 26/Sep/12  Updated: 06/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: data-structures, queue

Attachments: File clj-1048-add-queue-functions.diff     Text File queue.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Add queue function to create queues from collections and queue? predicate to check queueness.

Patch: clj-1048-add-queue-functions.diff



 Comments   
Comment by Andy Fingerhut [ 28/Sep/12 8:43 AM ]

Timothy, I tried applying both of these Sep 26, 2012 patches to latest Clojure master as of that date. I had to apply 0001-make-PersistentQueue-ctor-public.patch by hand since it failed to apply using git or patch. It built fine, but failed to pass several of the Clojure tests. Have you looked into those test failures to see if you can find the cause and fix them? I tested on Ubuntu 11.10 with Oracle JDK 1.6 and 1.7, and saw similar failures with both.

Comment by Timothy Baldridge [ 26/Oct/12 5:23 PM ]

Fixed the patch. Tests pass, created the patch, applied it to a different copy of the source and the tests still pass. So this new patch should be good to go.

Comment by Andy Fingerhut [ 26/Oct/12 5:43 PM ]

Timothy, I'm not sure how you are getting successful results when applying this patch. Can you try the steps below and see what happens for you? I get errors trying to apply the patch with latest Clojure master as of Oct 26, 2012. Also please use the steps on the JIRA workflow page to create a git format patch (http://dev.clojure.org/display/design/JIRA+workflow under "Development" heading).

% git clone git://github.com/clojure/clojure.git
% cd clojure
% patch -p1 < queues.patch
patching file src/clj/clojure/core.clj
patching file src/jvm/clojure/lang/PersistentQueue.java
Hunk #1 FAILED at 32.
1 out of 1 hunk FAILED – saving rejects to file src/jvm/clojure/lang/PersistentQueue.java.rej
patching file test/clojure/test_clojure/data_structures.clj
Hunk #1 succeeded at 123 with fuzz 2.
Hunk #2 succeeded at 861 with fuzz 2.
Hunk #3 FAILED at 872.
1 out of 3 hunks FAILED – saving rejects to file test/clojure/test_clojure/data_structures.clj.rej
patching file test/clojure/test_clojure/java_interop.clj

Comment by Timothy Baldridge [ 26/Oct/12 6:08 PM ]

I was using git apply. I tried the method you show above, and now I'm seeing the same issues you show above.

Comment by Andy Fingerhut [ 26/Oct/12 6:26 PM ]

Just so you know, the preferred way to create and apply patches are the "git format-patch master --stdout > patch.txt" to create a patch (after doing the branching commands described on the JIRA workflow page to create a branch for your changes), and the "git am --keep-cr -s < patch.txt" to apply a patch. If a patch was created that way and applies cleanly with that command, then you are definitely good to go.

The "patch -p1 < patch.txt" command is just a secondary method sometimes used to try to apply patches that aren't in the format produced above, or have errors when applying using that method.

Comment by Timothy Baldridge [ 26/Oct/12 9:15 PM ]

Just so you know, the preferred way to create and apply patches are the "git format-patch master --stdout > patch.txt" to create a patch (after doing the branching commands described on the JIRA workflow page to create a branch for your changes), and the "git am --keep-cr -s < patch.txt" to apply a patch. If a patch was created that way and applies cleanly with that command, then you are definitely good to go.

The "patch -p1 < patch.txt" command is just a secondary method sometimes used to try to apply patches that aren't in the format produced above, or have errors when applying using that method.

Comment by Timothy Baldridge [ 26/Oct/12 9:16 PM ]

added patch

Comment by Andy Fingerhut [ 26/Oct/12 9:37 PM ]

That one applies cleanly and passes all tests. It should show up on the next list of prescreened patches. Thanks.

Comment by Rich Hickey [ 29/Nov/12 9:54 AM ]

we don't use the queue* convention elsewhere, e.g. vec and vector. I think queue should take a collection like vec and set. (queue [1 2 3]) could be made to 'adopt' the collection as front.

Comment by Andy Fingerhut [ 11/Dec/12 1:00 PM ]

Patch queue.patch dated Oct 26 2012 no longer applies cleanly after recent CLJ-1000 commits, but only because of one line of changed patch context. It still applies cleanly with "patch -p1 < queue.patch". Not bothering to update the stale patch given Rich's comments suggesting more substantive changes.

Comment by Steve Miner [ 06/Apr/13 8:06 AM ]

See also CLJ-976 (tagged literal support for PersistentQueue)

Comment by John Jacobsen [ 23/May/13 8:54 PM ]

Don't want to step on Timothy B's toes here, but it looks straightforward to adopt his patch to implement Rich's suggestion. I'd offer to give it a whack if nobody else wants the ticket now.

Comment by John Jacobsen [ 26/May/13 9:04 AM ]

Discussion initiated on clojure-dev: https://groups.google.com/forum/?fromgroups#!topic/clojure-dev/2BOqHm24Vc4

Comment by John Jacobsen [ 31/May/13 9:58 AM ]

This patch (if accepted) supersedes Timothy Baldridge's patch; it implements "queue" and "queue?" (but not "queue*"); "queue" accepts a collection rather than being a variadic function, as per Rich's suggestion.

Comment by Andy Fingerhut [ 30/Jan/14 5:00 PM ]

The patch clj-1048-queue-takes-collections.diff applied cleanly to latest Clojure master as of Jan 23 2014, but not on Jan 30 2014. There were several commits made to Clojure during that week involving updating the hash functions that conflict in some way with this patch. I have not checked to see how easy or difficult it might be to update the patch.

Comment by John Jacobsen [ 05/Feb/14 1:45 PM ]

Hi Andy, I updated the patch and removed my previous version. The new one should apply cleanly and pass all tests.

Comment by John Jacobsen [ 05/Feb/14 2:24 PM ]

Updated ticket title.

Comment by Alex Miller [ 05/Feb/14 5:33 PM ]

Hi John... Can you condense these changes into a single commit? Please also remove the comments above queue* in java_interop.clj. Thanks...

Comment by John Jacobsen [ 05/Feb/14 6:55 PM ]

Hi Alex, the updated patch removes that comment and rebases all three commits into c9f77dd. Let me know if you need anything else. Thanks!





[CLJ-1073] Make print-sequential interruptible Created: 21/Sep/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Colin Jones Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: print

Attachments: Text File 0001-Allow-thread-interruption-in-print-sequential.patch     Text File clj-1073-add-print-interruptibly-patch-v2.txt     File perftest-print.clj     File test.sh    
Patch: Code
Approval: Triaged

 Description   

This allows print-sequential to be interrupted by Thread.interrupt(), rather than requiring clients to resort to Thread.stop(). This is especially helpful when printing very large sequences.

See also clojure-dev discussion at https://groups.google.com/d/topic/clojure-dev/vs0RNUQXiYE/discussion

Patch: clj-1073-add-print-interruptibly-patch-v2.txt

Approach:

Add a new var print-interruptibly, similar to print-length, print-dup, etc. Its default is false. When true, (Thread/interrupted) is checked for after printing each element of a sequence. If true, the writer is flushed and an InterruptedIOException is thrown.

An alternative patch proposed earlier did not include print-interruptibly, but simply always checked (Thread/interrupted) after printing each element of a sequence. Benchmark tests showed that this could slow down printing of long sequences by about 10%. The approach in the proposed patch only incurs this slowdown if print-interruptibly is true. When false, there is no significant slowdown measured in benchmarks.



 Comments   
Comment by Andy Fingerhut [ 21/Sep/12 7:04 PM ]

In a quickly hacked up performance test on Mac OS X 10.6.8 + Oracle/Apple JDK 1.6.0_35 which I can attach, I saw about a 9% to 10% slowdown for Colin's patch in printing large vectors.

I have a modified patch that only calls Thread/interrupted after every 20 items are printed, and with that version the slowdown versus the original code was about 3% to 4%.

Colin, would there be any down side to checking Thread/interrupted less often for your purposes?

To run performance test, edit attachment compile.sh to point at your clojure.jar, put file perftest-print.clj in the same directory, and run ./compile.sh It should work on Mac or Linux.

Comment by Andy Fingerhut [ 22/Sep/12 10:47 AM ]

clj-1073-allow-thread-interrupt-in-print-sequential-patch.txt dated Sep 22 2012 is similar to Colin's patch 0001-Allow-thread-interruption-in-print-sequential.patch dated Sep 21 2012, except it only checks interrupted status every 20 (or maybe 21?) times through the loop in print-sequential. It is the one that is 3-4% slower than the current latest master Clojure code in my performance test mentioned above, versus Colin's patch which is about 9-10% slower on that test.

Comment by Stuart Halloway [ 19/Oct/12 3:31 PM ]

Is this primarily intended for dev-time use? I wouldn't want to lose performance for this if there is any way to implement it as a dev-time feature.

Comment by Colin Jones [ 19/Oct/12 4:27 PM ]

Andy: The only caveat I can think of with checking Thread/interrupted less often is in the case of deeply nested collections.

Stu: Dev-time use was the original reason for opening this, yes. But I can imagine it being needed for anytime a thread can be interrupted, whether that's by ctrl-c or other means.

My original thinking, performance-wise, was that once we're already doing IO, we're probably not too concerned with CPU-bound checks like this, so I didn't bother with it. I guess with an SSD that's not as likely to be true.

Locally (w/ my SSD), I'm seeing that Andy's benchmark of printing a million numbers is about a second slower than the baseline with my original patch (12.08s -> 13.10s), and about the same with Andy's patch (12.08s -> 11.75s). Decreasing to a thousand numbers doesn't really show any difference (every version completes in ~1.3s). Bumping to 2 million adds 2 seconds above the baseline with my patch, and Andy's is again just a bit faster than the baseline (somehow). Bumping to 5 million runs me out of heap space

Comment by Andy Fingerhut [ 08/Nov/12 4:16 PM ]

clj-1073-add-print-interruptibly-patch-v1.txt dated Nov 8 2012 is the same idea as Colin's patch 0001-Allow-thread-interruption-in-print-sequential.patch dated Sep 21 2012, except it only checks (Thread/interrupted) if a new var print-interruptibly is true. Its default value is false.

Performance results of the perftest-print.clj program, as driven by the test.sh script, for Clojure 1.5-beta1 and with two different patches. All run times are elapsed, in seconds, and sorted in increasing order for easier comparison.

Executive summary: Performance results when print-interruptibly is left at default false value are pretty much same as today. With print-interruptibly bound to true, performance results are slower, as expected, and about the same as with Colin's patch.

Original 1.5-beta1 unchanged:
13.75 13.80 13.83 13.87 13.93

With this new print-interruptibly patch, with print-interruptibly
at default false value:
13.86 13.91 14.01 14.08 14.14

With this new print-interruptibly patch, with print-interruptibly
bound to true while printing (so a slightly modified version of
perftest-print.clj that does (binding [*print-interruptibly* true]
...) around the heart of the code):
15.29 15.44 15.45 15.62 15.63

With patch 0001-Allow-thread-interruption-in-print-sequential.patch
applied:
15.38 15.46 15.48 15.49 15.50

Comment by Colin Jones [ 12/Nov/12 1:37 PM ]

Andy's latest patch looks good & makes it easy for the REPL and other interruptible scenarios to opt into the "safe" behavior. I would personally have preferred to have the "safe" behavior on by default, but can understand the performance concerns, and this gets me what I really wanted.

Comment by Timothy Baldridge [ 30/Nov/12 3:09 PM ]

Vetting as it sounds like the performance issues are taken care of.

Comment by Andy Fingerhut [ 13/Feb/13 12:28 AM ]

clj-1073-add-print-interruptibly-patch-v2.txt dated Feb 12 2013 is identical to clj-1073-add-print-interruptibly-patch-v1.txt dated Nov 8 2012 (soon to be removed) except that it applies cleanly to latest master.

Comment by Alex Miller [ 22/Aug/13 10:38 PM ]

Moving back to Triaged as Rich has not vetted.





[CLJ-1026] Mixed end-of-line endings in the source code Created: 17/Jul/12  Updated: 04/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: John Szakmeister Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-Introduce-end-of-line-normalization.patch     File clj-1026-bash-script.sh    
Patch: Code
Approval: Triaged

 Description   

While examining some of the Clojure source code, I discovered that some files had mixed line endings, or CRLF line endings on a non-Windows box. Using .gitattributes, we can correct that so that files have the right endings for the platform that it's on.



 Comments   
Comment by John Szakmeister [ 17/Jul/12 2:26 PM ]

Patch to fix line endings and introduce .gitattributes.

Comment by Stuart Halloway [ 20/Jul/12 4:47 PM ]

This looks like a change to every line in the world, which makes it hard to vet. Also: will it render incompatible all other outstanding patches at the time it is applied?

Comment by John Szakmeister [ 21/Jul/12 6:52 AM ]

You can use git diff -w $(git merge-base HEAD master) to see the actual diff minus the line ending change. Here it is inline:

:: git diff -w $(git merge-base HEAD master)
diff --git a/.gitattributes b/.gitattributes
new file mode 100644
index 0000000..7b89cfa
--- /dev/null
+++ b/.gitattributes
@@ -0,0 +1,6 @@
+*.txt           text
+*.clj           text
+*.html          text
+*.js            text
+*.css           text
+*.java          text diff=java

Also, no, it won't render all the outstanding patches incompatible. For one, it seems like the files that have the eols mixed or in CRLF aren't touched much. I also went through the majority of outstanding patches and couldn't fix one that conflicts. Secondly, format-patch emits the patch inline and is intended to be sent via email. SMTP says that all lines must end with CRLF, so line endings are effectively lost. So git will convert it to the right line ending on application.

It can conflict with any outstanding branches that you may have. Supplying the renormalization option on merge can help:

git merge -X renormalize <branch-name>

Or, you can enable this by default for your repository:

git config --local merge.renormalize true

If you think it's too much trouble, let's just drop it though.

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

Patch does not apply on my working copy of Clojure. I am using

git am -s ...
/Users/stu/repos/clojure/.git/rebase-apply/patch:344: trailing whitespace.
  p {  	
/Users/stu/repos/clojure/.git/rebase-apply/patch:350: space before tab in indent.
  	margin-left: 0.5in;
error: patch failed: epl-v10.html:1
error: epl-v10.html: patch does not apply
error: patch failed: src/jvm/clojure/asm/AnnotationVisitor.java:1
error: src/jvm/clojure/asm/AnnotationVisitor.java: patch does not apply
error: patch failed: src/jvm/clojure/asm/AnnotationWriter.java:1
error: src/jvm/clojure/asm/AnnotationWriter.java: patch does not apply

I am willing to do this, just inept.

Comment by Andy Fingerhut [ 10/Aug/12 1:21 PM ]

Stuart, I updated this page http://dev.clojure.org/display/design/JIRA+workflow a while back when I had trouble applying some patches involving files with carriage return line endings. I did some Googling on git docs and found the option '--keep-cr' that seems to help in such cases. Try that out.

Comment by Alex Miller [ 04/Sep/13 12:36 PM ]

I think in this case, we should not provide a patch but a process. And I would like to see this cleaned up.

Comment by Andy Fingerhut [ 04/Sep/13 1:14 PM ]

I am not sure what kind of process you mean, Alex, but here are some thoughts.

There are some patches attached to tickets right now that modify Java source files that have CR/LF line endings throughout. Typically those patches remove some lines, and add new ones, and those new lines typically have CR/LF line endings, too, because whatever editor the person used while making the patch saw the file have CR/LF line endings, and preserved that on new lines of text they added to the file.

So if you want to get rid of CRs throughout the Clojure source code, and keep them out, one way would be to:

(1) do a big commit to get rid of all of the current CR characters in source files.

(2) update the few pending patches that introduce lines with CRs (I checked. There are only 15 tickets with such patches right now.)

(3) I can automate a process of detecting patches that have CRs and flag them for CR removal.

In fact (2) can be done as a result of (3).

Comment by Alex Miller [ 04/Sep/13 2:29 PM ]

I just mean that any patch for this ticket is doomed to be continuously broken until it is applied, so it would be better to instead have a set of steps that someone can follow to fix all line endings, than a patch. That is #1 on your list should be a set of steps. #2 and 3 would be nice as well.

Comment by Andy Fingerhut [ 04/Sep/13 7:48 PM ]

clj-1026-bash-script.sh is not a patch, but a bash script that can be run in an unmodified Clojure source tree, preferably freshly cloned, that will delete carriage returns characters from all files except those in the .git directory, and directories.

It also prints out the results of a few commands to help you gain confidence that those are the only changes it made.

Tested on Mac OS X 10.8.4 and Ubuntu Linux 12.04.3.





[CLJ-976] Implement reader literal and print support for PersistentQueue data structure Created: 27/Apr/12  Updated: 22/Aug/13

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

Type: Enhancement Priority: Minor
Reporter: Fogus Assignee: Fogus
Resolution: Unresolved Votes: 3
Labels: data-structures, queue, reader, tagged-literals

Attachments: File CLJ-976-queue-literal-eval-and-synquote.diff     Text File clj-976-queue-literal-eval-and-synquote-patch-v3.txt     File CLJ-976-queue-literal-eval.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Clojure's PersistentQueue structure has been in the language for quite some time now and has found its way into a fair share of codebases. However, the creation of queues is a two step operation often of the form:

(conj clojure.lang.PersistentQueue/EMPTY :a :b :c)

;=> #<PersistentQueue clojure.lang.PersistentQueue@78d5f6bc>

A better experience might be the following:

#queue [:a :b :c]

;=> #queue [:a :b :c]

(pop #queue [:a :b :c])

;=> #queue [:b :c]

This syntax is proposed and discussed in the Clojure-dev group at https://groups.google.com/forum/?fromgroups#!topic/clojure-dev/GQqus5Wycno

Open question: Should the queue literal's arguments eval? The implications of this are illustrated below:

;; non-eval case
#queue [1 2 (+ 1 2)]

;=> #queue [1 2 (+ 1 2)]


;; eval case
#queue [1 2 (+ 1 2)]

;=> #queue [1 2 3]

The answer to this open question will determine the implementation.



 Comments   
Comment by Steve Miner [ 27/Apr/12 10:18 AM ]

I think the non-eval behavior would be consistent with the other reader literals in Clojure 1.4. It's definitely better for interop where some other language implementation could be expected to handle a few literal representations, but not the evaluation of Clojure expressions. Use a regular function if the args need evaluation.

Comment by Chas Emerick [ 27/Apr/12 10:19 AM ]

The precedent of records seems relevant:

=> (defrecord A [b])
user.A
=> #user.A[(+ 4 5)]
#user.A{:b (+ 4 5)}
=> #user.A{:b (+ 4 5)}
#user.A{:b (+ 4 5)}

This continues to make sense, as otherwise queues would need to print with an extra (quote …) form around lists — which records neatly avoid:

=> (A. '(+ 4 5))
#user.A{:b (+ 4 5)}

Does this mean that a queue fn (analogous to vector, maybe) will also make an appearance? It'd be handy for HOF usage.

Comment by Fogus [ 27/Apr/12 11:00 AM ]

Added a patch for the tagged literal support ONLY. This is only one part of the total solution. This provides the read-string and printing capability. I'd like more discussion around the eval side before I get dive into the compiler.

Comment by Paul Michael Bauer [ 27/Apr/12 6:45 PM ]

In addition to Chas' observations on consistency with record literals, would eval in queue literals open up the same security hole as #=, needing to respect *read-eval*?
When needing eval inside a queue literal, embedding a #= seems more apropos.

Comment by Fogus [ 04/May/12 1:14 PM ]

Evalable queue literal support.

Comment by Andy Fingerhut [ 10/May/12 5:54 PM ]

Neither of the patches CLJ-976-queue-literal-tagged-parse-support-only.diff dated Apr 27, 2012 nor CLJ-976-queue-literal-eval.diff dated May 4, 2012 apply cleanly to latest master as of May 10, 2012.

Comment by Fogus [ 11/May/12 10:15 AM ]

Updated patch file to merge with latest master.

Comment by Fogus [ 20/Jul/12 1:14 PM ]

New patch with support fixed for syntax-quote.

Comment by Stuart Sierra [ 17/Aug/12 12:41 PM ]

Patch does not apply as of commit f5f4faf95051f794c9bfa0315e4457b600c84cef

Comment by Fogus [ 17/Aug/12 3:06 PM ]

Weird. I was able to download the CLJ-976-queue-literal-eval-and-synquote.diff patch and apply it to HEAD as of just now (f5f4faf95051f794c9bfa0315e4457b600c84cef). There were whitespace warnings, but the patch applied, compiles and passes all tests.

Comment by Andy Fingerhut [ 17/Aug/12 7:29 PM ]

With latest head I was able to successfully apply patch CLJ-976-queue-literal-eval-and-synquote.diff with this command:

git am --keep-cr -s < CLJ-976-queue-literal-eval-and-synquote.diff

with some warnings, but successfully applied. If I try it without the --keep-cr option, the patch fails to apply. I believe this is often a sign that either one of the files being patched, or the patch itself, contains CR/LF line endings, which some of the Clojure source files definitely do.

The command above (with --keep-cr) is currently the one recommended for applying patches on page http://dev.clojure.org/display/design/JIRA+workflow in section "Screening Tickets". I added the suggested --keep-cr option after running across another patch that applied with the option, but not without it.

Comment by Andy Fingerhut [ 28/Aug/12 5:45 PM ]

Presumptuously changing Approval from Incomplete back to Test, since the latest patch does apply cleanly if --keep-cr option is used.

Comment by Rich Hickey [ 08/Sep/12 6:48 AM ]

this needs more time

Comment by Fogus [ 18/Sep/12 8:15 AM ]

Rich,

Do you mind providing a little more detail? I would be happy to make any changes if needed. However, if it's just a matter of its relationship to EDN and/or waiting until the next release then I am happy to wait. In either case, I'd like to complete this or push it to the back of my mind. Thanks.

Comment by Andy Fingerhut [ 05/Oct/12 7:49 AM ]

clj-976-queue-literal-eval-and-synquote-patch-v2.txt dated Oct 5 2012 is identical to Fogus's patch CLJ-976-queue-literal-eval-and-synquote.diff dated Jul 20 2012. It simply removes one line addition to clojure.iml that Rich has since added in a different commit, so that this patch now applies cleanly to latest master.

Comment by Andy Fingerhut [ 16/Oct/12 12:20 PM ]

clj-976-queue-literal-eval-and-synquote-patch-v3.txt dated oct 16 2012 is identical to Fogus's patch CLJ-976-queue-literal-eval-and-synquote.diff dated Jul 20 2012. It simply removes one line addition to clojure.iml that Rich has since added in a different commit, so that this patch now applies cleanly to latest master.

Comment by Andy Fingerhut [ 20/Oct/12 12:26 PM ]

Fogus, with the recent commit of a patch for CLJ-1070, my touched-up patch clj-976-queue-literal-eval-and-synquote-patch-v3.txt dated Oct 16 2012 doesn't apply cleanly. In this case it isn't simply a few lines of context that have changed, it is the interfaces that PersistentQueue implements have been changed. It might be best if you take a look at the latest code and the patch and consider how it should be updated.

Comment by Steve Miner [ 06/Apr/13 8:07 AM ]

Related to CLJ-1078.

Comment by Alex Miller [ 22/Aug/13 10:38 PM ]

Moving back to Triaged as Rich has not vetted.





[CLJ-373] update-in with empty key paths Created: 04/Jun/10  Updated: 24/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File 0001-CLJ-373-Alter-behaviour-of-update-in-with-empty-key-.patch     Text File 0001-Support-empty-path-in-update-in.-CLJ-373.patch     Text File clj-373-alter-behavior-of-update-in-with-empty-key-patch2.txt     Text File CLJ-373-nested-ops.patch    
Patch: Code and Test
Approval: Triaged
Waiting On: Chouser

 Description   

To the topic of get-in and update-in. While I realize this is not a bug it is odd and in my eyes unexpected and unwanted behavior.

get-in called with an empty path returns the hashmap it was given to walk through - this is as I expect and makes a lot of sense when dynamically (with generated pathes ) walking a hash map.

update-in behaves differently and while from the implementation side, it's behavior too makes sense, it does not work as expected (at least not for me) update-in with an empty map creates a new key 'nil' so:

(update-in {...} [] f) ist he same as (update-in {...} [nil] f) while (get-in {...} []) is not the same as (get-in {...} [nil]) and of cause differs from what update-in does.

For automatically walking trees the behavior of get-in makes a lot more sense since the current behavior of update-in forces you to check for empty paths and if they are empty fall back to plain assoc and get (or get-in since this works):

(if-let [r (butlast @path)]
(do
  (alter m update-in r dissoc (last @path))
  (alter m update-in r assoc {:name @sr} c))
(do
  (alter m dissoc (last @path))
  (alter m assoc {:name @sr} c)))

Next argument is that update-in with an empty map working on nil isn't easy to gasp, one needs to know the implementation details to realize that it works, I think 90% of the people reading update-in with [] will not instinctively know that it works on the key nil, so changing this would most likely not break any current code, and if it would the code would be bad anyway .

Chouser has, a very nice solution on the mailing list that would fix the problem I'm not sure if I'm entitled to post it here since I did not wrote it but it can be found in this thread: http://groups.google.com/group/clojure/browse_thread/thread/de5b20b8c3fe498b?hl=en

Regards,
Heinz



 Comments   
Comment by Assembla Importer [ 08/Oct/10 10:33 AM ]

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

Comment by Chouser [ 13/Nov/10 6:37 PM ]

I've attached my code from the g.group thread in the form of a patch, in case it's sufficient. (Thanks to abedra for the gentle kick in the pants.)

Comment by Stuart Halloway [ 29/Nov/10 8:20 AM ]

Looks to me like the patch is misusing if-let, e.g.

(when-let [[k & mk] []] "Doh!") 
=> Doh!

Please correct, and add tests for nil and [] keys (at least).

Comment by Scott Lowe [ 08/May/12 12:20 PM ]

I will write some tests and correct this.

Comment by Scott Lowe [ 09/May/12 8:39 PM ]

I'm sorry to report that my good intentions of wanting to help clear some of the project backlog has created more work by way of further questions. I'd also like to clarify the desired new behaviours for the test cases.

Heinz proposed that an empty key sequence will not create a new nil key in the returned map.
He also suggested that the following behaviour changes be made (compare old and new* behaviours):

 
(update-in {1 2} [] (constantly {2 3}))
{nil {2 3}, 1 2}

(update-in* {1 2} [] (constantly {2 3}))
{2 3}

(update-in {1 2} [] assoc  1 3)
{nil {1 3}, 1 2}

(update-in* {1 2} [] assoc  1 3)
{1 3}

Chouser also added that nil keys should be honoured, as before:

 
(update-in* {nil 2} [nil] (constantly 3))
{nil 3}

I've added a variety of tests to cover the existing behaviour and would like to confirm that the above is all that's required for new behaviour.

The patch from November 2010 didn't work, but I tweaked it with a when-let as Stuart suggested and placed a check for an empty sequence of keys before the when-let block; because essentially, the primary behaviour change boils down to simply handling an empty sequence of keys, in addition to the existing behaviours.

I'm not entirely convinced that these changes are a good thing, but at least there's now something concrete for discussion. Please have a look at what is there. The good news is that at least there are some tests covering update-in now.

Comment by Andy Fingerhut [ 24/May/12 10:35 PM ]

clj-373-alter-behavior-of-update-in-with-empty-key-patch2.txt dated May 24, 2012 supersedes patch 0001-CLJ-373Alter-behaviour-of-update-in-with-empty-key.patch dated May 9, 2012. It makes no changes to the contents of the patch except in the lines of context that have changed since the earlier patch was created. It applies cleanly to the latest master whereas the earlier patch no longer does. Builds and passes tests with Oracle/Apple JDK 1.6 on Mac OS X 10.6.8.

Comment by Fogus [ 15/Aug/12 11:21 AM ]

This patch creates a new error mode highlighted by the following:

(update-in {:a 1} [] inc)

; ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.lang.Number

The reason is that the code that executes in the event that the keys are empty (or nil) blindly assumes that the function given can be applied to the map itself. This is no problem in the case of assoc and the like, but clearly not for inc and many other useful functions.

The old behavior was to throw an NPE and if any code relies on that fact is now broken. Maybe this is not a problem, but I'm kicking it back to get a little more discussion and to request that whatever the ultimate fix happens to be, its behavioral changes and error modes are noted in the docstring for update-in.

Comment by Gunnar Völkel [ 08/Sep/12 6:11 AM ]

I vote for changing `update-in` to support empty key vectors.

Because I think "(apply f m args)" in case of an empty vector is the natural continuation of the usual behavior of update-in:

  • update-in with 2 keys the second level map is updated
  • update-in with 1 key the first level map (child of given map) is updated
  • update-in with no key the given map (zero level) is updated.

Otherwise, you will always have to wrap update-in in something like the following when the keys vector is computed:

(if (seq keys) (apply update-in m keys f args) (apply f m args))

To Fogus' last post: (update-in {:a {:b 1}} [:a] inc) fails similar and is not handled with a special exception.

Comment by Tim McCormack [ 25/Nov/13 11:45 AM ]

Fogus, if there was code relying on that brokenness, I'd say it was only working by accident.

Comment by Ambrose Bonnaire-Sergeant [ 05/Jan/14 9:13 AM ]

Attached new patch CLJ-373-nested-ops.patch.

It implements update-in with an empty path equivalent to (apply f m args).

Since they are clearly related, I changed assoc-in to throw an error on an empty path, and updated {update,get,assoc}-in's docstring to reflect these changes.

I changed the terminology of a "sequence of keys" to a "path of keys (a sequence)", and henceforth referred to the sequence as a key path, or the name of the related arg.

Comment by Andy Fingerhut [ 14/Feb/14 11:42 AM ]

Patch CLJ-373-nested-ops.patch dated Jan 5 2014 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.

Comment by Ambrose Bonnaire-Sergeant [ 24/Mar/14 12:39 PM ]

Updated with clean patch.





[CLJ-1357] It's a small typo in the gen-class doc-string Created: 17/Feb/14  Updated: 17/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File CLJ-1357-its-typo.patch    
Patch: Code
Approval: Triaged

 Description   

"It's" should be "its" (possessive) in "It's return value is ignored."






[CLJ-1329] Unused local variable in PersistentVector.cons() Created: 22/Jan/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Trivial
Reporter: Smit Shah Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File fix.patch    
Patch: Code
Approval: Triaged

 Description   

in src/jvm/clojure/lang/PersistentVector.java:168, there is an integer i being defined which is not being used anywhere in the method.

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/PersistentVector.java#L168



 Comments   
Comment by Stuart Halloway [ 31/Jan/14 6:14 PM ]

Smit, can you please submit a CA? http://clojure.org/contributing

Comment by Smit Shah [ 02/Feb/14 1:16 PM ]

Stuart, I will send the CA via post ASAP.
It might take a couple of days to reach Rich though.

Comment by Smit Shah [ 01/Mar/14 11:51 AM ]

Stuart, I have successfully submitted the CA (http://clojure.org/contributing).
I guess now merging this patch shouldn't be a problem

Comment by Alex Miller [ 02/Mar/14 11:37 AM ]

Thanks Smit!





Generated at Tue Apr 15 23:37:50 CDT 2014 using JIRA 4.4#649-r158309.